Implement UI for app sharing

VERIFIED FIXED in Firefox 34

Status

()

Firefox
General
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 1 bug)

Trunk
Firefox 34
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

3 years ago
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.
Flags: firefox-backlog+
(Assignee)

Updated

3 years ago
Flags: qe-verify+
(Assignee)

Updated

3 years ago
Assignee: nobody → florian

Updated

3 years ago
Status: NEW → ASSIGNED
Iteration: --- → 34.3
(Assignee)

Comment 1

3 years ago
Created attachment 8478279 [details] [diff] [review]
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.
Created attachment 8478282 [details] [diff] [review]
split window and app sharing reports so the UI can handle them separately
Created attachment 8478287 [details] [diff] [review]
split window and app sharing reports so the UI can handle them separately

Updated

3 years ago
Attachment #8478282 - Attachment is obsolete: true
Created attachment 8478288 [details] [diff] [review]
split window and app sharing reports so the UI can handle them separately

Updated

3 years ago
Attachment #8478287 - Attachment is obsolete: true
Created attachment 8478308 [details] [diff] [review]
split window and app sharing reports so the UI can handle them separately

Updated

3 years ago
Attachment #8478288 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
Created attachment 8479092 [details] [diff] [review]
Patch v2
Attachment #8478279 - Attachment is obsolete: true
Attachment #8479092 - Flags: review?(gijskruitbosch+bugs)

Comment 7

3 years ago
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?
Attachment #8479092 - Flags: feedback+
(Assignee)

Comment 8

3 years ago
(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 hidden (obsolete)

Updated

3 years ago
Attachment #8478308 - Flags: review?(jib)

Updated

3 years ago
Depends on: 1058944

Comment 10

3 years ago
Filed bug 1058944 for comment #9.

Updated

3 years ago
Attachment #8479092 - Flags: review?(gijskruitbosch+bugs) → review+
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
Attachment #8478308 - Flags: review?(jib) → review+
(Assignee)

Updated

3 years ago
Depends on: 1059220
(Assignee)

Comment 12

3 years ago
Created attachment 8479817 [details] [diff] [review]
Patch v2 (fixed bitrot)

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

Carrying forward Gijs's r+.
Attachment #8479092 - Attachment is obsolete: true
Attachment #8479817 - Flags: review+
(Assignee)

Updated

3 years ago
Depends on: 1059295
(Assignee)

Comment 13

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af3510fa00fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffc10f34de13
https://hg.mozilla.org/mozilla-central/rev/af3510fa00fa
https://hg.mozilla.org/mozilla-central/rev/ffc10f34de13
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
QA Contact: florin.mezei
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.
Status: RESOLVED → VERIFIED
Depends on: 1132071
You need to log in before you can comment on or make changes to this bug.