This bug covers the implementation of the "Screen sharing doorhanger" part of attachment 8453837 [details] and of the URL bar indicator for screen sharing (image at the bottom of the first column in the mockup).

Added to Iteration 33.3

App sharing is no longer planned for Fx33, we are going to do window-sharing instead.

I started from bug 983504 attachment 8454954 [details] [diff] [review], simplified the code, and made it match what's been designed in bug 1031424. Things that aren't implemented yet: - change the label of the main action depending on what has been requested and the user has selected. - show screensharing icons in the doorhanger. - show the screensharing urlbar icon instead of the video one. - tests, if we have a good way to write them.

Add the icons from bug 1037418 attachment 8456995 [details]

This part requires jesup's patch from bug 1039529.

With the icons actually included in the patch this time.

Comment on attachment 8457033 [details] [diff] [review] Part 1 (prompt with the list of screens/windows) v2 Review of attachment 8457033 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable to me. ::: browser/locales/en-US/chrome/browser/browser.properties @@ +491,4 @@ > # The number of devices can be either one or two. > getUserMedia.shareCamera.message = Would you like to share your camera with %S? > getUserMedia.shareMicrophone.message = Would you like to share your microphone with %S? > +getUserMedia.shareScreen.message = Would you like to share your screen with %S? Nit: you should update the localization note to apply to these messages, too. ::: browser/modules/webrtcUI.jsm @@ +109,4 @@ > audioDevices.push(device); > break; > case "video": > + if (aVideo && (device.mediaSource == "camera") != sharingScreen) I'm finding this hard to read, because it essentially expands to: (device.mediaSource == "camera") != (aVideo.mediaSource != "camera") ... which has too many negations for my taste (plus a level of indirection) Is there some reason you can't just compare aVideo.mediaSource (undefined in the boolean case) to device.mediaSource? If not, can you at least add a comment with what you're trying to test here? @@ +173,5 @@ > > + if (aSecure && !sharingScreen) { > + // Don't show the 'Always' action if the connection isn't secure, or for > + // screen sharing (because we can't guess which window the user wants to > + // share without prompting). Aside: does this mean that if we're requesting microphone and screen access, we'll always prompt for microphone access, and there's no way to avoid having that in the dialog? Might want to followup improving that... @@ +266,3 @@ > let micMenupopup = chromeDoc.getElementById("webRTC-selectMicrophone-menupopup"); > + if (sharingScreen) { > + // The screensharing menu is specific, we can't just call listDevices. Can you break this out into a separate function, please? :-)

Comment on attachment 8457034 [details] [diff] [review] Part 2 (adapt the label of the main action), v1 Review of attachment 8457034 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/webrtcUI.jsm @@ +150,5 @@ > let stringId = "getUserMedia.share" + requestTypes.join("And") + ".message"; > let message = stringBundle.getFormattedString(stringId, [uri.host]); > > + let mainLabel; > + if (sharingScreen) Nit: braces for else means braces for the if, too. @@ +154,5 @@ > + if (sharingScreen) > + mainLabel = stringBundle.getString("getUserMedia.shareSelectedItems.label"); > + else { > + mainLabel = PluralForm.get(requestTypes.length, > + stringBundle.getString("getUserMedia.shareSelectedDevices.label")); Nit: put this in an intermediary.

Comment on attachment 8457040 [details] [diff] [review] Part 3 (add screensharing icons) v2 Review of attachment 8457040 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to avoid the "chuck all the things in this directory" thing that happened to browser/base/content - can we just put all the things in a webrtc/ directory under browser/themes/shared (and under chrome://browser/skin/webrtc/ for that matter) ? r=me on this bit with that fixed.

(In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #9) > ::: browser/modules/webrtcUI.jsm > @@ +109,4 @@ > > audioDevices.push(device); > > break; > > case "video": > > + if (aVideo && (device.mediaSource == "camera") != sharingScreen) > > I'm finding this hard to read, because it essentially expands to: > > (device.mediaSource == "camera") != (aVideo.mediaSource != "camera") > > ... which has too many negations for my taste (plus a level of indirection) > > Is there some reason you can't just compare aVideo.mediaSource (undefined in > the boolean case) to device.mediaSource? If not, can you at least add a > comment with what you're trying to test here? tbh, I'm not convinced that line is actually needed, as it's checking that the media manager code didn't return us a device that didn't match what we requested. What I tried to write was: <We got a camera> XOR <we requested a screen share>. > @@ +173,5 @@ > > > > + if (aSecure && !sharingScreen) { > > + // Don't show the 'Always' action if the connection isn't secure, or for > > + // screen sharing (because we can't guess which window the user wants to > > + // share without prompting). > > Aside: does this mean that if we're requesting microphone and screen access, > we'll always prompt for microphone access, and there's no way to avoid > having that in the dialog? Might want to followup improving that... Yes, and I think that's more or less by design. If you saved that you always want to allow the microphone and then a page asks for microphone+camera, we also show the full prompt.

(In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #11) > Comment on attachment 8457040 [details] [diff] [review] > Part 3 (add screensharing icons) v2 > > Review of attachment 8457040 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd like to avoid the "chuck all the things in this directory" thing that > happened to browser/base/content - can we just put all the things in a > webrtc/ directory under browser/themes/shared Ok for a webrtc subdirectory in 'shared'. > (and under > chrome://browser/skin/webrtc/ for that matter) ? I would rather avoid this, as this would make the chrome paths inconsistent for the webrtc icons (screen sharing icons would be in a subfolder, and other icons would be directly in skin/). I know Philipp plans to replace all the green sharing icons with orange ones, so that will be a good opportunity to reorganize later (I would also like to move the duplicated icons into shared/).

Comment on attachment 8457038 [details] [diff] [review] Part 4 (screensharing indicator in the url bar) v1 Review of attachment 8457038 [details] [diff] [review]: ----------------------------------------------------------------- I feel the most uneasy about this bit. Can you answer the questions I list below? I should have a chance to re-review in the next 2-3 hours, otherwise you may need to pick someone else. Sorry! ::: browser/base/content/browser-webrtcUI.js @@ +62,5 @@ > + let notif = PopupNotifications.getNotification("webRTC-sharingDevices", > + streamData.browser); > + if (!notif) { > + notif = PopupNotifications.getNotification("webRTC-sharingScreen", > + streamData.browser); This seems kind of hacky. Is there no better way to detect which of these is going on? Querying the mediamanagerservice, for instance? ::: browser/locales/en-US/chrome/browser/browser.properties @@ +512,5 @@ > getUserMedia.sharingCamera.message2 = You are currently sharing your camera with this page. > getUserMedia.sharingMicrophone.message2 = You are currently sharing your microphone with this page. > getUserMedia.sharingCameraAndMicrophone.message2 = You are currently sharing your camera and microphone with this page. > +getUserMedia.sharingScreen.message = You are currently sharing your screen with this page. > +getUserMedia.sharingWindow.message = You are currently sharing a window with this page. Can we followup improving this? It's kind of ominous to say you're sharing a window without specifying which window it is. ::: browser/modules/webrtcUI.jsm @@ +401,2 @@ > MediaManagerService.mediaCaptureWindowState(aBrowser.contentWindow, > + camera, microphone, screen, window); Conflict central. I'm moving this up into the updateIndicators method because I need the data elsewhere. Anyway, you're landing first, so go ahead... @@ +442,5 @@ > + > + // Performing an action from a notification removes it, but if the page > + // uses screensharing and a device, we may have another notification to remove. > + let outerWindowID = Services.wm.getCurrentInnerWindowWithId(windowId) > + .QueryInterface(Ci.nsIInterfaceRequestor) Nit: this is ligned up to the wrong '.' @@ +465,5 @@ > + let anchorId = captureState == "Microphone" ? "webRTC-sharingMicrophone-notification-icon" > + : "webRTC-sharingDevices-notification-icon"; > + let message = stringBundle.getString("getUserMedia.sharing" + captureState + ".message2"); > + chromeWin.PopupNotifications.show(aBrowser, "webRTC-sharingDevices", message, > + anchorId, mainAction, secondaryActions, options); Can you explain why you're showing both here? I'm somewhat confused. Won't the first just get nuked when we show the second?

(In reply to Florian Quèze [:florian] [:flo] from comment #12) > (In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #9) > > > ::: browser/modules/webrtcUI.jsm > > @@ +109,4 @@ > > > audioDevices.push(device); > > > break; > > > case "video": > > > + if (aVideo && (device.mediaSource == "camera") != sharingScreen) > > > > I'm finding this hard to read, because it essentially expands to: > > > > (device.mediaSource == "camera") != (aVideo.mediaSource != "camera") > > > > ... which has too many negations for my taste (plus a level of indirection) > > > > Is there some reason you can't just compare aVideo.mediaSource (undefined in > > the boolean case) to device.mediaSource? If not, can you at least add a > > comment with what you're trying to test here? > > tbh, I'm not convinced that line is actually needed, as it's checking that > the media manager code didn't return us a device that didn't match what we > requested. > > What I tried to write was: > <We got a camera> XOR <we requested a screen share>. That still sounds like you could just check device.mediaSource == aVideo.mediaSource...

(In reply to Florian Quèze [:florian] [:flo] from comment #13) > (In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #11) > > (and under > > chrome://browser/skin/webrtc/ for that matter) ? > > I would rather avoid this, as this would make the chrome paths inconsistent > for the webrtc icons (screen sharing icons would be in a subfolder, and > other icons would be directly in skin/). I know Philipp plans to replace all > the green sharing icons with orange ones, so that will be a good opportunity > to reorganize later (I would also like to move the duplicated icons into > shared/). OK. Let's have them straight in skin/ for now, but we should adjust this because it is likewise a bit of a footgun to have separate directories that all get merged into the same thing at jar.mn time. :-)

Comment on attachment 8457034 [details] [diff] [review] Part 2 (adapt the label of the main action), v1 Review of attachment 8457034 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to actually put this through its paces, but I don't have time before PTO. It may make sense to ask for a final review from someone else to just test that this behaves well, as I've just been staring at code. Besides that, tests and a try run would be nice... in fact, if you're unwilling/unable to spend time on tests now, please file a followup and let's ensure we address it during the next iteration.

(In reply to :Gijs Kruitbosch (PTO july 17-21) from comment #14) > ::: browser/base/content/browser-webrtcUI.js > @@ +62,5 @@ > > + let notif = PopupNotifications.getNotification("webRTC-sharingDevices", > > + streamData.browser); > > + if (!notif) { > > + notif = PopupNotifications.getNotification("webRTC-sharingScreen", > > + streamData.browser); > > This seems kind of hacky. Is there no better way to detect which of these is > going on? Querying the mediamanagerservice, for instance? It is hacky. I almost considered just not fixing that code, as we will completely remove it (bug 1037415) as soon as the global indicator lands; and the global indicator will have 2 separate buttons, so we won't have this problem. But I figured it's better to keep something non-broken in the tree, even if it just stays for 2-3 days. > ::: browser/locales/en-US/chrome/browser/browser.properties > @@ +512,5 @@ > > getUserMedia.sharingCamera.message2 = You are currently sharing your camera with this page. > > getUserMedia.sharingMicrophone.message2 = You are currently sharing your microphone with this page. > > getUserMedia.sharingCameraAndMicrophone.message2 = You are currently sharing your camera and microphone with this page. > > +getUserMedia.sharingScreen.message = You are currently sharing your screen with this page. > > +getUserMedia.sharingWindow.message = You are currently sharing a window with this page. > > Can we followup improving this? It's kind of ominous to say you're sharing a > window without specifying which window it is. Definitely something we want to change for Fx34. I discussed this in #media, and we don't have an API telling us anything about which window is being shared, and we don't feel confident we can have that soon enough for 33. All I got for 33 is a boolean (bug 1039529). > ::: browser/modules/webrtcUI.jsm > @@ +401,2 @@ > > MediaManagerService.mediaCaptureWindowState(aBrowser.contentWindow, > > + camera, microphone, screen, window); > > Conflict central. I'm moving this up into the updateIndicators method > because I need the data elsewhere. Anyway, you're landing first, so go > ahead... Sorry :-|. > @@ +465,5 @@ > > + let anchorId = captureState == "Microphone" ? "webRTC-sharingMicrophone-notification-icon" > > + : "webRTC-sharingDevices-notification-icon"; > > + let message = stringBundle.getString("getUserMedia.sharing" + captureState + ".message2"); > > + chromeWin.PopupNotifications.show(aBrowser, "webRTC-sharingDevices", message, > > + anchorId, mainAction, secondaryActions, options); > > Can you explain why you're showing both here? I'm somewhat confused. Won't > the first just get nuked when we show the second? I'm showing once the sharingDevices one, and once the sharingScreen one, as specified on the mockup.

Comment on attachment 8457038 [details] [diff] [review] Part 4 (screensharing indicator in the url bar) v1 Review of attachment 8457038 [details] [diff] [review]: ----------------------------------------------------------------- OK, per discussion on IRC, with the nits fixed I think we're good here. As noted before, it wouldn't hurt to have someone test this, and/or to have automated tests.

Carrying forward Gijs's r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6412347e6295 https://hg.mozilla.org/integration/mozilla-inbound/rev/dba1a5a7909b https://hg.mozilla.org/integration/mozilla-inbound/rev/31ba153201ec https://hg.mozilla.org/integration/mozilla-inbound/rev/fce93bbebc78

https://hg.mozilla.org/mozilla-central/rev/6412347e6295 https://hg.mozilla.org/mozilla-central/rev/dba1a5a7909b https://hg.mozilla.org/mozilla-central/rev/31ba153201ec https://hg.mozilla.org/mozilla-central/rev/fce93bbebc78

Hi Florin, can a QA contact be assigned for verification of this bug.

Hints for QA: - you need to set the media.getusermedia.screensharing.enabled pref to true - http://queze.net/goinfre/ff_gum_test.html is a test page that uses the various sharing options.

Verified that the screen/window sharing doorhangers are implemented plus did some exploratory around this on Windows 7 64bit, Ubuntu 13.04 64bit and Mac OS X 10.9.4 using latest Nightly. The dependencies will be verified individually.