Indicator for devices actively streaming or permissions being used

VERIFIED FIXED in Firefox 51

Status

()

Firefox
Device Permissions
P1
normal
VERIFIED FIXED
2 years ago
8 months ago

People

(Reporter: MarcoM, Assigned: florian)

Tracking

(Depends on: 3 bugs, Blocks: 1 bug)

unspecified
Firefox 51
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(6 attachments, 5 obsolete attachments)

5.73 KB, patch
johannh
: review+
Details | Diff | Splinter Review
18.48 KB, patch
johannh
: review+
Details | Diff | Splinter Review
4.98 KB, patch
jaws
: review-
Details | Diff | Splinter Review
15.32 KB, patch
johannh
: review+
Details | Diff | Splinter Review
4.44 KB, patch
jaws
: review+
Details | Diff | Splinter Review
13.55 KB, patch
johannh
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

2 years ago
Priority: P3 → P2
(Reporter)

Updated

2 years ago
Priority: P2 → P3

Comment 1

2 years ago
According to https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/96586951

Does it related to bug 1204007 but is about to show icons at the status bar?
Flags: needinfo?(jmoradi)
(Reporter)

Updated

2 years ago
Priority: P3 → P2

Updated

2 years ago
Blocks: 1214334

Comment 2

2 years ago
Hi paolo, do you have idea about what this issue means and what the actionable task is?
Flags: needinfo?(paolo.mozmail)

Comment 3

2 years ago
I think it's related to the work Florian is doing for the user story in bug 1214334, where we display a colored "in use" icon for camera and microphone. Maybe Florian plans to work on this bug as well.
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(jmoradi)
Flags: needinfo?(florian)
(Assignee)

Comment 4

2 years ago
The mock-up linked in comment 1 is obsolete. https://mozilla.invisionapp.com/share/AF71R266U#/screens/143001431 is a more current mock-up.

The work here is to replace the (i) icon in the location bar with a camera icon, with an animation between the (i) and the camera. And yes, it's definitely related to the work I'm starting in bug 1275262, where I'll be implementing the same animation, but for background tabs.
Flags: needinfo?(florian)
(Assignee)

Comment 5

2 years ago
Hmm, seeing that this blocks the "Site permissions in main panel" user story (bug 1188355), I now think this bug was about the red camera icon in the "Permissions" section of the panel on https://mozilla.invisionapp.com/share/AF71R266U#/screens/143001431

The work I described in comment 4 is likely meant for bug 1206246.
Do we need platform API support (detect if camera/microphone is running) to work on this issue?
Flags: needinfo?(florian)
(Assignee)

Comment 7

a year ago
(In reply to Fred Lin [:gasolin] from comment #6)
> Do we need platform API support (detect if camera/microphone is running) to
> work on this issue?

We already use this information for the current sharing indicators (see code in http://searchfox.org/mozilla-central/source/browser/modules/webrtcUI.jsm)
Flags: needinfo?(florian)
Thanks for the hint, getActiveStreams seems do the job
http://searchfox.org/mozilla-central/source/browser/modules/webrtcUI.jsm#106
Created attachment 8770828 [details] [diff] [review]
0001-Bug-1206233-show-webrtc-in-use-Indicator-in-site-per.patch

Hi florian, 

I've come out a workable patch, it shows in-use state for webrtc icons in site permissions panel when any page has a stream activated. 

I need help about how to get the current browser tab to match with the tab, so only the tab play the active stream will show in-use state icon.
Attachment #8770828 - Flags: feedback?(florian)
(Assignee)

Comment 10

a year ago
Comment on attachment 8770828 [details] [diff] [review]
0001-Bug-1206233-show-webrtc-in-use-Indicator-in-site-per.patch

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

I don't think this approach will get us where we want.

We want a sharing indicator replacing the (i) icon of the control center with an animation. I think this should be done by replacing the implementation of the updateBrowserSpecificIndicator function in browser/modules/webrtcUI.jsm.

Then, when the control center panel opens, it can take information from this indicator, without having to directly touch anything from webrtcUI.jsm.

Unless you really want to work on this bug, I think I should take it, as I'm already familiar with all of the webrtc related pieces, and all the existing sharing indicators need to be modified.
Attachment #8770828 - Flags: feedback?(florian) → feedback-
Iteration: --- → 50.4 - Aug 1
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 11

a year ago
Created attachment 8772907 [details] [diff] [review]
Part 1, animated indicator above the (i) icon
(Assignee)

Comment 12

a year ago
Created attachment 8772908 [details] [diff] [review]
Part 2, show used permissions in the control center panel
(Assignee)

Comment 13

a year ago
Created attachment 8772911 [details] [diff] [review]
Part 3, removed the old doorhangers

This patch set is in working shape, but I'm not requesting review yet because:
- Things that will have to change after bug 1203292 lands are left as placeholders
- This will break existing tests in browser/base/content/test/webrtc/
- This likely needs some additional tests.
- Some of the code removed by this third part will break sharing indicators for SocialAPI/Hello. I need to figure out how much we still care about them.

Notes:

- I'm not sure if handling the "Blocked temporarily" state in the control center should be part of this bug (I think it can be a follow-up; the current patchset doesn't handle it).

- The 'Allowed temporarily' state will likely need some polishing after bug 1203292 lands.

- The new animations added by these patches make bug 1281428 even more visibly annoying.
(Assignee)

Updated

a year ago
Depends on: 1203292, 1281428
(Assignee)

Updated

a year ago
Flags: qe-verify? → qe-verify+
(Assignee)

Updated

a year ago
Component: General → Device Permissions
Thanks for take care of this :)
(Assignee)

Updated

a year ago
Attachment #8770828 - Attachment is obsolete: true
(Assignee)

Comment 15

a year ago
Created attachment 8776940 [details] [diff] [review]
Part 1, animated indicator above the (i) icon
Attachment #8776940 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8772907 - Attachment is obsolete: true
(Assignee)

Comment 16

a year ago
Created attachment 8776941 [details] [diff] [review]
Part 2, show used permissions in the control center panel

I'm not sure here about adding the allowedTemporarily string in sitePermissions.properties, as it doesn't match the other strings there.
Attachment #8776941 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8772908 - Attachment is obsolete: true
(Assignee)

Comment 17

a year ago
Created attachment 8776945 [details] [diff] [review]
Part 3, remove the old doorhangers
Attachment #8776945 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8772911 - Attachment is obsolete: true
(Assignee)

Comment 18

a year ago
Created attachment 8776953 [details] [diff] [review]
Part 4, avoid flickering of the panel when removing permissions

The patch from bug 1280709 introduced a visible flicker when removing a permission item. For one frame, the panel disappears (height of 0px) and only the arrow is still visible. This felt unpolished already, but with the changes I'm making here it becomes possible for this to happen twice in a row (when we stop sharing the camera, the microphone share is also stopped automatically so both rows disappear, each calling setHeightToFit). The panel flickering twice feels really broken to me, so I wrote a hack that avoids this flickering at least for the control center panel.
Attachment #8776953 - Flags: review?(jaws)
Comment on attachment 8776953 [details] [diff] [review]
Part 4, avoid flickering of the panel when removing permissions

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

I actually think we need to back out the setHeightToFit code and figure out the root reason here instead of piling more on top of it. Why can't the panel resize properly when the items are removed?

Using the Browser Toolbox, I opened the bookmarks panel and selected #editBookmarkPanelHeader and set display:none on it, the panel resized properly without any flickering.
Attachment #8776953 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #19)
> Comment on attachment 8776953 [details] [diff] [review]
> Part 4, avoid flickering of the panel when removing permissions
> 
> Review of attachment 8776953 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I actually think we need to back out the setHeightToFit code and figure out
> the root reason here instead of piling more on top of it. Why can't the
> panel resize properly when the items are removed?
> 
> Using the Browser Toolbox, I opened the bookmarks panel and selected
> #editBookmarkPanelHeader and set display:none on it, the panel resized
> properly without any flickering.

You may want to see if the patch I have in bug 1252224 fixes the issue for you.
Blocks: 1286118
(Assignee)

Comment 21

a year ago
Created attachment 8777323 [details] [diff] [review]
Part 5, fix tests
Attachment #8777323 - Flags: review?(jhofmann)
Comment on attachment 8776940 [details] [diff] [review]
Part 1, animated indicator above the (i) icon

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

Looks good, please add the tooltip.

Also, just for the reference, the sharing-icon animation does not work on my machine unless I apply Bug 1287122. We should make sure that one lands soon.

::: browser/base/content/browser.xul
@@ +712,5 @@
>                     ondragstart="gIdentityHandler.onDragStart(event);">
>                  <image id="identity-icon"
>                         consumeanchor="identity-box"
>                         onclick="PageProxyClickHandler(event);"/>
> +                <image id="sharing-icon"/>

This needs a tooltip text. It's probably easiest and least confusing to give this the same tooltip as identity-icon, which is assigned here: http://searchfox.org/mozilla-central/source/browser/base/content/browser.js#6971
Attachment #8776940 - Flags: review?(jhofmann) → review+
Depends on: 1287122
(Assignee)

Comment 23

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f699eab79e69
Comment on attachment 8776945 [details] [diff] [review]
Part 3, remove the old doorhangers

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

Looks good!

::: browser/themes/shared/notification-icons.inc.css
@@ -42,3 @@
>  }
>  
>  #notification-popup-box > .notification-anchor-icon:not(.in-use):hover {

If there's no in-use notification-anchor icon anymore we can remove the :not(.in-use) part of this rule.

@@ -84,1 @@
>  .in-use {

I think this rule is no longer in-use (har, har) and can be removed.
Attachment #8776945 - Flags: review?(jhofmann) → review+
(Assignee)

Comment 25

a year ago
Created attachment 8777777 [details] [diff] [review]
Part 4, avoid flickering of the panel when removing permissions

So I investigated a little bit more why the panel doesn't shrink.
The issues I found:
- flex and setting explicitly the height is a bad mix. The flex attribute has been added in bug 1177161 and I'm relatively confident it's no longer useful, as the "More information" button isn't in the main view.
- the _setMaxHeight method of panelUI.xml setting the height property rather than the maxHeight property seems odd, and it prevents automatic resizing. This code has been added in bug 861703, but the comment thread and the patch size there are too large for me to figure out if there's any chance of this being intentional.

I left the setHeightToFit code in panelUI.xml and the download panel stuff as is, because it's way offtopic for this bug. We should probably reopen bug 1280709 or file a new bug to clean this up.
It's likely that your patch in bug 1252224 would help, as I see it removes the _setMaxHeight code, but I don't think it would be enough without also removing the flex attribute.
Attachment #8777777 - Flags: review?(jaws)
(Assignee)

Comment 26

a year ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)

> You may want to see if the patch I have in bug 1252224 fixes the issue for
> you.

I've just tested your patch, both with and without the flex attribute in panel.inc.xul, it causes the control center panel to shrink too much when removing permissions: http://i.imgur.com/xPHdCHQ.png

Comment 27

a year ago
(In reply to Florian Quèze [:florian] [:flo] from comment #25)
> - flex and setting explicitly the height is a bad mix.

Yeah, that's something Drew mentioned in bug 1280709 comment 11.

> - the _setMaxHeight method of panelUI.xml setting the height property rather
> than the maxHeight property seems odd, and it prevents automatic resizing.

With this change, does the footer in the application menu still move to the bottom when expanding a subview?

Comment 28

a year ago
(In reply to Florian Quèze [:florian] [:flo] from comment #26)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> I've just tested your patch, both with and without the flex attribute in
> panel.inc.xul, it causes the control center panel to shrink too much when
> removing permissions: http://i.imgur.com/xPHdCHQ.png

I know that the height of multi-line description and labels is miscalculated if not set explicitly when calling window.sizeToContent(), it's a long-standing platform bug and it might be playing a role here.
(Assignee)

Comment 29

a year ago
(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f699eab79e69

This shows "browser_devices_get_user_media.js | control center should be open" failures on Windows and Linux, but none on Mac (where I worked on the test changes).
(Assignee)

Comment 30

a year ago
(In reply to :Paolo Amadini from comment #27)

> > - the _setMaxHeight method of panelUI.xml setting the height property rather
> > than the maxHeight property seems odd, and it prevents automatic resizing.
> 
> With this change, does the footer in the application menu still move to the
> bottom when expanding a subview?

No, there's a white area at the bottom of the main view :(.
Comment on attachment 8776941 [details] [diff] [review]
Part 2, show used permissions in the control center panel

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

Looks good, most of these are nits but I'd still like to give it another look :)

::: browser/base/content/browser.js
@@ +7298,5 @@
>  
>      let uri = gBrowser.currentURI;
>  
> +    let permissions = SitePermissions.getPermissionDetailsByURI(uri);
> +    if (this._sharingState) {

This block is sufficiently complex that maybe a short comment about what you're doing and why at the top would help. :)

@@ +7316,5 @@
> +            });
> +            permissions.push({id: id, inUse: true,
> +                              label: SitePermissions.getPermissionLabel(id),
> +                              state: SitePermissions.getDefault(id),
> +                              availableStates: availableStates});

Since this data structure has to change whenever what comes out of SitePermissions.getPermissionDetailsByURI changes and they share more or less the same code, why don't we make a function in SitePermissions that creates permission items for both getPermissionDetailsByURI and this block?

Could have the signature getPermissionItem(id[, state]) where state will default to the SitePermissions.getDefault.

::: browser/locales/en-US/chrome/browser/sitePermissions.properties
@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  allow = Allow
>  allowForSession = Allow for Session
> +allowedTemporarily = Allowed Temporarily

I think this is the correct file for this string, we're just facing the general trouble that we either need two types of strings: one for the action ("Block this permission") and one for the state ("Permission is blocked") OR we just convert this string to "Allow Temporarily".

We should decide on what we want to do. I favor "Allow Temporarily".

::: browser/modules/SitePermissions.jsm
@@ +163,4 @@
>      switch (aState) {
>        case this.UNKNOWN:
> +        if (aInUse)
> +          return gStringBundle.GetStringFromName("allowedTemporarily");

This is ok as WIP, but a future implementation of allowed/blocked temporarily for all permissions (which is specified in the UX spec) will probably have to remove this. Since inUse is associated with webrtc usage we can't use it to signify other temporarily allowed permissions.
Attachment #8776941 - Flags: review?(jhofmann)

Comment 32

a year ago
(In reply to Johann Hofmann [:johannh] from comment #31)
> Since this data structure has to change whenever what comes out of
> SitePermissions.getPermissionDetailsByURI changes and they share more or
> less the same code, why don't we make a function in SitePermissions that
> creates permission items for both getPermissionDetailsByURI and this block?

I agree, and I also think that the module called from here should already merge the data for in-use permissions for the current tab, and gIdentityHandler should be dumber.

In other words, a generic signature in the current module would look like SitePermissions.getPermissionDetailsByURI(uri, tabState) whereas for this specific bug we may even have something simpler like SitePermissions.getPermissionDetailsByURI(uri, inUsePermissionList) where the latter can be an array like ["camera", "microphone"].

An existing thing we can fix is that the function getPermissionDetailsByURI should also output the right "stateLabel" in the permission items. Calling getStateLabel separately in the front-end code didn't fit with the existing pattern anyways, it was done for the dropdown but it's not necessary anymore.

Hope this makes sense, let me know if I have to clarify something.

Comment 33

a year ago
Helpful code constructs:

let foundPermissionOrUndefined = permissions.find(permission => permission.id == id);
let inUsePermissionList = ["camera", "microphone", "screen"].filter(id => this._sharingState[id]);
(In reply to :Paolo Amadini from comment #28)
> (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> > I've just tested your patch, both with and without the flex attribute in
> > panel.inc.xul, it causes the control center panel to shrink too much when
> > removing permissions: http://i.imgur.com/xPHdCHQ.png
> 
> I know that the height of multi-line description and labels is miscalculated
> if not set explicitly when calling window.sizeToContent(), it's a
> long-standing platform bug and it might be playing a role here.

Is this bug on file? Can we get more attention on it instead of trying to code around it?
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8777323 [details] [diff] [review]
Part 5, fix tests

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

Looks good! A few minor things.

::: browser/base/content/test/webrtc/head.js
@@ +331,5 @@
>    });
>  }
>  
> +function* stopSharing(aType = "camera") {
> +  let promise = promiseObserverCalled("recording-device-events");

Nit: Can you give this variable a name that reflects what the promise is waiting for?

@@ +336,5 @@
> +  gIdentityHandler._identityBox.click();
> +  let permissions = document.getElementById("identity-popup-permission-list");
> +  let cancelButton =
> +    permissions.querySelectorAll(".identity-popup-permission-icon." + aType + "-icon ~ " +
> +                                 ".identity-popup-permission-remove-button")[0];

Nit: You should be able to use permissions.querySelector if you're looking for the first match.

@@ +406,5 @@
>  function* checkSharingUI(aExpected) {
> +  // First check the icon above the control center (i) icon.
> +  let identityBox = document.getElementById("identity-box");
> +  ok(identityBox.hasAttribute("sharing"), "sharing attribute is set");
> +  let sharing = identityBox.getAttribute("sharing").split(" ");

You should remove the split here. Otherwise you're comparing an array with a string which seems to mysteriously work through some JS string conversion.

@@ +422,5 @@
> +        return "video";
> +      if (id == "microphone")
> +        return "audio";
> +      return id;
> +    };

Can't we just convert all calls to checkSharingUI to the new format? There are not that many, a quick search reveals 13 calls in two files. Audio and video are not used in the sharingState or the UI, so it would make more sense to me.

@@ +437,5 @@
> +        ok(true, "should not show " + id + " icon in the control center panel");
> +      } else {
> +        // This will happen if there are persistent permissions set.
> +        ok(!icon[0].classList.contains("in-use"),
> +           "if shown, the " + id + " icon should not have the in-use class");

You could add another assertion that icon.length == 1 here.
Attachment #8777323 - Flags: review?(jhofmann) → review+

Updated

a year ago
Depends on: 1293242

Comment 36

a year ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #34)
> (In reply to :Paolo Amadini from comment #28)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #26)
> > > (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #20)
> > > I've just tested your patch, both with and without the flex attribute in
> > > panel.inc.xul, it causes the control center panel to shrink too much when
> > > removing permissions: http://i.imgur.com/xPHdCHQ.png
> > 
> > I know that the height of multi-line description and labels is miscalculated
> > if not set explicitly when calling window.sizeToContent(), it's a
> > long-standing platform bug and it might be playing a role here.
> 
> Is this bug on file? Can we get more attention on it instead of trying to
> code around it?

It turns out that this might be as old as bug 84245, but I've filed the new bug 1293242 so that we have a recent and clear test case in the description of the bug. This bug, triggered by "overflow: hidden", is the one I had previously observed with "sizeToContent", that could be worked around by setting the "height" CSS property on the description element explicitly (d.style.height = window.getComputedStyle(d).height).

I'm not sure if this is exactly the same bug that affects the incorrect shrinking of the panel. It could well be. What is sure is that in at least two other occasions I had to add "display: block" to the styling where we also added "overflow: hidden".
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 37

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=817d6122d06e
(Assignee)

Comment 38

a year ago
Created attachment 8779304 [details] [diff] [review]
Part 2, show used permissions in the control center panel, v3

Updated patch to address comment 31. I haven't implemented the suggestion in comment 32 because I like SitePermissions.jsm to be a wrapper above the permission manager for now, and I think handling of tab-specific permissions is better in browser.js / gIdentityHanlder for now. We may revisit this when implementing a more generic support for temporariy permissions, especially the 'Blocked Temporarily' case (bug 1291642) that should not be limited to WebRTC.
Attachment #8779304 - Flags: review?(jhofmann)
(Assignee)

Updated

a year ago
Attachment #8776941 - Attachment is obsolete: true
(Assignee)

Comment 39

a year ago
(In reply to Johann Hofmann [:johannh] from comment #35)

> @@ +422,5 @@
> > +        return "video";
> > +      if (id == "microphone")
> > +        return "audio";
> > +      return id;
> > +    };
> 
> Can't we just convert all calls to checkSharingUI to the new format? There
> are not that many, a quick search reveals 13 calls in two files. Audio and
> video are not used in the sharingState or the UI, so it would make more
> sense to me.

I haven't implemented this specific suggestion as it's more complicated than just changing all the checkSharingUI callers: checkSharingUI calls assertWebRTCIndicatorStatus which actually uses the "audio" and "video" values to check the attributes set at http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/browser/base/content/webrtcIndicator.js#41
Comment on attachment 8779304 [details] [diff] [review]
Part 2, show used permissions in the control center panel, v3

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

Looks good! I agree that we should defer further improvements on SitePermissions.jsm to another bug.

::: browser/modules/SitePermissions.jsm
@@ +68,5 @@
> +            state: aState, availableStates};
> +  },
> +
> +  /* Returns a list of objects representing all permissions that are currently
> +   * set for the given URI. See getPermissionItem for the content of each object

Tiny nit: please end this sentence with a period. :)
Attachment #8779304 - Flags: review?(jhofmann) → review+
(Assignee)

Comment 41

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7819f6bc4f5d
(Assignee)

Comment 42

a year ago
https://hg.mozilla.org/integration/fx-team/rev/855fe322a553bd7fe067d6e9debeab535fc0d830
Bug 1206233 - Implement the animated sharing indicator above the control center (i) icon, r=johannh.

https://hg.mozilla.org/integration/fx-team/rev/c3796270f3c271122af6540891cdcf9aaaaa1244
Bug 1206233 - Show devices actively streaming in the control center panel, r=johannh.

https://hg.mozilla.org/integration/fx-team/rev/3e0bcd0f52c77b99f9708724880db7d2877be33d
Bug 1206233 - Removed the old device/screen sharing doorhangers, r=johannh.

https://hg.mozilla.org/integration/fx-team/rev/5fd912b9afedd1328764552772485a398aaf9c84
Bug 1206233 - Update webrtc browser-chrome tests to pass without the webRTC-sharingDevices doorhanger, r=johannh.
I had to back these out in https://hg.mozilla.org/integration/fx-team/rev/13abad48dfb6 for xpcshell bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=11067029&repo=fx-team
Flags: needinfo?(florian)
(Assignee)

Comment 44

a year ago
https://hg.mozilla.org/integration/fx-team/rev/576f62171a713f3d4d14c933fe6e837cd0a4ad83
Bug 1206233 - Implement the animated sharing indicator above the control center (i) icon, r=johannh.

https://hg.mozilla.org/integration/fx-team/rev/e3107e2095b8b4527f47706f40a6bdde512fc239
Bug 1206233 - Show devices actively streaming in the control center panel, r=johannh.

https://hg.mozilla.org/integration/fx-team/rev/f2b5ee82a99bd339eac7037ac45f6bb88f625bb5
Bug 1206233 - Removed the old device/screen sharing doorhangers, r=johannh.

https://hg.mozilla.org/integration/fx-team/rev/2f24daceada71e15c03b5f90765264c720ed035a
Bug 1206233 - Update webrtc browser-chrome tests to pass without the webRTC-sharingDevices doorhanger, r=johannh.
(Assignee)

Comment 45

a year ago
(In reply to Wes Kocher (:KWierso) from comment #43)
> I had to back these out in
> https://hg.mozilla.org/integration/fx-team/rev/13abad48dfb6 for xpcshell
> bustage:
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=11067029&repo=fx-team

Fixed by https://hg.mozilla.org/integration/fx-team/diff/e3107e2095b8/browser/modules/test/xpcshell/test_SitePermissions.js
Flags: needinfo?(florian)

Comment 46

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/576f62171a71
https://hg.mozilla.org/mozilla-central/rev/e3107e2095b8
https://hg.mozilla.org/mozilla-central/rev/f2b5ee82a99b
https://hg.mozilla.org/mozilla-central/rev/2f24daceada7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
(Assignee)

Updated

a year ago
Depends on: 1294430
(Assignee)

Updated

a year ago
Depends on: 1294435
(Assignee)

Comment 47

a year ago
Now that this bug is resolved, I think it's better to move the investigations related to the flickering of the panel to another bug, I filed bug 1294435.
Depends on: 1294541
Priority: P2 → P1
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Comment on attachment 8777777 [details] [diff] [review]
Part 4, avoid flickering of the panel when removing permissions

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

Bug 1252224 bounced and I will need to dig deeper in that bug so I won't hold this up any longer. The code changes look fine and I tested out the patch and it worked well for me.
Attachment #8777777 - Flags: review?(jaws) → review+

Comment 49

a year ago
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/dd8193695d2d
Avoid flickering of the panel when removing permission items, r=jaws

Comment 50

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd8193695d2d

Updated

a year ago
Depends on: 1294576

Updated

a year ago
status-firefox50: --- → affected
Since this affects 50, can we please uplift this as long as we don't see any other issues on Nightly? Since Florian is out to August 29, jaws or jhofmann can you coordinate the uplift?
Flags: needinfo?(jhofmann)
Flags: needinfo?(jaws)
I will defer to Johann since he reviewed most of this work.
Flags: needinfo?(jaws)
(Assignee)

Comment 53

a year ago
(In reply to Marcia Knous [:marcia - use ni] from comment #51)
> Since this affects 50, can we please uplift this as long as we don't see any
> other issues on Nightly? Since Florian is out to August 29, jaws or jhofmann
> can you coordinate the uplift?

The change to the status-firefox50 flag seems to be an accident. This is a new feature (which contains new strings), I see no reason to uplift.
status-firefox50: affected → ---
As Florian said, I'm not sure what to uplift here. On the contrary, I think this should get some Nightly baking time. :)
Flags: needinfo?(jhofmann)

Updated

a year ago
Depends on: 1297029

Updated

a year ago
Depends on: 1297041

Updated

a year ago
Depends on: 1297047
Depends on: 1297336

Comment 55

a year ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #48)
> Bug 1252224 bounced and I will need to dig deeper in that bug so I won't
> hold this up any longer. The code changes look fine and I tested out the
> patch and it worked well for me.

This change actually broke the height of the main view in the Application Menu when a taller subview like the History one is visible. I'll wait to file a regression as I'm investigating a general solution in bug 1292345.

Updated

a year ago
Depends on: 1009116
QA Contact: ovidiu.boca
This seems to have caused a few regressions.  Should we back it out before we get to aurora?  We don't have a lot of time.
Flags: needinfo?(past)
Flags: needinfo?(florian)
No longer depends on: 1297047
No longer depends on: 1297041
I don't mind backing this out, but the three regressions here (bug 1294576, bug 1297029, bug 1297336) may be fixed in time for the next merge. If Florian agrees, I would say let's make the call to back this out a week from today.
Flags: needinfo?(past)
(Assignee)

Comment 58

a year ago
Bug 1297029 looks like a platform issue, not a regression from this. The other 2 regressions are small enough that they could be fixed on aurora (but they'll likely be fixed before the merge anyway). I see no reason to backout.
No longer depends on: 1297029
Flags: needinfo?(florian)

Updated

a year ago
Depends on: 1299437

Comment 59

a year ago
(In reply to :Paolo Amadini from comment #55)
> This change actually broke the height of the main view in the Application
> Menu when a taller subview like the History one is visible. I'll wait to
> file a regression as I'm investigating a general solution in bug 1292345.

Since we're late in the cycle I'd rather not uplift bug 1009116, and in bug 1292345 we decided to accept the flickering for the Downloads Panel.

I've now filed bug 1299437 to take care of the height regression of the main menu in bug 1299437.
Depends on: 1300093

Updated

a year ago
Depends on: 1300089
Depends on: 1299211
No longer depends on: 1300089
No longer depends on: 1299211

Updated

a year ago
Depends on: 1301875
Whiteboard: [fxprivacy] → [fxprivacy][triage]

Updated

a year ago
Depends on: 1297041
(Assignee)

Updated

a year ago
No longer depends on: 1297041
Whiteboard: [fxprivacy][triage] → [fxprivacy]
(Assignee)

Updated

a year ago
Depends on: 1303339
No longer depends on: 1294430
Hi :flo,
May I know if this is enabled in nightly only or will it ride on 51?
Flags: needinfo?(florian)
(Assignee)

Comment 61

a year ago
(In reply to Gerry Chang [:gchang] from comment #60)
> Hi :flo,
> May I know if this is enabled in nightly only or will it ride on 51?

It will ride the trains with 51.
Flags: needinfo?(florian)
Depends on: 1309576
Depends on: 1309584
Depends on: 1309604
(Assignee)

Updated

a year ago
No longer depends on: 1309584
No longer depends on: 1309604
(Assignee)

Updated

a year ago
Depends on: 1320375
The bug is verified fixed on Windows 10 x64, Mac 10.11.6 and Ubuntu 14.04 x86, using 51.0b6 build1 (20161201172143).
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified

Updated

9 months ago
Depends on: 1345687

Updated

9 months ago
Depends on: 1345683

Updated

8 months ago
Depends on: 1345667
You need to log in before you can comment on or make changes to this bug.