Closed Bug 1059220 Opened 6 years ago Closed 6 years ago

display window count of each application in applications list of the app share prompt

Categories

(Firefox :: General, defect)

defect
Not set
Points:
1

Tracking

()

VERIFIED FIXED
Firefox 34
Iteration:
34.3

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file)

The mock-up in attachment 8475862 [details] from bug 1053221 showed the window count for each application.

This wasn't implemented as part of bug 1057006 because it required additional platform support, which is being added in bug 1058766.
Flags: qe-verify+
Flags: firefox-backlog+
Depends on: 1058766
Attached patch PatchSplinter Review
Attachment #8479809 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8479809 [details] [diff] [review]
Patch

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

Curiosity question: what is shared when sharing an app with 0 windows on OS X?

Are we happy just showing this in the prompt and not in other places? :-)

::: browser/modules/webrtcUI.jsm
@@ +360,5 @@
>              name = devices[i].name;
> +            if (type == "application") {
> +              // The application names returned by the platform are of the form:
> +              // <window count>\x1e<application name>
> +              let sepIndex = name.indexOf("\x1e");

Holy hackman batman. Why not just create a separate field on the device object? :-\
Attachment #8479809 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #2)

> Curiosity question: what is shared when sharing an app with 0 windows on OS
> X?

I wondered the same thing. We share a completely blank stream, and once a window of the application is displayed, it appears correctly.

> Are we happy just showing this in the prompt and not in other places? :-)

Not really, but we aren't more unhappy about this than about not being able to tell in the other indicators what window/application is being shared. I'm hoping all of this will be fixed correctly by the redesign for 35.

> ::: browser/modules/webrtcUI.jsm
> @@ +360,5 @@
> >              name = devices[i].name;
> > +            if (type == "application") {
> > +              // The application names returned by the platform are of the form:
> > +              // <window count>\x1e<application name>
> > +              let sepIndex = name.indexOf("\x1e");
> 
> Holy hackman batman. Why not just create a separate field on the device
> object? :-\

Short answer is that the webrtc folks didn't want to change the device interface for 34.

Longer answer can be found in #media IRC logs from last Thursday I think:
22:02:26 - jesup: linuxwolf: I presume adding a number of windows to the descriptions when getting the list wouldn't be too hard?
22:02:33 - linuxwolf: hrm
22:02:58 - flo-retina: jesup: note the plural string.
22:03:10 - linuxwolf: I don't think so, but it'll take some doing for localization down to that layer of the code
22:04:09 - linuxwolf: or tweaking of a number of interfaces to bubble it up separately
22:04:09 - flo-retina: maybe we could find a way to expose the screen count to the UI, and have the UI deal with the localized plural string?
22:05:14 - linuxwolf: if it's ok for the display name to have semantic meaning (e.g., <window count> <app name>"), then it's very doable
22:05:41 - linuxwolf: (or whatever the property is called at the UI layer)
22:06:55 - flo-retina: hmm, that's a bit strange, but seems like an easy solution :).
22:07:20 - linuxwolf: certainly easier than changing a bunch of interfaces and their impls (-:
22:08:21 - jesup: flo-retina: I'm good with linuxwolfs, or just application names,  No major reworks please (and not localization nightmares ;-)
22:10:04 - flo-retina: jesup: alright, let's parse the display name in the UI then :)
Iteration: --- → 34.3
Florin, we don't have a feature owner for app sharing yet. Is this something your team can take on ? I will bring it up with Marc as well. Thanks!
QA Whiteboard: appsharing
Flags: needinfo?(florin.mezei)
QA Contact: florin.mezei
https://hg.mozilla.org/mozilla-central/rev/226c2201e285
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
(In reply to Liz Henry :lizzard from comment #5)
> Florin, we don't have a feature owner for app sharing yet. Is this something
> your team can take on ? I will bring it up with Marc as well. Thanks!

Appsharing is pretty much another form of screensharing, so I'm guessing this feature should go to Nils. Liz, can you talk to Nils and see if he should take these app sharing bugs and send me an email? I've had a look today over app sharing, so if needed I can continue working on bugs related to it.

With regards to this bug, I've verified it today on the latest Firefox Nightly 34 (BuildID=20140828030205) on Windows 7 x64, Mac OS X 10.8.5 and Ubuntu 13.04 x64, and I can see the window count displayed for each item in the application list, whether it's 1, 2 or more windows.

I see some issues with the actual counting (e.g. count does not update unless Firefox is restarted, counting of some applications does not look right on some OS), but these seem to belong under bug 1058766.

The steps used to test this were:
- had multiple applications with multiple windows open
- 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
- clicked on the "Application to share:" drop-down to get the list of applications

Let me know if you think anything more should be done for this.
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei) → needinfo?(lhenry)
QA Contact: florin.mezei → drno
Clearing needinfo a bit late.... thanks Florin!
Flags: needinfo?(lhenry)
You need to log in before you can comment on or make changes to this bug.