1206233 - Indicator for devices actively streaming or permissions being used

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P1)

Comment 1

[[e-mail]]

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?

Comment 2

[[e-mail]]

Hi paolo, do you have idea about what this issue means and what the actionable task is?

Comment 3

[:Paolo Amadini]

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.

Comment 4

[Florian Quèze [:florian]]

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.

Comment 5

[Florian Quèze [:florian]]

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.

Comment 6

[[e-mail]]

Do we need platform API support (detect if camera/microphone is running) to work on this issue?

Comment 7

[Florian Quèze [:florian]]

(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)

Comment 8

[[e-mail]]

Thanks for the hint, getActiveStreams seems do the job
http://searchfox.org/mozilla-central/source/browser/modules/webrtcUI.jsm#106

Comment 9

[[e-mail]]

Attachment: 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.

Comment 10

[Florian Quèze [:florian]]

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.

Comment 11

[Florian Quèze [:florian]]

Attachment: Part 1, animated indicator above the (i) icon

Comment 12

[Florian Quèze [:florian]]

Attachment: Part 2, show used permissions in the control center panel

Comment 13

[Florian Quèze [:florian]]

Attachment: 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.

Comment 14

[[e-mail]]

Thanks for take care of this :)

Comment 15

[Florian Quèze [:florian]]

Attachment: Part 1, animated indicator above the (i) icon

Comment 16

[Florian Quèze [:florian]]

Attachment: 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.

Comment 17

[Florian Quèze [:florian]]

Attachment: Part 3, remove the old doorhangers

Comment 18

[Florian Quèze [:florian]]

Attachment: 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.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

(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.

Comment 21

[Florian Quèze [:florian]]

Attachment: Part 5, fix tests

Comment 22

[Johann Hofmann [:johannh]]

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

Comment 23

[Florian Quèze [:florian]]

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

Comment 24

[Johann Hofmann [:johannh]]

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.

Comment 25

[Florian Quèze [:florian]]

Attachment: 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.

Comment 26

[Florian Quèze [:florian]]

(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

[:Paolo Amadini]

(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

[:Paolo Amadini]

(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.

Comment 29

[Florian Quèze [:florian]]

(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).

Comment 30

[Florian Quèze [:florian]]

(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 31

[Johann Hofmann [:johannh]]

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.

Comment 32

[:Paolo Amadini]

(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

[:Paolo Amadini]

Helpful code constructs:

let foundPermissionOrUndefined = permissions.find(permission => permission.id == id);
let inUsePermissionList = ["camera", "microphone", "screen"].filter(id => this._sharingState[id]);

Comment 34

[Jared Wein [:jaws] (please needinfo? me)]

(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?

Comment 35

[Johann Hofmann [:johannh]]

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.

Comment 36

[:Paolo Amadini]

(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".

Comment 37

[Florian Quèze [:florian]]

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

Comment 38

[Florian Quèze [:florian]]

Attachment: 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.

Comment 39

[Florian Quèze [:florian]]

(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 40

[Johann Hofmann [:johannh]]

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. :)

Comment 41

[Florian Quèze [:florian]]

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

Comment 42

[Florian Quèze [:florian]]

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.

Comment 43

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

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

Comment 44

[Florian Quèze [:florian]]

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.

Comment 45

[Florian Quèze [:florian]]

(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

Comment 46

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

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

Comment 47

[Florian Quèze [:florian]]

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.

Comment 48

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/dd8193695d2d
Avoid flickering of the panel when removing permission items, r=jaws

Comment 50

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

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

Comment 51

[Marcia Knous [:marcia]]

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?

Comment 52

[Jared Wein [:jaws] (please needinfo? me)]

I will defer to Johann since he reviewed most of this work.

Comment 53

[Florian Quèze [:florian]]

(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.

Comment 54

[Johann Hofmann [:johannh]]

As Florian said, I'm not sure what to uplift here. On the contrary, I think this should get some Nightly baking time. :)

Comment 55

[:Paolo Amadini]

(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.

Comment 56

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 57

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 58

[Florian Quèze [:florian]]

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.

Comment 59

[:Paolo Amadini]

(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.

Comment 60

[Gerry Chang [:gchang]]

Hi :flo,
May I know if this is enabled in nightly only or will it ride on 51?

Comment 61

[Florian Quèze [:florian]]

(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.

Comment 62

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The bug is verified fixed on Windows 10 x64, Mac 10.11.6 and Ubuntu 14.04 x86, using 51.0b6 build1 (20161201172143).