Closed
Bug 1359733
Opened 7 years ago
Closed 7 years ago
Upgrade icon in hamburger menu not present in new windows
Categories
(Toolkit :: Application Update, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox54 | --- | unaffected |
firefox55 | + | fixed |
People
(Reporter: ato, Assigned: alexical)
References
Details
Attachments
(7 files, 1 obsolete file)
7.01 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
robert.strong.bugs
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
eoger
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
robert.strong.bugs
:
review+
|
Details |
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•7 years ago
|
Version: unspecified → 55 Branch
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dothayer
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Priority: -- → P1
Comment 3•7 years ago
|
||
mozreview-review |
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•7 years 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•7 years 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•7 years 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) |
Comment 13•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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)
Assignee | ||
Comment 17•7 years 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•7 years ago
|
Attachment #8867854 -
Attachment is obsolete: true
Attachment #8867854 -
Flags: review?(robert.strong.bugs)
Assignee | ||
Updated•7 years ago
|
Attachment #8864594 -
Flags: review?(gijskruitbosch+bugs)
Attachment #8867855 -
Flags: review?(aswan)
Attachment #8867856 -
Flags: review?(markh)
Attachment #8867857 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 23•7 years 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 24•7 years ago
|
||
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•7 years 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 26•7 years ago
|
||
mozreview-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.
Comment 27•7 years ago
|
||
(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•7 years 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•7 years 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)
Comment 30•7 years ago
|
||
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•7 years ago
|
Attachment #8867856 -
Flags: review?(markh) → review?(eoger)
Comment 35•7 years 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•7 years 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•7 years 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 49•7 years ago
|
||
mozreview-review |
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-
Comment 50•7 years ago
|
||
Doug, this is real close to being done. Please send the updated patches to try.
Comment 51•7 years ago
|
||
mozreview-review |
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+
Comment 52•7 years ago
|
||
This was called out in the mid nightly 55 testing sign off report, tracking for 55.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 58•7 years ago
|
||
Link to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fb893941ba3fd9b27eadf5ef022bc95f51f5a4d
Comment 59•7 years ago
|
||
mozreview-review |
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•7 years ago
|
Attachment #8867857 -
Flags: review?(paolo.mozmail) → review?(gijskruitbosch+bugs)
Comment 66•7 years 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•7 years 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 68•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0574649a7653 https://hg.mozilla.org/mozilla-central/rev/2d6b455cc316 https://hg.mozilla.org/mozilla-central/rev/ec0fb144db75 https://hg.mozilla.org/mozilla-central/rev/c778bb66f828 https://hg.mozilla.org/mozilla-central/rev/771a25029111 https://hg.mozilla.org/mozilla-central/rev/d4dc4bfab1ee
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 69•7 years 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
Comment 70•7 years ago
|
||
(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.
Description
•