1141122 - Replace 'about:conversation' in Loop gUM doorhanger with a more meaningful alternative source

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

During the Window/ tabsharing feature review the issue was raised that the source mentioned in the Screenshare gUM doorhanger, anchored to the conversation window, mentions 'about:loopconversation', which is not useful.

For Fx38 we decided to replace this source with the Conversation/ room name.

In Fx39 we'll be changing the string, but that's not viable for Fx38 (string freeze).

Comment 1

[Mike de Boer [:mikedeboer]]

Mark, we saw this when you were sharing your screen on Vidyo, but the thing is that I see '...with this page' instead of '...with about:loopconversation'.

So my question is: was this bug perhaps due to something you had applied locally?

Comment 2

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #1)
> Mark, we saw this when you were sharing your screen on Vidyo, but the thing
> is that I see '...with this page' instead of '...with
> about:loopconversation'.

That sounds like you're looking at the prompts after permission is granted. Try looking at the window sharing prompt before granting permission.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: supply a more canonical alternative to the about:loopconversation

This will display 'Firefox Hello' for gUM permission request prompts inside Loop conversation windows. (Only shown for Window sharing, to select a window)

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8576685 [details] [diff] [review]
Patch v1: supply a more canonical alternative to the about:loopconversation

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

::: browser/modules/webrtcUI.jsm
@@ +188,5 @@
>    let chromeWin = chromeDoc.defaultView;
> +
> +  // Special case-ing Loop/ Hello gUM requests.
> +  if (host == "about:loopconversation" && "LoopUI" in chromeWin)
> +    host = chromeWin.LoopUI.getUserMediaHost();

Can this go in the "getHost" method that already has a special case for 'about:'?

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: supply a more canonical alternative to the about:loopconversation

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8576695 [details] [diff] [review]
Patch v2: supply a more canonical alternative to the about:loopconversation

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

::: browser/modules/webrtcUI.jsm
@@ +170,5 @@
>        // For about URIs, just use the full spec, without any #hash parts
>        host = uri.specIgnoringRef;
> +      // Special case-ing Loop/ Hello gUM requests.
> +      if (host == "about:loopconversation" && "LoopUI" in chromeWin)
> +        host = chromeWin.LoopUI.getUserMediaHost();

Introducing a dependency on chromeWin is annoying here. I would prefer if you just used Services.strings, like the 'else' case does.

@@ +188,4 @@
>    let chromeDoc = aBrowser.ownerDocument;
>    let chromeWin = chromeDoc.defaultView;
> +  let uri = Services.io.newURI(aRequest.documentURI, null, null);
> +  let host = getHost(uri, null, chromeWin);

This isn't the only caller of getHost.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: supply a more canonical alternative to the about:loopconversation

Like this, you mean?

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8576704 [details] [diff] [review]
Patch v2: supply a more canonical alternative to the about:loopconversation

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

::: browser/modules/webrtcUI.jsm
@@ +167,5 @@
>    } catch (ex) {};
>    if (!host) {
>      if (uri && uri.scheme.toLowerCase() == "about") {
>        // For about URIs, just use the full spec, without any #hash parts
>        host = uri.specIgnoringRef;

This line looks like it should go in a 'else' block. And the comment above changed to say "For other about URIs..."

@@ +191,5 @@
>    let uri = Services.io.newURI(aRequest.documentURI, null, null);
>    let host = getHost(uri);
>    let chromeDoc = aBrowser.ownerDocument;
>    let chromeWin = chromeDoc.defaultView;
> +

Please revert this whitespace-only change.

Comment 9

[Mike de Boer [:mikedeboer]]

Pushed to fx-team with comment addressed: https://hg.mozilla.org/integration/fx-team/rev/912179142d7e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/912179142d7e

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8576704 [details] [diff] [review]
Patch v2: supply a more canonical alternative to the about:loopconversation

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello screensharing milestone
[User impact if declined]: User will see a new option that allows her/ him to share a window or Firefox tabs inside a room (aka. conversation).
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor
[String/UUID change made/needed]: n/a

Comment 12

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d98504aa80c2

Comment 13

[Bogdan Maris, Desktop Test Engineering]

Verified that 'about:loopconversation' was replaced with 'Firefox Hello' in doorhanger across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) on latest Nightly. I`m gonna close this bug as verified but will get back to verify on Aurora tomorrow.

Comment 14

[Bogdan Maris, Desktop Test Engineering]

Verified on Firefox Developer edition as well across platforms (Windows 8.1 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit).