1057006 - Implement UI for app sharing

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Florian Quèze [:florian]]

The patch in bug 1036653 does the strict minimum in browser/ for app sharing to work, but we need to implement the UX designed in bug 1053221.

Comment 1

[Florian Quèze [:florian]]

Attachment: WIP1

This WIP adds all strings, and implements the changes to the doorhanger requesting permissions.

Changes to the various sharing indicators we have require a change to nsIMediaManagerService.mediaCaptureWindowState; jesup is on it.

Comment 2

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

Attachment: split window and app sharing reports so the UI can handle them separately

Comment 3

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

Attachment: split window and app sharing reports so the UI can handle them separately

Comment 4

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

Attachment: split window and app sharing reports so the UI can handle them separately

Comment 5

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

Attachment: split window and app sharing reports so the UI can handle them separately

Comment 6

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 7

[:Gijs (he/him)]

Comment on attachment 8479092 [details] [diff] [review]
Patch v2

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

Code-wise this seems OK, but see my question below.

::: browser/modules/webrtcUI.jsm
@@ +708,3 @@
>        webrtcUI.showScreenSharingIndicator = "Window";
> +    else if (app.value && !webrtcUI.showScreenSharingIndicator)
> +      webrtcUI.showScreenSharingIndicator = "Application";

This is odd. Isn't sharing an entire application more important than sharing a window?

Comment 8

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #7)

> ::: browser/modules/webrtcUI.jsm
> @@ +708,3 @@
> >        webrtcUI.showScreenSharingIndicator = "Window";
> > +    else if (app.value && !webrtcUI.showScreenSharingIndicator)
> > +      webrtcUI.showScreenSharingIndicator = "Application";
> 
> This is odd. Isn't sharing an entire application more important than sharing
> a window?

I did this because showing "Applications" (plural) when I was sharing one application and one window with 2 different tabs felt wrong when I tested it. See bug 1053221 comment 7 for the discussion I had with phlsa on this topic.

Comment 9

[:Gijs (he/him)]

With the following STR, I can reproducibly crash with these patches + bug 1036653's applied:

1) open nightly on a clean profile
2) open http://queze.net/goinfre/ff_gum_test.html
3) share app #1 (let's say terminal.app)
4) open http://queze.net/goinfre/ff_gum_test.html in a new tab, while keeping the other one open
5) share app #2 (let's say thunderbird.app)

as soon as I click "Share Selected Application" in the doorhanger, things go pear-shaped:


Stack trace:

#0	0x00007fff8b4c8732 in strlen ()
#1	0x00000001029f5bc1 in webrtc::ViEInputManager::CreateCaptureDevice(char const*, unsigned int, int&) at /Users/gkruitbosch/dev/fx-team/media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc:190
#2	0x00000001029e1334 in webrtc::ViECaptureImpl::AllocateCaptureDevice(char const*, unsigned int, int&) at /Users/gkruitbosch/dev/fx-team/media/webrtc/trunk/webrtc/video_engine/vie_capture_impl.cc:101
#3	0x0000000102545a3e in mozilla::MediaEngineWebRTCVideoSource::Allocate(mozilla::VideoTrackConstraintsN const&, mozilla::MediaEnginePrefs const&) at /Users/gkruitbosch/dev/fx-team/content/media/webrtc/MediaEngineWebRTCVideo.cpp:453
#4	0x0000000102090dae in mozilla::GetUserMediaRunnable::ProcessGetUserMedia(mozilla::MediaEngineAudioSource*, mozilla::MediaEngineVideoSource*) at /Users/gkruitbosch/dev/fx-team/dom/media/MediaManager.cpp:1175
#5	0x00000001020905f7 in mozilla::GetUserMediaRunnable::Run() at /Users/gkruitbosch/dev/fx-team/dom/media/MediaManager.cpp:1056
#6	0x00000001010807c4 in nsThread::ProcessNextEvent(bool, bool*) at /Users/gkruitbosch/dev/fx-team/xpcom/threads/nsThread.cpp:769
#7	0x000000010109df13 in NS_ProcessNextEvent(nsIThread*, bool) at /Users/gkruitbosch/dev/fx-team/xpcom/glue/nsThreadUtils.cpp:265
#8	0x00000001012e78aa in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) at /Users/gkruitbosch/dev/fx-team/ipc/glue/MessagePump.cpp:355
#9	0x00000001012cd0ed in MessageLoop::RunInternal() [inlined] at /Users/gkruitbosch/dev/fx-team/ipc/chromium/src/base/message_loop.cc:229
#10	0x00000001012cd0dd in MessageLoop::RunHandler() [inlined] at /Users/gkruitbosch/dev/fx-team/ipc/chromium/src/base/message_loop.cc:222
#11	0x00000001012cd0dd in MessageLoop::Run() at /Users/gkruitbosch/dev/fx-team/ipc/chromium/src/base/message_loop.cc:196
#12	0x000000010107f678 in nsThread::ThreadFunc(void*) at /Users/gkruitbosch/dev/fx-team/xpcom/threads/nsThread.cpp:345
#13	0x00000001006465b1 in _pt_root at /Users/gkruitbosch/dev/fx-team/nsprpub/pr/src/pthreads/ptthread.c:212
#14	0x00007fff883b2899 in _pthread_body ()
#15	0x00007fff883b272a in _pthread_start ()


From looking in XCode, vie_capture is null and/or returns null for device_name (opt build, hard to be sure).

Comment 10

[:Gijs (he/him)]

Filed bug 1058944 for comment #9.

Comment 11

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

Comment on attachment 8478308 [details] [diff] [review]
split window and app sharing reports so the UI can handle them separately

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

r=me with missing %s added.

::: dom/media/MediaManager.cpp
@@ +2103,3 @@
>  #ifdef DEBUG
>    nsCOMPtr<nsPIDOMWindow> piWin = do_QueryInterface(aWindow);
>    LOG(("%s: window %lld capturing %s %s %s %s", __FUNCTION__, piWin ? piWin->WindowID() : -1,

need to add another %s

Comment 12

[Florian Quèze [:florian]]

Attachment: Patch v2 (fixed bitrot)

Fixed minor bitrot in browser.dtd caused by the landing of bug 1043797.

Carrying forward Gijs's r+.

Comment 13

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af3510fa00fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffc10f34de13

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/af3510fa00fa
https://hg.mozilla.org/mozilla-central/rev/ffc10f34de13

Comment 15

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified this on the latest Firefox 34 Nightly (BuildID=20140829030204) on Win 7 x64, Mac OS X 10.9.4 and Ubuntu 13.04 x64, and there are no UI issues specific to this implementation (there are some very small issues related to doorhangers in general).

The steps used to test this were:
- started Firefox Nightly 34
- ensured media.getusermedia.screensharing.enabled = true 
- set media.getusermedia.screensharing.allowed_domains = queze.net
- opened http://queze.net/goinfre/ff_gum_test.html
- started Application
- verified: display and behaviour of the App Sharing doorhanger and its components (before/after starting to share something), display in the Mac top bar during sharing.