1010325 - Handle about:loop* pages in WebRTC permission prompts

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

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

Comment 1

[Mark Banner (:standard8)]

Attachment: Work around for uri.host being undefined in webrtc prompts

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.

Comment 2

[Adam Roach [:abr]]

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.

Comment 3

[Mark Banner (:standard8)]

Attachment: Work around for uri.host being undefined in webrtc prompts v2

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

Comment 4

[Florian Quèze [:florian]]

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.

Comment 5

[Mark Banner (:standard8)]

(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.

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Mark Banner (:standard8)]

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

Comment 8

[Mark Banner (:standard8)]

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

Comment 9

[u279076]

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

Comment 10

[Mark Banner (:standard8)]

(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.