Upgrade icon in hamburger menu not present in new windows

VERIFIED FIXED in Firefox 55

Status

()

Toolkit
Application Update
P1
normal
VERIFIED FIXED
10 months ago
9 months ago

People

(Reporter: ato, Assigned: dthayer)

Tracking

55 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 unaffected, firefox55+ fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

10 months ago
Created attachment 8861819 [details]
screenshot-2017-04-26-095127.png

When an update becomes available, a small green cue is added to the hamburger menu in Firefox.  For any subsequent windows you open, the indication that an update is available is missing.

This means that if you close one of your original windows and open a new one, you will be oblivious to the fact that an update is available.  The only way to still check is by opening the About dialogue or by waiting for the next update.

In the attached screenshot, the bottom window was opened after the top window was notified of a pending update.
(Reporter)

Updated

10 months ago
Version: unspecified → 55 Branch
(Assignee)

Updated

10 months ago
Assignee: nobody → dothayer
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8864594 [details]
Bug 1359733 - Move menu notification state to jsm

https://reviewboard.mozilla.org/r/136274/#review141286

Discussed this over irc with Gijs and dthayer and implementing this in a jsm would be a better approach.
Attachment #8864594 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 4

9 months ago
@Gijs, do you know the convention for defining jsm's that don't need to be called from anywhere specifically, but just need to observe messages? In order to move the PanelUI notification state out into a jsm, we will want something other than browser.js driving it for the updater UI.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 5

9 months ago
(In reply to Doug Thayer [:dthayer] from comment #4)
> @Gijs, do you know the convention for defining jsm's that don't need to be
> called from anywhere specifically, but just need to observe messages? In
> order to move the PanelUI notification state out into a jsm, we will want
> something other than browser.js driving it for the updater UI.

Yes, you can use this code: https://dxr.mozilla.org/mozilla-central/rev/ebbcdaa5b5802ecd39624dd2acbdda8547b8384d/browser/components/nsBrowserGlue.js#125

There's different sets of observers and other things, and we will avoid loading the jsm (which isn't free, so delaying it is good!) until the relevant observer notification fires. I *believe* this should work for the update code, right?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 6

9 months ago
(In reply to :Gijs from comment #5)
> Yes, you can use this code:
> https://dxr.mozilla.org/mozilla-central/rev/
> ebbcdaa5b5802ecd39624dd2acbdda8547b8384d/browser/components/nsBrowserGlue.
> js#125
> 
> There's different sets of observers and other things, and we will avoid
> loading the jsm (which isn't free, so delaying it is good!) until the
> relevant observer notification fires. I *believe* this should work for the
> update code, right?

Yup, that looks like exactly what I wanted. Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Doug, do you think the jsm is generic enough that it could be moved to toolkit? If so, I would really prefer if it were since other apps could use it and I will be able to remove the old app update ui sooner rather than later?
Flags: needinfo?(dothayer)
(Assignee)

Comment 14

9 months ago
It's pretty generic - the only thing that might not be so generic is the replaceReleaseNotes calls, which get the release notes link by ID in order to set the URL based on the value the nsIUpdate provides. But as long as the application provides something with those IDs ("update-available-whats-new", "update-manual-whats-new") it should be fine. If you'd like me to push up a version where we do nothing if those don't exist, I can, but I figure for right now erroring is a better option.
Flags: needinfo?(dothayer)
(Assignee)

Comment 15

9 months ago
Oh, actually there's one more piece that's not so generic, which is the call to openUILinkIn for manual update. I'm not sure what other applications have that can serve the same purpose. Thoughts?
Flags: needinfo?(robert.strong.bugs)

Comment 16

9 months ago
mozreview-review
Comment on attachment 8864594 [details]
Bug 1359733 - Move menu notification state to jsm

https://reviewboard.mozilla.org/r/136274/#review143112

I have a question about the architecture here (see below), and a few nits I noted, but this looks like a fine first step. Thanks!

::: browser/components/customizableui/content/panelUI.js:70
(Diff revision 3)
>      for (let event of this.kEvents) {
>        this.notificationPanel.addEventListener(event, this);
>      }
>  
>      this._initPhotonPanel();
> +    this._updateNotifications();

So, this feels tricky... this is what made things expensive in bug 1358173, AIUI. Doing this on init() is not ideal - it won't be necessary for the first window as there won't be any notifications.

The other thing I'm wondering about is whether we can avoid loading AppMenuNotifications on startup, and can leave it unloaded until a notification is actually created through it.

Perhaps what could work is if this code use `Services.obs.notifyObservers()` to effectively 'ask' if there were any notifications, and for AppMenuNotifications.jsm to send out an appMenu-notifications "reminder" about extant notifications *if* they exist, or something, that would only trigger in windows that had never heard about any notifications? That would effectively be a no-op on startup for the first window, and still result in future windows that post-date notifications to get those notifications.

Doug & Florian, what do you think?

::: browser/components/customizableui/content/panelUI.js:727
(Diff revision 3)
> +    if (notification.options.beforeShowDoorhanger) {
> +      notification.options.beforeShowDoorhanger(document);
> +    }

Is this supposed to be in a different patch?

::: toolkit/modules/AppMenuNotifications.jsm:9
(Diff revision 3)
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

Nit: use a destructuring assignment:

```js
const {interfaces: Ci, utils: Cu, classes: Cc} = Components;
```

::: toolkit/modules/AppMenuNotifications.jsm:25
(Diff revision 3)
> +  this.options = options;
> +  this.dismissed = this.options.dismissed || false;
> +}
> +
> +var AppMenuNotifications = {
> +  notifications: [],

So, this means that anybody can alter this array. Normally, for encapsulation, we make something like this a getter that produces a clone of the array that we keep internally (like with `Array.from(this._notifications)`). Is there a particular reason not to do this here?

(If we do do that, we'll need to use the underscore version internally, which I recognize is moderately annoying to switch to. :-( )
Attachment #8864594 - Flags: review?(gijskruitbosch+bugs)

Updated

9 months ago
Blocks: 1364473
(Assignee)

Comment 17

9 months ago
mozreview-review-reply
Comment on attachment 8864594 [details]
Bug 1359733 - Move menu notification state to jsm

https://reviewboard.mozilla.org/r/136274/#review143112

> So, this feels tricky... this is what made things expensive in bug 1358173, AIUI. Doing this on init() is not ideal - it won't be necessary for the first window as there won't be any notifications.
> 
> The other thing I'm wondering about is whether we can avoid loading AppMenuNotifications on startup, and can leave it unloaded until a notification is actually created through it.
> 
> Perhaps what could work is if this code use `Services.obs.notifyObservers()` to effectively 'ask' if there were any notifications, and for AppMenuNotifications.jsm to send out an appMenu-notifications "reminder" about extant notifications *if* they exist, or something, that would only trigger in windows that had never heard about any notifications? That would effectively be a no-op on startup for the first window, and still result in future windows that post-date notifications to get those notifications.
> 
> Doug & Florian, what do you think?

Sounds good to me. Hopefully the new diff matches what you were expecting!

> Is this supposed to be in a different patch?

I suppose it makes more sense in the updater patch. Moving it there.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8867854 - Attachment is obsolete: true
Attachment #8867854 - Flags: review?(robert.strong.bugs)
(Assignee)

Updated

9 months ago
Attachment #8867856 - Flags: review?(markh)
Attachment #8867857 - Flags: review?(paolo.mozmail)
Attachment #8864594 - Flags: review?(gijskruitbosch+bugs)
Attachment #8867855 - Flags: review?(aswan)
(Assignee)

Comment 23

9 months ago
Comment on attachment 8864594 [details]
Bug 1359733 - Move menu notification state to jsm

For some reason, though Reviewboard automatically re-requested review from Gijs, after adding other review requests it automatically rescinded it. Manually adding it back :/
Attachment #8864594 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8867856 [details]
Bug 1359733 - (pt. 4) Pull out browser-sync.js badges

This looks fine to me, but eoger recently redid all this code, so his eye on it would be good too.
Attachment #8867856 - Flags: review?(markh) → review?(eoger)

Comment 25

9 months ago
mozreview-review
Comment on attachment 8867855 [details]
Bug 1359733 - (pt. 3) Pull out browser-addons.js badges

https://reviewboard.mozilla.org/r/139362/#review143294
Attachment #8867855 - Flags: review?(aswan) → review+
Comment on attachment 8867853 [details]
Bug 1359733 - (pt. 2) Refactor gMenuButtonUpdateBadge

https://reviewboard.mozilla.org/r/139358/#review143296

::: toolkit/modules/UpdateListener.jsm:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please move this to toolkit/mozapps/update/

JSM's specific to a component are typically in the components directory.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #26)
> Comment on attachment 8867853 [details]
> Bug 1359733 - (pt. 2) Refactor gMenuButtonUpdateBadge
> 
> https://reviewboard.mozilla.org/r/139358/#review143296
> 
> ::: toolkit/modules/UpdateListener.jsm:1
> (Diff revision 2)
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Please move this to toolkit/mozapps/update/
> 
> JSM's specific to a component are typically in the components directory.
See the following for one example
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions
Flags: needinfo?(robert.strong.bugs)

Comment 28

9 months ago
mozreview-review
Comment on attachment 8867856 [details]
Bug 1359733 - (pt. 4) Pull out browser-sync.js badges

https://reviewboard.mozilla.org/r/139364/#review143314

UIState is not supposed to manipulate the UI directly.
I'd rather see nsBrowserGlue listen to UIState.ON_UPDATE and update the badge if necessary.
Attachment #8867856 - Flags: review?(eoger) → review-
(Assignee)

Comment 29

9 months ago
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #27)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #26)
> > Comment on attachment 8867853 [details]
> > Bug 1359733 - (pt. 2) Refactor gMenuButtonUpdateBadge
> > 
> > https://reviewboard.mozilla.org/r/139358/#review143296
> > 
> > ::: toolkit/modules/UpdateListener.jsm:1
> > (Diff revision 2)
> > > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > 
> > Please move this to toolkit/mozapps/update/
> > 
> > JSM's specific to a component are typically in the components directory.
> See the following for one example
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions

Should I leave the appUpdate tests where they are for right now or move them under something like toolkit/mozapps/update/tests/browser?
Flags: needinfo?(robert.strong.bugs)
Good point and I forgot about those. Yes, it would be best if they were moved as well.
Flags: needinfo?(robert.strong.bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

9 months ago
Attachment #8867856 - Flags: review?(markh) → review?(eoger)

Comment 35

9 months ago
mozreview-review
Comment on attachment 8864594 [details]
Bug 1359733 - Move menu notification state to jsm

https://reviewboard.mozilla.org/r/136274/#review143970

::: toolkit/modules/AppMenuNotifications.jsm:47
(Diff revisions 3 - 4)
> +    switch (topic) {
> +      case "xpcom-shutdown":
> +        this.uninit();
> +        break;
> +      case "appMenu-notifications-request":
> +        Services.obs.notifyObservers(null, "appMenu-notifications", "init");

Can we avoid sending this at all if `this._notifications.length == 0` ?

::: browser/components/customizableui/test/browser_panelUINotifications_multiWindow.js:114
(Diff revision 4)
> +  let notifications = [...win.PanelUI.notificationPanel.children].filter(n => !n.hidden);
> +  let doorhanger = notifications[0];
> +  let button = doc.getAnonymousElementByAttribute(doorhanger, "anonid", "secondarybutton");
> +  button.click();

Might be useful to add a test assertion here that the notifications[0] entry exists, because the failure case here right now is going to be this code throwing an error and failing to click and dismiss the button.

::: browser/components/nsBrowserGlue.js:50
(Diff revision 4)
>  
>  [
>    ["AboutHome", "resource:///modules/AboutHome.jsm", "init"],
>    ["AboutNewTab", "resource:///modules/AboutNewTab.jsm"],
>    ["AddonManager", "resource://gre/modules/AddonManager.jsm"],
> +  ["AppMenuNotifications", "resource://gre/modules/AppMenuNotifications.jsm"],

This doesn't seem to be used in nsBrowserGlue - should this be part of a later commit? Also, if you add this and do end up using it, you will need to update the commented list at the top for eslint's benefit.
Attachment #8864594 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 36

9 months ago
mozreview-review
Comment on attachment 8867856 [details]
Bug 1359733 - (pt. 4) Pull out browser-sync.js badges

https://reviewboard.mozilla.org/r/139364/#review144176

Almost there, thank you!

::: browser/base/content/test/sync/browser_sync.js:5
(Diff revision 3)
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  

The new badge logic does not reside in browser-sync anymore, so let's put the tests in a different file too.
Since I feel like I'm asking a bit much, I wrote the new test file for you:
https://pastebin.mozilla.org/9022117

::: browser/base/content/test/sync/browser_sync.js:173
(Diff revision 3)
>    let monthAgoString = gSync.formatLastSyncDate(monthAgo);
>    is(monthAgoString, "Last sync: " + monthAgo.toLocaleDateString(undefined, {month: "long", day: "numeric"}),
>       "The date is correctly formatted");
>  });
>  
>  function checkFxABadge(shouldBeShown) {

Ditto:
Remove your changes from this file, then remove checkFxABadge and all its usages.

::: browser/components/nsBrowserGlue.js:16
(Diff revision 3)
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/AppConstants.jsm");
>  Cu.import("resource://gre/modules/AsyncPrefs.jsm");
> +Cu.import("resource://services-sync/UIState.jsm");

Please make this a lazy getter

::: browser/components/nsBrowserGlue.js:496
(Diff revision 3)
>          });
>          break;
>        case "test-initialize-sanitizer":
>          this._sanitizer.onStartup();
>          break;
> +      case UIState.ON_UPDATE:

Replace this by "sync-ui-state:update" so we don't pull UIState too early.

::: browser/components/nsBrowserGlue.js:537
(Diff revision 3)
>      }
>      os.addObserver(this, "browser-search-engine-modified");
>      os.addObserver(this, "restart-in-safe-mode");
>      os.addObserver(this, "flash-plugin-hang");
>      os.addObserver(this, "xpi-signature-changed");
> +    os.addObserver(this, UIState.ON_UPDATE);

Ditto

::: browser/components/nsBrowserGlue.js:592
(Diff revision 3)
>        os.removeObserver(this, "keyword-search");
>      }
>      os.removeObserver(this, "browser-search-engine-modified");
>      os.removeObserver(this, "flash-plugin-hang");
>      os.removeObserver(this, "xpi-signature-changed");
> +    os.removeObserver(this, UIState.ON_UPDATE);

Ditto
Attachment #8867856 - Flags: review?(eoger) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 42

9 months ago
mozreview-review
Comment on attachment 8867856 [details]
Bug 1359733 - (pt. 4) Pull out browser-sync.js badges

https://reviewboard.mozilla.org/r/139364/#review144262

Perfect thanks!
Attachment #8867856 - Flags: review?(eoger) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8867853 [details]
Bug 1359733 - (pt. 2) Refactor gMenuButtonUpdateBadge

https://reviewboard.mozilla.org/r/139358/#review143894

::: commit-message-3d26b:10
(Diff revision 5)
> +within browser.js. This patch moves the browser.js logic out into
> +a jsm file that is wired up through nsBrowserGlue, such that it
> +will be lazily instantiated on the first update event it would
> +receive[1].
> +
> +I was uncertain where this code should go. browser/components

Please be sure to update this to reflect the decision.

::: browser/base/content/test/appUpdate/head.js:359
(Diff revision 5)
>        }
>  
>        let testUpdater = testUpdaterDir.clone();
>        testUpdater.append(FILE_UPDATER_BIN);
> +
> +      logTestInfo(testUpdater.path);

If you want to keep these please make them debugDump instead of logTestInfo.

Also, prefix the path with what is being logged.

::: browser/base/moz.build
(Diff revision 5)
>      'content/test/webextensions/browser.ini',
>      'content/test/webrtc/browser.ini',
>      'content/test/windows/browser.ini',
>  ]
>  
> -if CONFIG['MOZ_UPDATER']:

Please also remove the following from browser/base/content/moz.build
with Files("test/appUpdate/**"):
    BUG_COMPONENT = ("Toolkit", "Application Update")

::: browser/components/nsBrowserGlue.js:182
(Diff revision 5)
>    },
>  
>    observe(subject, topic, data) {
>      for (let module of this.observers[topic]) {
>        try {
> -        this[module].observe(subject, topic, data);
> +        global[module].observe(subject, topic, data);

I don't think you want to change this to global. The change to global in nsBrowserGlue.js is for messageManager and not observers. Also, I don't see any other observers using global.

::: toolkit/modules/moz.build:148
(Diff revision 5)
>      BUG_COMPONENT = ('Firefox', 'Toolbars and Customization')
>  
>  with Files('Sqlite.jsm'):
>      BUG_COMPONENT = ('Toolkit', 'Storage')
>  
> +with Files('UpdateListener.jsm'):

Remove this... it is already added in the dir it lives in.

::: toolkit/mozapps/update/UpdateListener.jsm:9
(Diff revision 5)
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["UpdateListener"];
> +
> +const Cu = Components.utils;

nit: this is fine if you want to leave it this way... just wanted to say that I typically go with the following.
const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

::: toolkit/mozapps/update/UpdateListener.jsm:69
(Diff revision 5)
> +      }
> +    }
> +  },
> +
> +  requestRestart() {
> +    let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"]

nit: I typically go with the following style
let longerFooBar = Cc["@mozilla.org/longer-foo-bar;1"].
                   getService(Ci.nsILongerFooBar);
https://developer.mozilla.org/en-US/docs/User:GavinSharp_JS_Style_Guidelines

::: toolkit/mozapps/update/UpdateListener.jsm:115
(Diff revision 5)
> +    this.showUpdateNotification("restart", dismissed, () => this.requestRestart());
> +  },
> +
> +  showUpdateAvailableNotification(update, dismissed) {
> +    this.showUpdateNotification("available", dismissed, () => {
> +      let updateService = Cc["@mozilla.org/updates/update-service;1"]

nit: I typically go with the following style
let longerFooBar = Cc["@mozilla.org/longer-foo-bar;1"].
                   getService(Ci.nsILongerFooBar);
https://developer.mozilla.org/en-US/docs/User:GavinSharp_JS_Style_Guidelines

::: toolkit/mozapps/update/moz.build:31
(Diff revision 5)
>  EXTRA_JS_MODULES += [
> +    'UpdateListener.jsm',
>      'UpdateTelemetry.jsm',
>  ]
>  
> +TEST_HARNESS_FILES.testing.mochitest.browser.toolkit.mozapps.update.tests.browser += [

Please move this to
toolkit/mozapps/update/tests/moz.build

just above
TEST_HARNESS_FILES.testing.mochitest.chrome.toolkit.mozapps.update.tests.data += [

and make the appropriate path changes

::: toolkit/mozapps/update/tests/moz.build:11
(Diff revision 5)
>  
>  HAS_MISC_RULE = True
>  
>  FINAL_TARGET = '_tests/xpcshell/toolkit/mozapps/update/tests/data'
>  
> +BROWSER_CHROME_MANIFESTS += ['browser/browser.ini']

To prevent these tests from failing on Seamonkey this needs to be excluded for Seamonkey. I think you can just go with the following. Please verify with one of the build engineers (gps, ted, etc.) that this is ok.
if not CONFIG['MOZ_SUITE']:
    BROWSER_CHROME_MANIFESTS += ['browser/browser.ini']

Also, please file a followup bug for converting these tests to mochitest-chrome tests so the tests can run on Thunderbird if they implement the same UI. I have no time frame for converting them and I'm not looking to have the tests converted anytime in the near future.
Attachment #8867853 - Flags: review?(robert.strong.bugs) → review-
Doug, this is real close to being done. Please send the updated patches to try.
Comment on attachment 8869258 [details]
Bug 1359733 - (pt. 2.1) Move remaining test files

https://reviewboard.mozilla.org/r/140816/#review144462
Attachment #8869258 - Flags: review?(robert.strong.bugs) → review+
This was called out in the mid nightly 55 testing sign off report, tracking for 55.
status-firefox54: --- → unaffected
status-firefox55: --- → affected
tracking-firefox55: --- → +
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8867853 [details]
Bug 1359733 - (pt. 2) Refactor gMenuButtonUpdateBadge

https://reviewboard.mozilla.org/r/139358/#review144726

This looks good to me. There have been changes to nsBrowserGlue.js that removed the observer so you will need to update the patches to latest m-c and re-add that code. I can take a look at those changes and land the patches after the patches have been updated.
Attachment #8867853 - Flags: review?(robert.strong.bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8867857 - Flags: review?(paolo.mozmail) → review?(gijskruitbosch+bugs)

Comment 66

9 months ago
mozreview-review
Comment on attachment 8867857 [details]
Bug 1359733 - (pt. 5) Pull out indicator.js badges

https://reviewboard.mozilla.org/r/139366/#review145612
Attachment #8867857 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 67

9 months ago
Pushed by rstrong@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0574649a7653
Move menu notification state to jsm r=Gijs
https://hg.mozilla.org/integration/autoland/rev/2d6b455cc316
(pt. 2) Refactor gMenuButtonUpdateBadge r=rstrong
https://hg.mozilla.org/integration/autoland/rev/ec0fb144db75
(pt. 2.1) Move remaining test files r=rstrong
https://hg.mozilla.org/integration/autoland/rev/c778bb66f828
(pt. 3) Pull out browser-addons.js badges r=aswan
https://hg.mozilla.org/integration/autoland/rev/771a25029111
(pt. 4) Pull out browser-sync.js badges r=eoger
https://hg.mozilla.org/integration/autoland/rev/d4dc4bfab1ee
(pt. 5) Pull out indicator.js badges r=Gijs

Comment 69

9 months ago
I tested this bug fix. Reported issue is not reproducible anymore. 

Note (My observations): Doorhanger UI doesn't appear concurrently in all opened windows. Doorhanger UI appears after windows get focused. Please see this video https://ranisharma22.tinytake.com/sf/MTYzNjcyMV81NTI1NDQ2

Is this intended?
Status: RESOLVED → VERIFIED
(In reply to Kanchan Kumari QA from comment #69)
> I tested this bug fix. Reported issue is not reproducible anymore. 
> 
> Note (My observations): Doorhanger UI doesn't appear concurrently in all
> opened windows. Doorhanger UI appears after windows get focused. Please see
> this video https://ranisharma22.tinytake.com/sf/MTYzNjcyMV81NTI1NDQ2
> 
> Is this intended?
Yes. See bug 1358363 comment #5 and Bram's approval of this approach in bug 1358363 comment #9
You need to log in before you can comment on or make changes to this bug.