1139907 - Show WebRTC screen sharing icon in conversation window and globally toolbar when tab sharing is active

Status: VERIFIED FIXED

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

Description

[Mike de Boer [:mikedeboer]]

Right now you'll see a screen sharing icon added next to the conversation windows' window controls when Window Sharing is activated.

This is not (yet) the case when Tab Sharing is activated, but it should show the same icon as Window Sharing.

When you click the icon, a doorhanger should drop down with exactly the same content as the Window Sharing one.

Comment 1

[Mike de Boer [:mikedeboer]]

[Tracking Requested - why for this release]: Loop/ Hello window & tab sharing is slated for Fx38, this requesting to track this bug for uplift when it's resolved.

Comment 2

[Mark Banner (:standard8)]

We should do the global indicator at the same time I suspect.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: show WebRTC screen sharing notification icon in conversation window

Randell, can you take a look at the MediaManager changes I needed to make?

Florian, can you take a look at the WebRTC (content-)UI changes I needed to make?

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8575280 [details] [diff] [review]
Patch v1: show WebRTC screen sharing notification icon in conversation window

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

::: browser/modules/ContentWebRTC.jsm
@@ +206,2 @@
>        if (state.showScreenSharingIndicator != "Screen")
> +        state.showScreenSharingIndicator = videoSource;

If you are adding a new value for showScreenSharingIndicator, please update the comment at http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm?rev=0b8ec9a2c5e3#53

Also, do we need new strings in http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties?rev=6b4aa57860cf#570 for the sharing menu and in http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/webrtcIndicator.properties for the global indicator?

::: browser/modules/webrtcUI.jsm
@@ +794,2 @@
>    options = {
> +    hideNotNow: !isBrowserSharing,

What's the reason for this change?

@@ +816,5 @@
>        mm.sendAsyncMessage("webrtc:StopSharing", "screen:" + windowId);
>      }
>    }];
> +
> +  // Ending browser-sharing from the gUM doorhanger is not supported at the moment.

Is there a bug on file to support this? If so, could you please include the bug number in this comment?

@@ +817,5 @@
>      }
>    }];
> +
> +  // Ending browser-sharing from the gUM doorhanger is not supported at the moment.
> +  if (isBrowserSharing) {

You are adding a { but I see no }, is the file still parsable?

Comment 5

[Mike de Boer [:mikedeboer]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> If you are adding a new value for showScreenSharingIndicator, please update
> the comment at
> http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.
> jsm?rev=0b8ec9a2c5e3#53
> 
> Also, do we need new strings in
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.properties?rev=6b4aa57860cf#570 for the sharing menu and in
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/webrtcIndicator.properties for the global indicator?

Yes, I think so, but that will be something to add for Fx39. This feature is intended for Fx38, so I'd like to add proper strings in a follow-up.

> What's the reason for this change?

If I don't set this to false when I set secondaryActions to `null`, PopupNotifications.jsm will through a (validation) error.

> Is there a bug on file to support this? If so, could you please include the
> bug number in this comment?

Not yet. I'll file a bug once you think this patch is reasonable.

> You are adding a { but I see no }, is the file still parsable?

Oops! It _is_ there locally, but apparently not when I exported the patch.

Comment 6

[Mike de Boer [:mikedeboer]]

s/through/throw/

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8575280 [details] [diff] [review]
Patch v1: show WebRTC screen sharing notification icon in conversation window

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

r+ - some day we need to refactor this (and rename "browsershare"), but that's for another day

::: dom/media/MediaManager.h
@@ +149,5 @@
> +  {
> +    NS_ASSERTION(NS_IsMainThread(), "Only call on main thread");
> +    // XXXmikedeboer: IsAvailable() returns |true| for browser sharing, for which
> +    //                I don't have a reasonable explanation.
> +    return mVideoSource && !mStopped && mVideoSource->IsAvailable() &&

IsAvailable() is false if the device is allocated/started (MediaEngine.h)

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: show WebRTC screen sharing notification icon in conversation window

Carrying over r=jesup.

Comment 9

[Florian Quèze [:florian]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> (In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > If you are adding a new value for showScreenSharingIndicator, please update
> > the comment at
> > http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.
> > jsm?rev=0b8ec9a2c5e3#53
> > 
> > Also, do we need new strings in
> > http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> > browser/browser.properties?rev=6b4aa57860cf#570 for the sharing menu and in
> > http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> > browser/webrtcIndicator.properties for the global indicator?
> 
> Yes, I think so, but that will be something to add for Fx39. This feature is
> intended for Fx38, so I'd like to add proper strings in a follow-up.

So what happens for 38 for the sharing menu and the global indicator? I saw you are falling back to the 'screen' string for the sharing doorhanger. Do you need to implement similar things for the other parts of the UI?

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> So what happens for 38 for the sharing menu and the global indicator? I saw
> you are falling back to the 'screen' string for the sharing doorhanger. Do
> you need to implement similar things for the other parts of the UI?

It'll fall back to the "Window" string, which is more or less accurate. This is just for the strings inside the doorhanger. I don't see a problem for the global indicator, as this is specific only to Loop.

Comment 11

[Florian Quèze [:florian]]

(In reply to Mike de Boer [:mikedeboer] from comment #10)

> I don't see a problem for the
> global indicator, as this is specific only to Loop.

Is Loop receiving a special treatment for the global indicator? I thought it was shown there like other pages.

Comment 12

[Mike de Boer [:mikedeboer]]

(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> Is Loop receiving a special treatment for the global indicator? I thought it
> was shown there like other pages.

No special treatment indeed.

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: show WebRTC screen sharing notification icon in conversation window

Unbreaking the global indicators. Carrying over r=jesup.

Comment 14

[Florian Quèze [:florian]]

Comment on attachment 8575889 [details] [diff] [review]
Patch v3: show WebRTC screen sharing notification icon in conversation window

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

Looks good, but I would still like to have a quick look at the final version of the patch.

::: browser/base/content/webrtcIndicator.js
@@ +57,5 @@
>    // Screen sharing button tooltip.
>    let screenShareButton = document.getElementById("screenShareButton");
>    if (webrtcUI.showScreenSharingIndicator) {
> +    let typeForL10n = webrtcUI.showScreenSharingIndicator == "Browser" ?
> +      "Window" : webrtcUI.showScreenSharingIndicator;

I wonder if this would be more readable:
  let typeForL10n = webrtcUI.showScreenSharingIndicator;
  if (typeForL10n == "Browser")
    typeForL10n = "Window";

Should this include the bug number of where the strings will be added?

::: browser/modules/ContentWebRTC.jsm
@@ +206,1 @@
>        if (state.showScreenSharingIndicator != "Screen")

Should this also check that state.showScreenSharingIndicator is different from "Window"? This is for the case of a web page currently sharing a window and a different page sharing a tab.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Patch v4: show WebRTC screen sharing notification icon in conversation window

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8576023 [details] [diff] [review]
Patch v4: show WebRTC screen sharing notification icon in conversation window

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

Not exactly what I expected, but this works too! :-)

::: browser/modules/webrtcUI.jsm
@@ +820,5 @@
>      }
>    }];
> +
> +  // Ending browser-sharing from the gUM doorhanger is not supported at the moment.
> +  // See bug XXX.

Just a comment to remind you to file this bug :).

Comment 17

[Mike de Boer [:mikedeboer]]

Thanks! I filed bug 1142091 for comment 16.

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/9cfd185f1ff8

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cfd185f1ff8

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8576023 [details] [diff] [review]
Patch v4: show WebRTC screen sharing notification icon in conversation window

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]: The nsIMediaManagerService has a new UUID, because one additional, optional, argument was added to the `MediaCaptureWindowState` method.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8576023 [details] [diff] [review]
Patch v4: show WebRTC screen sharing notification icon in conversation window

Thanks for revving the UUID, we can take this uplift in Aurora.

Comment 21

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8acc69cac05d

Comment 22

[Bogdan Maris, Desktop QA]

Verified that the screen sharing icon shows in global indicator and conversation window when tab sharing is active 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 23

[Bogdan Maris, Desktop QA]

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