Closed Bug 1139907 Opened 5 years ago Closed 5 years ago

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

Categories

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

defect
Points:
5

Tracking

(firefox38+ verified, firefox39 verified)

VERIFIED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox38 + verified
firefox39 --- verified

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Flags: qe-verify+
Flags: firefox-backlog+
[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.
Severity: normal → blocker
Priority: -- → P1
We should do the global indicator at the same time I suspect.
Summary: Show WebRTC screen sharing icon in conversation window toolbar when tab sharing is active → Show WebRTC screen sharing icon in conversation window and globally toolbar when tab sharing is active
Rank: 1
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Points: 3 → 5
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?
Attachment #8575280 - Flags: review?(rjesup)
Attachment #8575280 - Flags: review?(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?
Attachment #8575280 - Flags: review?(florian) → review-
(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.
s/through/throw/
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)
Attachment #8575280 - Flags: review?(rjesup)
Attachment #8575280 - Flags: review?(florian)
Attachment #8575280 - Flags: review-
Attachment #8575280 - Flags: review+
Attachment #8575280 - Flags: review?(florian) → review-
Carrying over r=jesup.
Attachment #8575280 - Attachment is obsolete: true
Attachment #8575297 - Flags: review?(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?
(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.
(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.
(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.
Unbreaking the global indicators. Carrying over r=jesup.
Attachment #8575297 - Attachment is obsolete: true
Attachment #8575297 - Flags: review?(florian)
Attachment #8575889 - Flags: review?(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.
Attachment #8575889 - Flags: review?(florian) → feedback+
Blocks: 1142066
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 :).
Attachment #8576023 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/9cfd185f1ff8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
QA Contact: bogdan.maris
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.
Attachment #8576023 - Flags: approval-mozilla-aurora?
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.
Attachment #8576023 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
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).
You need to log in before you can comment on or make changes to this bug.