Florian Quèze [:florian] Assignee
	Description • 10 years ago

The global sharing indicator that landed in bug 1037408 is sometimes mis-centered when there's an external screen plugged in.

I've seen this only on Mac (where we no longer show that popup-indicator), but I expect the bug to also exist on Windows/Linux.

By using nsIScreenManager.primaryScreen instead of window.screen, we may get a more correct value for the width of the screen.

	Florian Quèze [:florian] Assignee
	Comment 1 • 10 years ago

Now that I've figured out which screen coordinates we want to use while working on bug 1041663, it made sense to also fix this.

This bug is visible only if:
- the Firefox browser window is displayed on the external monitor at the time we show the webrtc indicator.
- the external screen and the primary screen don't have the same width.

	Marco Mucci [:MarcoM]
	Comment 2 • 10 years ago

Added to Iteration 34.1

	Justin Dolske [:Dolske]
	Comment 3 • 10 years ago

Comment on attachment 8466226 [details] [diff] [review]
Patch

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

::: browser/base/content/webrtcIndicator.js
@@ +133,5 @@
> +      // Until we have moved the window to y=0, 'screen.width' may give a value
> +      // for a secondary screen, so use values from the screen manager instead.
> +      let width = {};
> +      Cc["@mozilla.org/gfx/screenmanager;1"].getService(Ci.nsIScreenManager)
> +        .primaryScreen.GetRectDisplayPix({}, {}, width, {});

Hmm. When there are multiple screens, what does WebRTC share? Is it just one of the screens (and which one), or does it share everything (union rect)?

If it's sharing only 1 physical screen, the indicator should be centered there. If it's all screens, I'm not sure if there's a clearly preferred place to put the indicator (especially given variations in physical arrangement)... The primary seems reasonable (since that's where the Windows taskbar / OS X menubar is, and presumably similar for Linux). OTOH one could argue for it being the screen where the current Firefox window is, or perhaps having an indicator on every screen to ensure (1) visibility and (2) clarity that everything is being shared.

The last point maybe merits some thinking -- it might feel like overkill (I _think_ most legit uses tend to show your self-sharing view, so it's obvious what's being sent), but it could be a serious privacy issue if someone thinks only one physical screen is being shared, and starts doing something they don't want shared on another screen.

What do other screensharing apps do?

	Justin Dolske [:Dolske]
	Comment 4 • 10 years ago

Philipp: any opinion here?

	Florian Quèze [:florian] Assignee
	Comment 5 • 10 years ago

(In reply to Justin Dolske [:Dolske] from comment #3)

> ::: browser/base/content/webrtcIndicator.js
> @@ +133,5 @@
> > +      // Until we have moved the window to y=0, 'screen.width' may give a value
> > +      // for a secondary screen, so use values from the screen manager instead.
> > +      let width = {};
> > +      Cc["@mozilla.org/gfx/screenmanager;1"].getService(Ci.nsIScreenManager)
> > +        .primaryScreen.GetRectDisplayPix({}, {}, width, {});
> 
> Hmm. When there are multiple screens, what does WebRTC share? Is it just one
> of the screens (and which one), or does it share everything (union rect)?

Firefox 33 is only able to share the primary screen.

In the future (34+ but it's not implemented yet), we are hoping to display a list of screens in the permission prompt and to let the user select the screen he wants to share (like we let the user select the application he wants to share). Once we do that, we will probably want to rethink on which screen the sharing indicator should be displayed.

For now, this bug is just about centering correctly the indicator on the screen where it is currently displayed.

	Justin Dolske [:Dolske]
	Comment 6 • 10 years ago

Ah, ok, that makes this bug simpler then.

	Florian Quèze [:florian] Assignee
	Comment 7 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/a4c0a19cf1cf

	Florian Quèze [:florian] Assignee
	Comment 8 • 10 years ago

Comment on attachment 8466226 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1037408
[User impact if declined]: the global indicator may appear at the wrong place if the browser window is displayed on an external screen.
[Describe test coverage new/current, TBPL]: will be verified by QA once it reaches m-c.
[Risks and why]: Low, self contained change.
[String/UUID change made/needed]: none.

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 10 years ago

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

	Marco Mucci [:MarcoM]
	Comment 10 • 10 years ago

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

	Florian Quèze [:florian] Assignee
	Comment 11 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/2597220a5926

	Bogdan Maris, Desktop QA
	Comment 12 • 10 years ago

I tried to reproduce the initial issue on four different machines using Nightly build (21.07.2014) but with no success. Tried on Windows 7 and Ubuntu OSs.

Steps:
1. Changed the resolution of the secondary screen to 800x600 and the primary to 1920x1080
2. Started Nightly on the secondary screen
3. Visited http://queze.net/goinfre/ff_gum_test.html
4. Click on Video and Share

Actual: The global indicator opens in the primary screen and it`s centered.

Setting this [qa-] for now since I can`t reproduce the initial issue. If anyone else can reproduce on a build before the fix and can then confirm that this is fixed please flip the [qa+] keyword back.