1450658 - Should bring window to front when screen-sharing a window

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Ershov Andrew]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

Share application window.
https://mozilla.github.io/webrtc-landing/gum_test.html


Actual results:

When I started sharing of application, it must be bringed to top.
(for example, on Chrome it works correctly)


Expected results:

Application is still minimized.

Comment 1

[Stefan [:StefanG_QA]]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0 (20180404224504)

I have tested this issue on Windows 10 x64 using latest Nightly and Fx release builds.

When I press the application button I`m asked what application to share, then when I choose any application nothing happened.

On Nightly when I choose any application to share, then the displayed dialog is cut and I can not select any button to confirm.

In Chrome and Edge I`m able to successfully share my webcam view.

I think the correct component would be Core:WebRTC:Audio/Video. Please change if this is not the right component.

Comment 2

[Dan Minor [:dminor]]

I can confirm that the application remains minimized even after it is shared. It's not obvious to me that bringing the application to the top is the expected behaviour after it is shared and I was unable to verify the Chrome behaviour, visiting https://mozilla.github.io/webrtc-landing/gum_test.html in Chrome always used the camera for me regardless of which option I chose.

The current behaviour is not great, if an application window is minimized, it just shows as a yellow square. This is a side effect of how application sharing is implemented, basically it is just screensharing with black drawn over parts of the screen that are not supposed to be visible. Window sharing, on the other hand, will display the contents of a window properly even if it is minimized. (Or, that is the behaviour on Linux, anyway.)

I think a more reasonable behaviour would be that this worked like window sharing, where each window is visible regardless of whether it is minimized. Popping the application up in front of the browser when it is shared does not seem ideal to me.

:jib, do you have any thoughts on this? Is application sharing even used enough to make fixing this a priority? Thanks!

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hi Ershov, thanks for filing. Where did you verify Chrome behavior for this? I didn't know Chrome supported sharing per application.

Also, Chrome and Firefox implement different incompatible non-spec screensharing APIs, so https://mozilla.github.io/webrtc-landing/gum_test.html would not work in Chrome.

Personally, I always found application sharing weird and redundant. I'd be happy to entertain removing it, or just leave it out whenever we implement bug 1321221, unless someone wants to advocate for it.

Comment 4

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

Application sharing is useful for (example) giving a demo of how to use Gimp (which opens N windows and you move between them constantly).  The Cisco people very much wanted app sharing for providing WebEx-like features in webrtc-webex (etc).  You can ping them if you think they don't care anymore.

Comment 5

[Ershov Andrew]

Jan-Ivar Bruaroey, in Chrome I meant sharing of window.
You can choose window of some program, click "share" and it become on top.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Ershov Andrew from comment #5)
> in Chrome I meant sharing of window.

Ok, please click the [Window] button instead in https://mozilla.github.io/webrtc-landing/gum_test.html for that (Application is something different, sharing all windows of an app).

(In reply to Ershov Andrew from comment #0)
> Actual results:
> 
> When I started sharing of application, it must be bringed to top.
> 
> Expected results:
> 
> Application is still minimized.

Note that "bring to front" is not the opposite of minimized.

When I try Firefox and Chrome in OSX, I find Firefox mirrors Chrome's behavior wrt MINIMIZED windows: That is, Firefox does not list minimized windows at all. I used https://appear.in/randomroom as an example. Chrome did one better by actually removing choices live in the selector as I minimized windows more windows while the selector prompt was up.

What appears to be different from Chrome is that when capturing a non-minimized window, Firefox does not push said window to front when starting capture. Is that what this report is asking for?

Comment 7

[Dan Minor [:dminor]]

There is a WindowCapturerWin::FocusOnSelectedSource in window_capturer_win.cc that attempts to bring the window to the front. We're just not calling it at the moment.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #6)
> What appears to be different from Chrome is that when capturing a
> non-minimized window, Firefox does not push said window to front when
> starting capture. Is that what this report is asking for?

Proceeding as if it is (we just got a second independent request for this).

I've compared the "Share screen" behavior to Chrome in https://appear.in/someroom1234 (which lets you pick a window, which then is pushed to front) with https://mozilla.github.io/webrtc-landing/gum_test.html in Firefox which does not push the target window to front.

I agree pushing the window to the front should improve user experience. Thanks Ershov for filing this and for your patience!

Comment 9

[Dan Minor [:dminor]]

Attachment: Bug 1450658 - Should bring window to front when screen-sharing a window;

This plumbs through whether or not the screenshare is running in chrome and uses
that information to decide whether or not to focus the chosen window. This is
necessary because we run a live preview while choosing the window to share and
if we focused all of the time, we would bring the chosen window over top of the
prompt.

Whether or not we are running in chrome is not a natural fit with the other data
in VideoCaptureCapability and MediaEnginePrefs, but the alternative would be to
add a new IPC call to focus the window, which seems like overkill for this.

Review commit: https://reviewboard.mozilla.org/r/243360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243360/

Comment 10

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

https://reviewboard.mozilla.org/r/243360/#review249232

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc:670
(Diff revision 1)
> +  if (!capability.isChrome) {
> +    desktop_capturer_cursor_composer_->FocusOnSelectedSource();
> +  }

If the application changes some constraints (resolution, framerate, etc) with applyConstraints, then this will re-focus the window, correct?

That seems wrong.

I'd rather be explicit and add the extra IPC call so we can bring this logic out to where we have enough context to take this decision. 

MediaEngineRemoteVideoSource::Start() should be enough, as we don't Stop() a window-sharing source on track-disable, even though the gUM code in MediaManager is probably the proper place.

Comment 11

[Dan Minor [:dminor]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243360/diff/1-2/

Comment 12

[Dan Minor [:dminor]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243360/diff/2-3/

Comment 13

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

https://reviewboard.mozilla.org/r/243360/#review250238

r- because MediaEngineRemoteVideoSource shouldn't have to know about chrome. The user can care about that instead.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:191
(Diff revision 3)
>  
>    int mCaptureIndex;
>    const dom::MediaSourceEnum mMediaSource; // source of media (camera | application | screen)
>    const camera::CaptureEngine mCapEngine;
>    const bool mScary;
> +  bool mIsChrome;

MediaEngineRemoteVideoSource itself doesn't need to know whether the user is chrome or not.

It doesn't have to know what chrome is either.

Preferably I'd like to keep its state about itself or things it depend on (like mMediaSource, mCapEngine which make sense for the underlying backend).

This knowledge is currently in MediaManager. Can we keep it there? It would then be at the user's discretion to tell the source to focus on the window, and the source would just plumb it through.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:344
(Diff revision 3)
> +  if (!mIsChrome) {
> +    if (camera::GetChildAndCall(&camera::CamerasChild::FocusOnSelectedSource,
> +                                mCapEngine, mCaptureIndex)) {
> +      LOG(("FocusOnSelectedSource failed"));
> +    }
> +  }

This would still re-focus the window on Start() if the user had stopped the capture when its track was disabled, like is done for cameras.

MediaManager doesn't do this for screen/window/application captures, but it could, and it should be able to.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:347
(Diff revision 3)
>    }));
>  
> +  if (!mIsChrome) {
> +    if (camera::GetChildAndCall(&camera::CamerasChild::FocusOnSelectedSource,
> +                                mCapEngine, mCaptureIndex)) {
> +      LOG(("FocusOnSelectedSource failed"));

Is there any backend where this currently fails? If not, do we want to cover it with an assert?

::: media/webrtc/trunk/webrtc/video_engine/desktop_capture_impl.cc:677
(Diff revision 3)
> +  if (started_) {
> +    desktop_capturer_cursor_composer_->FocusOnSelectedSource();
> +  }

Why wait for it to start? Source selection seems to happen on Init, which happens during our call to Allocate().

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

https://reviewboard.mozilla.org/r/243360/#review250238

> This would still re-focus the window on Start() if the user had stopped the capture when its track was disabled, like is done for cameras.
> 
> MediaManager doesn't do this for screen/window/application captures, but it could, and it should be able to.

What is the desired behaviour? It seems to me that if the user stops the capture and then starts it again, the expectation would be that the window would focused again, but I'm not sure how this relates to the track being disabled, I'm not very familiar with this code.

Comment 15

[Andreas Pehrson [:pehrsons]]

(In reply to Dan Minor [:dminor] from comment #14)
> Comment on attachment 8974972 [details]
> Bug 1450658 - Should bring window to front when screen-sharing a window;
> 
> https://reviewboard.mozilla.org/r/243360/#review250238
> 
> > This would still re-focus the window on Start() if the user had stopped the capture when its track was disabled, like is done for cameras.
> > 
> > MediaManager doesn't do this for screen/window/application captures, but it could, and it should be able to.
> 
> What is the desired behaviour? It seems to me that if the user stops the
> capture and then starts it again, the expectation would be that the window
> would focused again, but I'm not sure how this relates to the track being
> disabled, I'm not very familiar with this code.

If the application stops by MediaStreamTrack::Stop(), that will call down to MediaEngineSource::Stop() and MediaEngineSource::Deallocate() and finally throw the source out. Re-starting capture would then have to re-do gUM, which will need user permission to finally reach down to MediaEngineSource::Allocate(), MediaEngineSource::SetTrack() and MediaEngineSource::Start().

Disabling a track through MediaStreamTrack::mEnabled = false goes to MediaEngineSource::Stop(), and enabling again by MediaStreamTrack::mEnabled = true goes to MediaEngineSource::Start().

So track-disabling/enabling can happen at the will of the application, while stopping/restarting needs user approval. The latter makes more sense for focusing an external window. The former could probably be abused for some kind of DOS attack.

Comment 16

[Dan Minor [:dminor]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243360/diff/3-4/

Comment 17

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

https://reviewboard.mozilla.org/r/243360/#review251342

Thanks, this now has the right structure. r=me with errors properly propagated.

::: dom/media/MediaManager.cpp:1724
(Diff revision 4)
>          if (mAudioDevice) {
>            mAudioDevice->Deallocate();
>          }
> +      } else {
> +        if (!mIsChrome) {
> +          rv = mVideoDevice->FocusOnSelectedSource();

You're not doing anything with `rv`. We should at least log a warning IMHO.

::: dom/media/systemservices/CamerasChild.cpp:544
(Diff revision 4)
>    return dispatcher.ReturnValue();
>  }
>  
>  int
> +CamerasChild::FocusOnSelectedSource(CaptureEngine aCapEngine,
> +                                    const int capture_id)

This file apparently has a mix of styles. New code should still follow the mozilla coding style I think.

s/capture_id/aCaptureId/

::: dom/media/systemservices/CamerasParent.cpp:986
(Diff revision 4)
> +CamerasParent::RecvFocusOnSelectedSource(const CaptureEngine& aCapEngine,
> +                                         const int& capnum)
> +{
> +  RefPtr<CamerasParent> self(this);
> +  RefPtr<Runnable> webrtc_runnable =
> +    media::NewRunnableFrom([self, aCapEngine, capnum]() -> nsresult {

You can now also inline `self = RefPtr<CamerasParent>(this)` if you want.

::: dom/media/systemservices/CamerasParent.cpp:987
(Diff revision 4)
> +                                         const int& capnum)
> +{
> +  RefPtr<CamerasParent> self(this);
> +  RefPtr<Runnable> webrtc_runnable =
> +    media::NewRunnableFrom([self, aCapEngine, capnum]() -> nsresult {
> +      LOG((__PRETTY_FUNCTION__));

Put this log outside the runnable, at the top of RecvFocusOnSelectedSource.

::: dom/media/systemservices/CamerasParent.cpp:991
(Diff revision 4)
> +    media::NewRunnableFrom([self, aCapEngine, capnum]() -> nsresult {
> +      LOG((__PRETTY_FUNCTION__));
> +      if (auto engine = self->EnsureInitialized(aCapEngine)) {
> +        engine->WithEntry(capnum,[](VideoEngine::CaptureEntry& cap){
> +          if (cap.VideoCapture()) {
> +            cap.VideoCapture()->FocusOnSelectedSource();

I think it makes sense to forward whether this succeeded or not to the caller. It may not be needed now but in the future -- who knows.

There appears to be support for this in both the backends and at the callsite.

::: dom/media/webrtc/MediaEngineDefault.cpp:236
(Diff revision 4)
> +MediaEngineDefaultVideoSource::FocusOnSelectedSource(const RefPtr<const AllocationHandle>& aHandle)
> +{
> +  AssertIsOnOwningThread();
> +  return NS_OK;
> +}

This could be a default implementation in MediaEngineSource, but returning NS_ERROR_NOT_IMPLEMENTED.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:192
(Diff revision 4)
>  
>    int mCaptureIndex;
>    const dom::MediaSourceEnum mMediaSource; // source of media (camera | application | screen)
>    const camera::CaptureEngine mCapEngine;
>    const bool mScary;
> +  bool mIsChrome;

This shouldn't be needed anymore.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:351
(Diff revision 4)
> +  int result;
> +  result  = camera::GetChildAndCall(&camera::CamerasChild::FocusOnSelectedSource,
> +                              mCapEngine, mCaptureIndex);
> +  MOZ_ASSERT(result == 0, "FocusOnSelectedSource should not fail");

There's no guarantee this succeeds as IPC is involved.

::: dom/media/webrtc/MediaEngineSource.h:155
(Diff revision 4)
>     * will be started.
>     */
>    virtual nsresult Start(const RefPtr<const AllocationHandle>& aHandle) = 0;
>  
>    /**
> +   * This brings focus to the selected source, e.g. to bring a shared window

s/shared/captured/

::: dom/media/webrtc/MediaEngineSource.h:155
(Diff revision 4)
> +   * This brings focus to the selected source, e.g. to bring a shared window
> +   * to the front.
> +   */

Please mention what the return values mean.

IMO these make sense:
NS_OK for success
NS_ERROR_NOT_AVAILABLE for backends where it doesn't make sense, including video backends capturing cameras
NS_ERROR_NOT_IMPLEMENTED for backends where it makes sense but there's no implementation, like tab capture
NS_ERROR_UNEXPECTED for ipc errors, if we can distinguish them
NS_ERROR_FAILURE for reported failures from underlying code

::: dom/media/webrtc/MediaEngineTabVideoSource.cpp:403
(Diff revision 4)
>  }
>  
>  nsresult
> +MediaEngineTabVideoSource::FocusOnSelectedSource(const RefPtr<const AllocationHandle>& aHandle)
> +{
> +  return NS_OK;

Please fail an assert here since this is not implemented. After all this is a backend where focusing would make sense to implement.

The decision on whether to use it or not for a specific form of capture should be taken in MediaManager, so that a caller can expect the method to do what is advertised.

Comment 18

[Dan Minor [:dminor]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243360/diff/4-5/

Comment 19

[Dan Minor [:dminor]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

https://reviewboard.mozilla.org/r/243360/#review251342

> Please fail an assert here since this is not implemented. After all this is a backend where focusing would make sense to implement.
> 
> The decision on whether to use it or not for a specific form of capture should be taken in MediaManager, so that a caller can expect the method to do what is advertised.

This is reachable from the unit tests, so rather than an assert I'll return NS_ERROR_NOT_IMPLEMENTED.

Comment 20

[Dan Minor [:dminor]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243360/diff/5-6/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c13ca75c9bcb
Should bring window to front when screen-sharing a window; r=pehrsons

Comment 22

[Dorel Luca [:dluca]]

Backed out changeset c13ca75c9bcb (bug 1450658) for Browser-Chrome failures on browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js 

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=179822542&repo=autoland&lineNumber=3637

343 INFO getUserMedia application only
13:46:57     INFO -  344 INFO requesting devices
13:46:57     INFO -  Buffered messages finished
13:46:57    ERROR -  345 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js | Test timed out -
13:46:57     INFO -  GECKO(3828) | MEMORY STAT | vsize 734MB | vsizeMaxContiguous 710MB | residentFast 219MB | heapAllocated 78MB
13:46:57     INFO -  346 INFO TEST-OK | browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js | took 90071ms
13:46:57     INFO -  Not taking screenshot here: see the one that was previously logged
13:46:57    ERROR -  347 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js | Found a tab after previous test timed out: https://example.com/browser/browser/base/content/test/webrtc/get_user_media.html -
13:46:57     INFO -  GECKO(3828) | ++DOCSHELL 05909800 == 1 [pid = 2240] [id = {aea37ac9-9838-40bb-b268-85b9cbb47625}]
13:46:57     INFO -  GECKO(3828) | ++DOMWINDOW == 1 (059942E0) [pid = 2240] [serial = 22] [outer = 00000000]
13:46:57     INFO -  348 INFO checking window state
13:46:57     INFO -  GECKO(3828) | must wait for focus
13:46:57     INFO -  GECKO(3828) | ++DOMWINDOW == 2 (05910400) [pid = 2240] [serial = 23] [outer = 059942E0]
13:46:57     INFO -  349 INFO TEST-START | browser/base/content/test/webrtc/browser_devices_get_user_media_tear_off_tab.js
13:46:57     INFO -  GECKO(3828) | ++DOCSHELL 0540F000 == 2 [pid = 2344] [id = {fb210fc9-4605-4ef8-8cbc-f6b73a529ce3}]
13:46:57     INFO -  GECKO(3828) | ++DOMWINDOW == 3 (054882E0) [pid = 2344] [serial = 8] [outer = 00000000]
13:46:57     INFO -  GECKO(3828) | ++DOCSHELL 089CF000 == 3 [pid = 2344] [id = {e1f345c1-c37d-4a26-a9ec-e0f876f99919}]
13:46:57     INFO -  GECKO(3828) | ++DOMWINDOW == 4 (05488670) [pid = 2344] [serial = 9] [outer = 00000000]
13:46:57     INFO -  GECKO(3828) | ++DOMWINDOW == 5 (0A958C00) [pid = 2344] [serial = 10] [outer = 054882E0]
13:46:57     INFO -  GECKO(3828) | ++DOMWINDOW == 6 (0A961400) [pid = 2344] [serial = 11] [outer = 05488670]
13:46:57     INFO -  GECKO(3828) | ++DOMWINDOW == 7 (0A965C00) [pid = 2344] [serial = 12] [outer = 054882E0]
13:46:57     INFO -  GECKO(3828) | ++DOMWINDOW == 8 (0540F800) [pid = 2344] [serial = 13] [outer = 05488670]
13:46:57     INFO -  GECKO(3828) | TEST DEVICES: No test devices found (in media.{audio,video}_loopback_dev, using fake streams.
13:46:58     INFO -  Not taking screenshot here: see the one that was previously logged
13:46:58     INFO -  Buffered messages logged at 13:46:57
13:46:58     INFO -  350 INFO Entering test bound test
13:46:58     INFO -  351 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_tear_off_tab.js | should start the test without any prior popup notification -
13:46:58     INFO -  352 INFO TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_tear_off_tab.js | should start the test with the control center hidden -
13:46:58     INFO -  353 INFO getUserMedia: tearing-off a tab keeps sharing indicators
13:46:58     INFO -  354 INFO requesting devices
13:46:58     INFO -  Buffered messages finished

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c13ca75c9bcb320ec2024fdead70e8e83a67bc18

Backout:
https://hg.mozilla.org/integration/autoland/rev/2f4ca72455da69f532bdbc3355463c086e94ad95

Comment 23

[Dan Minor [:dminor]]

I think the problem is the browser losing focus after it shares a window and that is causing something to timeout. This reproduces locally if I don't restore focus to the browser during the test run after a window is shared. I was concerned about this for the mochitests, but they ended up being fine on try. I didn't realize we had browser chrome tests for window sharing as well :/

Comment 24

[Dan Minor [:dminor]]

Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1989a9ab8e81d41b017997780d8993f9ef6e7d0e with a simple solution, I just moved the screen share test to run last, so losing focus at the end of the test does not cause problems for the remaining tests.

Another possibility would be to detect tests are running and not focus the window in that case. This would probably be useful when running the tests locally, but it means no test coverage of the window focusing code.

Another possibility would be to try to focus Firefox when a window share ends. I'm not sure how great that is from a UI perspective. I guess most of the time people would have been interacting with Firefox when the share ends, so it's probably not going to be a noticeable difference.

Andreas, any opinions on this? Thanks!

Comment 25

[Andreas Pehrson [:pehrsons]]

(In reply to Dan Minor [:dminor] from comment #24)
> Here's a try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=1989a9ab8e81d41b017997780d8993f9ef6e7d0e with a
> simple solution, I just moved the screen share test to run last, so losing
> focus at the end of the test does not cause problems for the remaining tests.

That could be annoying if we ever wanted to add window capture to another browser chrome test.
I assume mochitests are not affected because there are no other windows actually present?


> Another possibility would be to detect tests are running and not focus the
> window in that case. This would probably be useful when running the tests
> locally, but it means no test coverage of the window focusing code.

That would work. Perhaps add a pref to make this behavior configurable, and disable it in browser chrome tests?


> Another possibility would be to try to focus Firefox when a window share
> ends. I'm not sure how great that is from a UI perspective. I guess most of
> the time people would have been interacting with Firefox when the share
> ends, so it's probably not going to be a noticeable difference.

I see two cases of a window capture ending:
- Application ended it. If the user had focus on another window it would be annoying if focus was stolen.
- User interaction ended it. Firefox was already focused so noop.

I think we should avoid setting focus on ending the capture.


Regardless of approach taken, please make sure a test case is added to the manual QA test suite, as we cannot properly test that the right window is focused with our automation test frameworks AIUI.

Comment 26

[Dan Minor [:dminor]]

Attachment: Bug 1450658 - Add preference to disable focusing source;

Focusing a source during a test run can cause Firefox to lose focus and
subsequent tests to timeout. This adds a preference which defaults to true
that allows the source to be focused. It is then set to false for tests that
involve screen sharing.

Review commit: https://reviewboard.mozilla.org/r/247884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/247884/

Comment 27

[Dan Minor [:dminor]]

Comment on attachment 8974972 [details]
Bug 1450658 - Should bring window to front when screen-sharing a window;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243360/diff/6-7/

Comment 28

[Dan Minor [:dminor]]

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5886370b0f8283837ee3ccda109f0396ae2ee59

Please let me know if there is a better place to read the pref.

Comment 29

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8981836 [details]
Bug 1450658 - Add preference to disable focusing source;

https://reviewboard.mozilla.org/r/247884/#review254046

::: browser/base/content/test/webrtc/browser.ini:2
(Diff revision 1)
>  [DEFAULT]
> +prefs = media.webrtc.testing.focus_source=false

Browser chrome webrtc tests seem to set prefs at [1]. Should we do the same? Might want to ask florian.

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/base/content/test/webrtc/head.js#4

::: dom/media/MediaManager.cpp:2899
(Diff revision 1)
>            }
>          }
>        }
>  
> +      bool focusSource;
> +      focusSource = mozilla::Preferences::GetBool("media.webrtc.testing.focus_source", true);

Can the name mention getusermedia instead of webrtc?

To follow the pattern of this camera pref: [1]

Perhaps "media.getusermedia.window.focus_source.enabled"?


[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/modules/libpref/init/all.js#446

::: dom/media/tests/mochitest/mochitest.ini:2
(Diff revision 1)
>  [DEFAULT]
> +prefs = media.webrtc.testing.focus_source=false

Let's put this where we have the other prefs, [1].

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/dom/media/tests/mochitest/head.js#408-426

Comment 30

[Dan Minor [:dminor]]

Comment on attachment 8981836 [details]
Bug 1450658 - Add preference to disable focusing source;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247884/diff/1-2/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/182095efdd8e
Should bring window to front when screen-sharing a window; r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/d34ba6416eaa
Add preference to disable focusing source; r=pehrsons

Comment 32

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/182095efdd8e
https://hg.mozilla.org/mozilla-central/rev/d34ba6416eaa

Comment 33

[Dan Minor [:dminor]]

I've submitted a PI request for manual testing for this for when they test webrtc in the future.

Comment 34

[Stefan [:StefanG_QA]]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0 (20180404224504)

I was able to reproduce this issue using the above old Nightly build. When navigating to https://mozilla.github.io/webrtc-landing/gum_test.html and press the Window button to share a window, the selected window is still minimized.


Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0 (20180615100050)

This issue has been verified using the latest Nightly build on Windows 10 x64, Windows 7 x86, Ubuntu 18.04 and OS X 10.13. The issue is no more reproducible. When navigating to https://mozilla.github.io/webrtc-landing/gum_test.html and press the Window button to share a window, the selected application is brought to focus.