1275262 - implement device sharing animation on background tabs

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect, P1)

Description

[Florian Quèze [:florian]]

This is the implementation bug for the last point of the user story in bug 1214334:

* If the user leaves the tab, the tab's fav icon gets replaced with the red permission icon (camera/mic) and animates between the fav icon and the red icon as long as the camera/mic session is open. 

See bug 1214334 comment 1 for the proposed UX.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch v1

The patch already works, but I would like too look at it again before requesting review. I think some of it would deserve a test. Also, assuming bug 1274480 lands soon, I should probably wait for it.

Comment 2

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=afb1596362e9

Comment 3

[Florian Quèze [:florian]]

Attachment: implement device sharing animation on background tabs

Only requesting feedback for now as I know the way the icons are included will change once bug 1274480 lands, but this should otherwise be ready for review.

Comment 4

[:Paolo Amadini]

Comment on attachment 8761296 [details] [diff] [review]
implement device sharing animation on background tabs

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

Just a quick feedback for now, I haven't looked at the code in detail.

You can now try to use the glyphs and filters from the SVG icons bug. In the CSS, ensure the filter is not applied unless the tab is in "sharing" state, to avoid performance issues.

::: browser/base/content/tabbrowser.xml
@@ +1368,5 @@
>        </method>
>  
> +      <method name="setBrowserSharing">
> +        <parameter name="aBrowser"/>
> +        <parameter name="aSharingState"/>

The tabbrowser.xml changes should be reviewed by someone more familiar with it.

::: browser/base/content/test/general/browser_devices_get_user_media_anim.js
@@ +99,5 @@
> +
> +      for (let test of gTests) {
> +        info(test.desc);
> +        yield test.run();
> +      }

Why the custom test runner? Could easily use add_task.

::: browser/modules/webrtcUI.jsm
@@ +879,5 @@
> +  let tabbrowser = chromeWin.gBrowser;
> +  if (tabbrowser) {
> +    let sharing;
> +    if (aState.screen)
> +      sharing = "screen";

Add curly braces around single lines. It's going to be the new style everywhere, but more importantly it's also the style used just a few lines below.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v3

(In reply to :Paolo Amadini from comment #4)

> ::: browser/base/content/test/general/browser_devices_get_user_media_anim.js
> @@ +99,5 @@
> > +
> > +      for (let test of gTests) {
> > +        info(test.desc);
> > +        yield test.run();
> > +      }
>
> Why the custom test runner? Could easily use add_task.

I copied the structure of the browser_devices_get_user_media.js test, and in that one there's a |yield expectNoObserverCalled();| line that runs between each test. I don't care much about testing notifications in the test about animations, but having all the related tests sharing the same structure makes it easier (I think) to copy/paste things from one test to the other.

> ::: browser/modules/webrtcUI.jsm
> @@ +879,5 @@
> > +  let tabbrowser = chromeWin.gBrowser;
> > +  if (tabbrowser) {
> > +    let sharing;
> > +    if (aState.screen)
> > +      sharing = "screen";
>
> Add curly braces around single lines. It's going to be the new style
> everywhere, but more importantly it's also the style used just a few lines
> below.

The style used a few lines below, but not a few lines above. Oh well... (I did the requested change).

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8762059 [details] [diff] [review]
Patch v3

Paolo asked for someone familiar with tabbrowser.xml to look at this, I'm confident Gijs is :-).

Comment 7

[:Gijs (he/him)]

Comment on attachment 8762059 [details] [diff] [review]
Patch v3

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

(I'm very sorry, I won't be able to get to this review until Monday.)

Comment 8

[:Gijs (he/him)]

Comment on attachment 8762059 [details] [diff] [review]
Patch v3

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

r- because of the copy-paste confusion (why didn't that trip any tests) and the fact that:
1) I see the icon initially just fading to blank and back on my machine when I just set the sharing attribute to "camera" (tested on fx-team from earlier this morning) - after a while it starts working. Not sure what triggers that.
2) When setting the attribute on a tab, the tab text noticeably shifts left

Both tests on OSX. I haven't reviewed the test yet.

::: browser/base/content/tabbrowser.css
@@ +19,5 @@
>  }
>  
>  .tab-close-button[pinned],
>  .tabbrowser-tabs[closebuttons="activetab"] > * > * > * > .tab-close-button:not([visuallyselected="true"]),
> +.tab-icon-image:not([src]):not([pinned]):not([crashed]):-moz-any([selected],:not([sharing])),

Nit: I'd sooner just split the rule:

.tab-icon-image:not([src]):not([pinned]):not([crashed]):not([sharing]),
.tab-icon-image:not([src]):not([pinned]):not([crashed])[selected],

::: browser/base/content/tabbrowser.xml
@@ +2639,5 @@
>                modifiedAttrs.push("usercontextid");
>              }
> +            if (aOtherTab.hasAttribute("soundplaying")) {
> +              aOurTab.setAttribute("soundplaying", aOtherTab.getAttribute("soundplaying"));
> +              modifiedAttrs.push("soundplaying");

Copy-paste error?

::: browser/themes/shared/tabs.inc.css
@@ +127,5 @@
> +}
> +
> +.tab-sharing-icon-overlay[sharing] {
> +  filter: url("chrome://browser/skin/filters.svg#fill");
> +  fill: rgb(224, 41, 29);

This feels like it won't work well for lightweight themes... Is there a particular reason to diverge from the standard dark-on-light/light-on-dark styling we've used everywhere else?

Comment 9

[Florian Quèze [:florian]]

Attachment: implement device sharing animation on background tabs,

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

> r- because of the copy-paste confusion (why didn't that trip any tests) and
> the fact that:
> 1) I see the icon initially just fading to blank and back on my machine when
> I just set the sharing attribute to "camera" (tested on fx-team from earlier
> this morning) - after a while it starts working. Not sure what triggers that.

I can't reproduce on my current fx-team build from last Friday. Will file a platform bug if I see this after updating.

> 2) When setting the attribute on a tab, the tab text noticeably shifts left

Fixed, a 6px margin-end was missing on the next <xul:image>.

> > +.tab-icon-image:not([src]):not([pinned]):not([crashed]):-moz-any([selected],:not([sharing])),
>
> Nit: I'd sooner just split the rule

Done.

> ::: browser/base/content/tabbrowser.xml
> @@ +2639,5 @@
> >                modifiedAttrs.push("usercontextid");
> >              }
> > +            if (aOtherTab.hasAttribute("soundplaying")) {
> > +              aOurTab.setAttribute("soundplaying", aOtherTab.getAttribute("soundplaying"));
> > +              modifiedAttrs.push("soundplaying");
>
> Copy-paste error?

Messed up unbitrotting, sorry about that :-/.

> ::: browser/themes/shared/tabs.inc.css
> @@ +127,5 @@
> > +}
> > +
> > +.tab-sharing-icon-overlay[sharing] {
> > +  filter: url("chrome://browser/skin/filters.svg#fill");
> > +  fill: rgb(224, 41, 29);
>
> This feels like it won't work well for lightweight themes... Is there a
> particular reason to diverge from the standard dark-on-light/light-on-dark
> styling we've used everywhere else?

Red is pretty standard for recording indicators (eg. on real camera). I'll bring this concern to UX, and file a follow-up if we want to change the color for dark themes.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8762651 [details] [diff] [review]
implement device sharing animation on background tabs,

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

::: browser/base/content/test/general/browser.ini
@@ +299,5 @@
>  [browser_devices_get_user_media.js]
>  skip-if = buildapp == 'mulet' || (os == "linux" && debug) # linux: bug 976544
>  [browser_devices_get_user_media_about_urls.js]
>  skip-if = e10s && debug
> +[browser_devices_get_user_media_anim.js]

Can we move all the browser_devices* tests into a sibling directory to browser/base/content/test/general, like browser/base/content/test/webrtc/ or something?

::: browser/base/content/test/general/browser_devices_get_user_media_anim.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

No licenses in test files (or if anything, public domain).

@@ +16,5 @@
> +
> +{
> +  desc: "device sharing animation on background tabs",
> +  run: function checkAudioVideo() {
> +    function *getStreamAndCheckBackgroundAnim(aAudio, aVideo, aSharing) {

Nit: function* getStreamAndCheckBackgroundAnim

I'm kind of surprised this version doesn't throw a syntax error.

@@ +18,5 @@
> +  desc: "device sharing animation on background tabs",
> +  run: function checkAudioVideo() {
> +    function *getStreamAndCheckBackgroundAnim(aAudio, aVideo, aSharing) {
> +      // Get a stream
> +      let promise = promisePopupNotificationShown("webRTC-shareDevices");

Nit: name this (e.g. popupPromise) instead of just 'promise'.

@@ +53,5 @@
> +      is(gBrowser.selectedTab.getAttribute("sharing"), "",
> +         "the new tab doesn't have the 'sharing' attribute");
> +      is(tab.getAttribute("sharing"), aSharing,
> +         "the tab still has the 'sharing' attribute");
> +      is(window.getComputedStyle(icon).display, "-moz-box",

I would prefer isnot(..., "none", ...) to ensure that if we ever use some other non-none display style the test won't fail.

@@ +70,5 @@
> +    }
> +
> +    yield getStreamAndCheckBackgroundAnim(true, true, "camera");
> +    yield getStreamAndCheckBackgroundAnim(false, true, "camera");
> +    yield getStreamAndCheckBackgroundAnim(true, false, "microphone");

Can't we just define the expected sharing value as:

let sharingValue = aVideo ? "camera" : "microphone";

instead of duplicating that information in the arguments?

Also, is there a particular reason that we don't test the screen case?

Comment 11

[Florian Quèze [:florian]]

Attachment: Patch v5

(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)

> > 1) I see the icon initially just fading to blank and back on my machine when
> > I just set the sharing attribute to "camera" (tested on fx-team from earlier
> > this morning) - after a while it starts working. Not sure what triggers that.
>
> I can't reproduce on my current fx-team build from last Friday. Will file a
> platform bug if I see this after updating.

Unfortunately, I can now reproduce on an updated fx-team build. Filed bug 1281428.


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

> Can we move all the browser_devices* tests into a sibling directory to
> browser/base/content/test/general, like browser/base/content/test/webrtc/ or
> something?

This is something I would like to do, but I would prefer doing it in a separate bug as there are other related cleanups to these tests that I would like to do. I filed bug 1281427 and assigned it to me.

> @@ +70,5 @@
> > +    }
> > +
> > +    yield getStreamAndCheckBackgroundAnim(true, true, "camera");
> > +    yield getStreamAndCheckBackgroundAnim(false, true, "camera");
> > +    yield getStreamAndCheckBackgroundAnim(true, false, "microphone");
>
> Can't we just define the expected sharing value as:
>
> let sharingValue = aVideo ? "camera" : "microphone";
>
> instead of duplicating that information in the arguments?

I find it clearer to have the test stating explicitly the expected result.

> Also, is there a particular reason that we don't test the screen case?

Testing the screen sharing case is slightly more difficult, because we can't just accept the "default device" (it's "none"), and there may be platform variations, or variations between test slaves in what's exposed. This is absolutely not a good reason to not have test coverage though. The real reason for the complete lack of screen sharing test coverage is that we initially landed the screensharing UI under time pressure because it was urgently needed in a specific release by a partner.

Redesigning the screen sharing permission prompt so that we can remove the screensharing whitelist is one of my goals for the next few months, so expect tests to come. And this makes moving the tests to their own subfolder even more attractive ;-).

Comment 12

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/e077a9ed54589316850312a6c4460ef15472109d
Bug 1275262 - implement device sharing animation on background tabs, r=Gijs.

Comment 13

[Carsten Book [:Tomcat]]

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