Mark Banner (:standard8) Assignee
	Description • 11 years ago

For MLP, we are planning on just using the permission prompts for the about:loopconversation page. After MLP, we'll redesign the Loop parts of the accept/permissions system punching through our special privs where necessary.

The work around is to account for the fact that about uris don't have hosts. So for Loop permissions, my suggestion is to use either the brand name, or the hostname of the Loop server.

I think this will be adequate for MLP.

I currently have a minor issue with the patch that I'm working on that will probably affect the choice of brand name versus hostname, but once I've done a bit more investigation, I should know if I can use either/or both.

Current WIP is here (although I've got additional changes locally): https://github.com/adamroach/gecko-dev/commit/796bda00f15452bf780b8a999769ee04e809824e

	Mark Banner (:standard8) Assignee
	Comment 1 • 11 years ago

This does enough to make prompts work for about:loopconversation. In the case of loop, it replaces the hostname with the short brand name, which seems reasonable for Loop's status.

This is temporary for the initial landing of loop on nightly. For the main release, we're expecting that bug 990678 will implement improvements/replacements for the permissions dialog, such that we get a simple accept/reject, possibly with some sort of device selection (although no UX flow is available yet). My suggestion is that either bug 990678 implements the changed permissions and backs out this patch, or re-examines this code and decides if it is sufficient for release, or to improve it.

I'm guessing on reviewers, so please feel free to reassign if appropraite.

	Adam Roach [:abr]
	Comment 2 • 11 years ago

Comment on attachment 8423070 [details] [diff] [review]
Work around for uri.host being undefined in webrtc prompts

Review of attachment 8423070 [details] [diff] [review]:
-----------------------------------------------------------------

From a technical perspective, this looks reasonable.

This is just a temporary workaround for MLP, so I'd like some kind of reminder structure that will lead to this being removed once we have privileged access to the camera/mic (and corresponding answer buttons) -- something like: "// TODO: Remove after Bug 1000146 lands", and add a reference to 1000146 back to this bug so it doesn't get forgotten.

	Mark Banner (:standard8) Assignee
	Comment 3 • 11 years ago

Updated to include Adam's suggested comment. Switching reviewer to Florian per discussion with Randel on irc.

	Florian Quèze [:florian]
	Comment 4 • 10 years ago

Comment on attachment 8424809 [details] [diff] [review]
Work around for uri.host being undefined in webrtc prompts v2

Review of attachment 8424809 [details] [diff] [review]:
-----------------------------------------------------------------

As long as about:loop is a local page (Standard8 confirmed on IRC: https://github.com/adamroach/gecko-dev/blob/privs-landing/browser/components/about/AboutRedirector.cpp#L100) this seems fine. If about:loop ever gets turned into a redirection to a page hosted on a web server, I think it would then be better to display the server's hostname.

I understand this is meant to be temporary only. If it's going to stay long enough to reach mozilla-release, I think it needs a test. If you are sure you'll replace this soon, then I think it's OK as is.

	Mark Banner (:standard8) Assignee
	Comment 5 • 10 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> I understand this is meant to be temporary only. If it's going to stay long
> enough to reach mozilla-release, I think it needs a test. If you are sure
> you'll replace this soon, then I think it's OK as is.

Agreed, thanks. I've added a couple of comments to bug 1000146 stating that we need to fix it before mvp, and to backout or address the review comments here.

	Mark Banner (:standard8) Assignee
	Comment 6 • 10 years ago

Landed on our integration branch:

https://github.com/adamroach/gecko-dev/commit/54e1597d51f5209107866021a99282388868f45a

This will get moved to Mercurial at some stage soon when we are able to. Closing the bug for tracking purposes - I'll comment again when this moves.

	Mark Banner (:standard8) Assignee
	Comment 7 • 10 years ago

https://hg.mozilla.org/projects/elm/rev/b9cc429a2d77

	Mark Banner (:standard8) Assignee
	Comment 8 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/b9cc429a2d77

	u279076
	Comment 9 • 10 years ago

Mark, does this need QA testing? If so, please advise.

	Mark Banner (:standard8) Assignee
	Comment 10 • 10 years ago

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #9)
> Mark, does this need QA testing? If so, please advise.

No, this code was temporary and was replaced by bug 1015486, so I'll comment there about testing.