Andreas Tolfsen ❲:ato❳ Reporter
	Description • 7 years ago

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.

	Robert Strong (they/them - no direct email)
	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.

	Alex Thayer [:alexical] (she/her) 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.

	:Gijs (he/him)
	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?

	Alex Thayer [:alexical] (she/her) 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!

	Robert Strong (they/them - no direct email)
	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?

	Alex Thayer [:alexical] (she/her) 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.

	Alex Thayer [:alexical] (she/her) 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?

	:Gijs (he/him)
	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. :-( )

	Alex Thayer [:alexical] (she/her) 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.

	Alex Thayer [:alexical] (she/her) 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 :/

	Mark Hammond [:markh] [:mhammond]
	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.

	Andrew Swan [:aswan]
	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

	Robert Strong (they/them - no direct email)
	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.

	Robert Strong (they/them - no direct email)
	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

	Edouard Oger [:eoger]
	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.

	Alex Thayer [:alexical] (she/her) 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?

	Robert Strong (they/them - no direct email)
	Comment 30 • 7 years ago

Good point and I forgot about those. Yes, it would be best if they were moved as well.

	:Gijs (he/him)
	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.

	Edouard Oger [:eoger]
	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

	Edouard Oger [:eoger]
	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!

	Robert Strong (they/them - no direct email)
	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.

	Robert Strong (they/them - no direct email)
	Comment 50 • 7 years ago

Doug, this is real close to being done. Please send the updated patches to try.

	Robert Strong (they/them - no direct email)
	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

	Julien Cristau [:jcristau] (back April 22)
	Comment 52 • 7 years ago

This was called out in the mid nightly 55 testing sign off report, tracking for 55.

	Alex Thayer [:alexical] (she/her) Assignee
	Comment 58 • 7 years ago

Link to try:

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

	Robert Strong (they/them - no direct email)
	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.

	:Gijs (he/him)
	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

	Pulsebot
	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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	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

	Kanchan Kumari QA
	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?

	Robert Strong (they/them - no direct email)
	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