893505 - Simplify the application update UI

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Attachment: mockup

It has been several years since we last used the remote billboard and the license pages so these should be removed. Also, the current UI, which can be seen in attachment #430909 [details], is much larger than it needs to be and was made this size to support a large billboard. I'd like to use panels if at all possible (see attached mockup).

This bug does not cover the about dialog. That is bug 617101

Basic requirements as I see them:
* inform users of the add-ons that aren't compatible with the update before downloading an update (this list can be long so it will need to be scrollable).
* inform users that an update is available.
* inform users that they should restart to apply an update (for users that don't restart often).
* inform users that can't apply an update that they should manually install the new version to upgrade via a link.
* inform users when there are repeated errors when trying to update and should manually install the new version to upgrade via a link.

Possible requirements:
* Ability to show UI equivalent to the remote billboard (open a web page?)
* Ability to survey users that don't update (open a web page?)

Note: I haven't included all of the UI in the mockup that is needed (e.g. errors, manual install, etc.)

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Attachment: sc.png

Maybe using the e10s windows could facilitate the work on this feature.
The current update window could need a refresh.

Comment 4

[Robert Strong (they/them - no direct email)]

It's unlikely that I will get to work on this in the next two weeks, so unassigning myself for now.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Bram, could you please share your plans? Thanks

Comment 7

[Bram Pitoyo [:bram]]

Hi Sylvestre,

My plan is to continue the design work that you and rstrong had done with the update UI. This will involve specifying its behaviour, doing some of the visual design, and maybe creating an interactive prototype of it.

Earlier, a few of us met and made a few decision related to this, that you might like to review and see how it will impact other parts of Firefox. You can find it under the heading “Decisions Needed”:

https://docs.google.com/document/d/1CGPMWaOxtBMDgKkAJCLg7FHn_Z-9wcFjt98wj9frEks/edit

Comment 9

[Bram Pitoyo [:bram]]

The latest user flow can be found here:
https://www.lucidchart.com/documents/view/0be491ba-ae0f-4c3f-adba-28d81c1d9356

It accounts for all of these conditions:

1. On update check disabled
2. On update check failure (and multiple failures)
3. On download failure
4. If user hasn’t restarted after x period of time
5 On user type ≠ administrator (I think we are working on this problem, so that you can still install an update even though you’re not an admin)
6. On admin password entry failure
7. On update apply failure

Robert is going to provide some feedback on this flow, then we can go ahead and design the notifications and write the messages.

(At the moment, my mockup only accounts for condition #3 and #4: https://invis.io/Y776FIBWS)

Comment 10

[Robert Strong (they/them - no direct email)]

(In reply to Bram Pitoyo [:bram] from comment #9)
> The latest user flow can be found here:
> https://www.lucidchart.com/documents/view/0be491ba-ae0f-4c3f-adba-
> 28d81c1d9356
> 
> It accounts for all of these conditions:
> 
> 1. On update check disabled
> 2. On update check failure (and multiple failures)
> 3. On download failure
> 4. If user hasn’t restarted after x period of time
It might be a good thing to have a visual indicator like the green arrow when we are ready to restart and then after x period of time bring up ui as this states.

> 5 On user type ≠ administrator (I think we are working on this problem, so
> that you can still install an update even though you’re not an admin)
There will always be the case where the user does not have the privileges to install an update. This should be the Download a Fresh copy path.

> 6. On admin password entry failure
> 7. On update apply failure
Note: This notification is displayed after restart.

Comment 12

[Bram Pitoyo [:bram]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)

I’ve updated the logic diagram to account for the situation you’ve mentioned:

1. Green arrow when update is available and when we’re ready to restart

2. There’s no more administrator check within Firefox itself, because we obviously can’t do that. Instead, we’ll ask user to input an admin password when Firefox has already quit, and when escalation is required before applying the update. If password input fails, the update can’t install and Firefox will still start, but user is then invited to Download a Fresh Copy

3. Elaborated the restart condition. I’ve split it into “Quit Firefox”, “Apply update” and “Start Firefox”, so it’s clear exactly when each notification is displayed. The “Download a Fresh Copy” notification is displayed after restart.

The latest user flow can always be found on comment 9, with the URL:
https://www.lucidchart.com/documents/view/0be491ba-ae0f-4c3f-adba-28d81c1d9356

Comment 13

[Robert Strong (they/them - no direct email)]

Hi Bram, I finished some app update code removal that should make this easier to implement and I'm going to work on this as time permits.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Do we want a link to the release notes from the update windows?
Anyway, any progress on this would be great!

Comment 15

[Robert Strong (they/them - no direct email)]

This will be a very simple user interface and the one in the about window might be a better place for it.

Comment 16

[Neil Deakin]

Attachment: Notifications work in progress

This is some code which shows the three new notifications. Some dummy buttons are added to browser.xul to trigger them.

Work that needs to happen:

1. The icons need to be added.
2. Buttons and links hooked up to actions
3. PopupNotifications.jsm assumes that all notifications will hang out of the icon on the url field, so this will need to be adjusted to support the 'PanelUI-button'

Comment 17

[Alex Thayer [:alexical] (she/her)]

Attachment: Multiple notifications

(In reply to Neil Deakin from comment #16)
> Created attachment 8819330 [details] [diff] [review]
> Notifications work in progress
> 
> This is some code which shows the three new notifications. Some dummy
> buttons are added to browser.xul to trigger them.
> 
> Work that needs to happen:
> 
> 1. The icons need to be added.
> 2. Buttons and links hooked up to actions
> 3. PopupNotifications.jsm assumes that all notifications will hang out of
> the icon on the url field, so this will need to be adjusted to support the
> 'PanelUI-button'

There seem to be a few little bugs with the `PopupNotifications.jsm` stuff in general. It seems like in the event of multiple notifications coming in, it tries to just display them one-by-one FIFO style, but it doesn't quite succeed. Attaching a screenshot to highlight the issue. You can repro this by going to addons.mozilla.org and trying to install several addons in quick succession, not accepting anything but just letting them download. You'll see them pile up while they're downloading, then collapse down when they're done. At the end of it it behaves like a queue of dialogs.

I'm trying to figure out what the intended behavior is - this effect becomes a little weirder once we start allowing anchoring to the `PanelUI-menu-button`, because if we take the simple path there, it can cause the add-on notification to jump to be anchored there.

Comment 18

[Bram Pitoyo [:bram]]

In your interest, my invision mocks were updated last week and can be seen here:
http://mozilla.invisionapp.com/share/Y776FIBWS

Comment 19

[Sylvestre Ledru [:Sylvestre]]

> In your interest, my invision mocks were updated last week and can be seen
> here.
Clap clap clap, I am a big fan. Much better. Bravo!

Comment 20

[Alex Thayer [:alexical] (she/her)]

I'm wondering if `PopupNotifications.jsm` should really be what we're using here. It doesn't seem like too big of a lift to make a different notifications service for the hamburger dropdown, and I think that might simplify things overall. A lot of the complexity of `PopupNotifications.jsm` seems to come from ideas like scoping notifications to a particular tab, and keeping a queue of notifications around, which we probably don't want to do. Additionally, this would let us keep it in a different <panel> element, which would let us change the properties around if we need to, and it's going to make it easier to change set the alignment to "bottomcenter topright", which we'll need in order to have the notifications display well in the top right of the window. Thoughts?

Regarding the UI, do we want this popup to autohide when the user clicks somewhere else in the screen, or do we want to require them to click "Not now"?

Comment 21

[Neil Deakin]

> Additionally, this would let us keep it in a different <panel> element,
> which would let us change the properties around if we need to, and it's
> going to make it easier to change set the alignment to "bottomcenter
> topright", which we'll need in order to have the notifications display well
> in the top right of the window. Thoughts?

Yes, it seems like it should just be a different panel and not use PopupNotifications. Perhaps break out just the pieces from PopupNotifications that are needed.

Comment 22

[Alex Thayer [:alexical] (she/her)]

Attachment: Draft of new update notifications

Let me know how this looks. I copied over `PopupNotifications.jsm` and stripped out things that didn't make any sense when applied to the hamburger dropdown (is PanelUI the canonical name for this?) I also stripped out telemetry, but we can add back in the telemetry that we want - I'm not super familiar with best practices in that regard. It's still more complex than it needs to be for doing just want we want it to do, but if we ever want to add more notifications hanging from the hamburger dropdown, this should make it simpler.

The tests aren't done yet, I brought over some of the tests for the `PopupNotifications.jsm` stuff, but I wanted to get feedback on if this general approach was good or not before diving too deep.

Two alternatives I can think of to this approach:
- Don't make a `PanelUINotifications.jsm` module - just write the minimum code required to display and dismiss our three notifications. This would make our notifications simpler, but it would make it more complicated down the line in the event that we wanted to add more notifications to the hamburger which could conceivably happen at the same time as our update notifications. Our current solution makes sure that we don't have multiple notifications stacked on top of each other.

- Just extend `PopupNotifications.jsm` with what we need. We'd have to add an option to the notification to specify it's alignment (`bottomcenter topright` in our case), and we'd have to add an option allowing us to specify that we want this notification to persist across tab changes, and we would have to grab elements by id outside of just the icon box. Ultimately it wouldn't be too crazy, but I think that these notifications will in general have different kinds of requirements than notifications that use the icon box, so it makes sense to split them out early.

Comment 23

[Alex Thayer [:alexical] (she/her)]

Attachment: Latest

I have a few questions about this:

On the technical side (:enn):

- At what level should I provide hooks to wire into, and what should those hooks look like? Should I add methods into `nsIUpdateService.idl`? Or should I just add observers and get notified (like the old system: `Services.obs.addObserver(this, "update-staged", false);`)?

- Right now the green menu item and the green arrow badge are separate from the doorhanger, and need to be wired up together. Should they instead just all be automatically worked into the new `PanelUINotifications.jsm` logic (assuming we stick with the `PanelUINotifications.jsm` approach)?

On the design side (:bram):

- The "Restyle doorhanger notifications" bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1267604) seems to overlap with this a bit, in that we're using those designs for the doorhangers for this. However, they don't _quite_ match up with the design you linked at https://mozilla.invisionapp.com/share/Y776FIBWS#/screens/155743946. For instance, the border on the top of the buttons is just grey, whereas in your design it's a dark blue above the blue button. I like your design better than what's actually in Nightly right now, because since the grey line is lighter than the blue CTA, but darker than the default button, it makes it look like the CTA is one pixel shorter than the default button. Should I update the styles for doorhangers to match your designs?

- Another way the design doesn't match up is that the top label ("Firefox can't update to the latest version"), isn't any larger than the bottom description ("Download a fresh copy..."). Again, if we were to align this with your design, we'd probably want to align all doorhangers. Should I also make the change to do that?

- Should we display the green arrow _before_ the doorhanger (maybe after a few days of not restarting rather than a whole week)? I wasn't totally sure of this based on the design. It might be nice because they have the opportunity to voluntarily engage with it and not have to be disrupted by the doorhanger when they didn't realize they even needed to restart, but it also could be worse because the ideal scenario is they never had to notice anything at all.

Comment 24

[Neil Deakin]

> - At what level should I provide hooks to wire into, and what should those
> hooks look like? Should I add methods into `nsIUpdateService.idl`? Or should
> I just add observers and get notified (like the old system:
> `Services.obs.addObserver(this, "update-staged", false);`)?

I think that would be a question for Rob Strong instead.

> - Right now the green menu item and the green arrow badge are separate from
> the doorhanger, and need to be wired up together. Should they instead just
> all be automatically worked into the new `PanelUINotifications.jsm` logic
> (assuming we stick with the `PanelUINotifications.jsm` approach)?

I think they should be specified separately in the browser's css, similarly to the ones for the existing notifications (those are in browser/themes/shared/notification-icons.inc.css). Either a default or no image should probably be used by default.

The image for the green menu item should go where the existing code for the update menuitem is.

As a tip, make sure to check the 'Need more information from' field if you want a question answered as bugzilla will track these for the people you're asking.

Comment 25

[Alex Thayer [:alexical] (she/her)]

(In reply to Neil Deakin from comment #24)
> > - At what level should I provide hooks to wire into, and what should those
> > hooks look like? Should I add methods into `nsIUpdateService.idl`? Or should
> > I just add observers and get notified (like the old system:
> > `Services.obs.addObserver(this, "update-staged", false);`)?
> 
> I think that would be a question for Rob Strong instead.

Ah, okay then. Adding him.

> As a tip, make sure to check the 'Need more information from' field if you
> want a question answered as bugzilla will track these for the people you're
> asking.

Thank you! Still learning the ropes. Adding in :rstrong and :bram.

Comment 26

[Robert Strong (they/them - no direct email)]

Go with the existing method and I'll evaluate a new method later.

Comment 27

[Alex Thayer [:alexical] (she/her)]

Just wanted to get a bit of clarification, Neil - my plan is to wrap the code for displaying the menu badge and green menu options (not the css or images) in the PanelUINotifications.jsm module, and migrate existing things that use this (firefox account sync is the only one that's not behind a disabled pref) to use the PanelUINotifications.jsm module. I think this will simplify things in the long term, as any questions about the priority of multiple badges and menu items and their interaction when they're both present will be answerable in one central place. It's also just going to make our code read much nicer if we colocate most of it.

Does that seem like the right approach to you?

Comment 28

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 893505 - Simplify the application update UI

Review commit: https://reviewboard.mozilla.org/r/100222/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100222/

Comment 29

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/1-2/

Comment 30

[Alex Thayer [:alexical] (she/her)]

I'm just realizing this, sorry it's a bit late, but there's an infinite UI loop in this diagram if downloads consistently fail: https://pageshot.net/osmBeugOW0Mhmh32/www.lucidchart.com. What do we want to do about this? I also don't really understand the user flow in this loop. If the download failed, what reason do we have to believe that the user manually clicking something to trigger the same download is going to work any better? I understand if we were directing them to a manual download page, like in the "Download a fresh copy" flow, but in this case we're just re-triggering what we tried already. It seems like we should automatically retry, and if we fail enough times, then show them the "Download a fresh copy" flow. In this case, we don't need the "Update available" notification at all.

Thoughts, rstrong? I'm also including you on the code review - if we end up scrapping the "Update available" flow, then this should already be wired up correctly, but I'd still appreciate any feedback.

I'm not sure where to add overall comments in reviewboard, so I'll add them here for now:

- There aren't any tests yet - again I'd like to know that this isn't the completely wrong path before investing too much time in the tests. In the future I'll be trending toward a more TDD approach, but with this uncertainty I think it's simpler to test afterwards.

- I brought the menu badge and menu footer mechanisms together with our system, since they are interrelated. I was hoping I could modify the Firefox Accounts stuff to use the system the same way we do, but I realized their menu item was a bit more complicated than ours and didn't gel nicely with our badge/menu item system. Might be a TODO to consolidate that.

- I colocated the notifications stuff with all the regular panelUI logic. This is because we need to be aware of what's going on in the PanelUI (when it shows, hides, etc.) so that we hide our doorhangers and badges when the user opens up the menu. I thought it seemed the cleanest and most discoverable to keep it all in there. There were a few earlier comments in this bug about it, but using PopupNotifications.jsm seemed wrong, since our doorhangers/badges are complicated by things that existing doorhangers don't care about, and existing doorhangers are complicated in ways that our doorhangers/badges aren't.

Comment 31

[Robert Strong (they/them - no direct email)]

(In reply to Doug Thayer [:dthayer] from comment #30)
> I'm just realizing this, sorry it's a bit late, but there's an infinite UI
> loop in this diagram if downloads consistently fail:
> https://pageshot.net/osmBeugOW0Mhmh32/www.lucidchart.com. What do we want to
> do about this? I also don't really understand the user flow in this loop. If
> the download failed, what reason do we have to believe that the user
> manually clicking something to trigger the same download is going to work
> any better? I understand if we were directing them to a manual download
> page, like in the "Download a fresh copy" flow, but in this case we're just
> re-triggering what we tried already. It seems like we should automatically
> retry, and if we fail enough times, then show them the "Download a fresh
> copy" flow. In this case, we don't need the "Update available" notification
> at all.
If all mar downloads fail it should be the Download a fresh copy UI. Note: some updates have both a partial and complete mar and others will only have a complete mar.

> Thoughts, rstrong? I'm also including you on the code review - if we end up
> scrapping the "Update available" flow, then this should already be wired up
> correctly, but I'd still appreciate any feedback.
Sounds good

Comment 32

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/2-3/

Comment 33

[Neil Deakin]

I can take a look at reviewing some of this, but Gijs for the panelUI changes and Dao for style changes would be better reviewers for those portions.

I assume you mean to remove the debugging buttons from browser.xul

Comment 34

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/3-4/

Comment 35

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/4-5/

Comment 36

[Alex Thayer [:alexical] (she/her)]

Thanks Neil! (and yes, I was planning on removing the debugging buttons before landing this anywhere - just removed them in the latest review though anyway.)

Adding Gijs first per your suggestion.

Comment 37

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/5-6/

Comment 38

[:Gijs (he/him)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review102470

I haven't tested this patch - these are just code comments. I'll try to remember to look in more detail in the next round of reviews.

This generally looks pretty good already, so please don't be put off by the comments. :-)

Note though that the current badging system is preffed off except on nightly/devedition. Do we want to use it everywhere? And if so do we want to remove / turn off the dialog stuff?

::: browser/base/content/browser.js:2667
(Diff revision 6)
> -      this.displayBadge(false);
> +      let openManualUpdateUrl = this.openManualUpdateUrl;
> +      let action = {
> +        callback: () => { openManualUpdateUrl(); }
> +      };

Instead of pulling this method out into a local variable, just make the callback function call `gMenuButtonUpdateBadge.openManualUpdateUrl();` directly? That also fixes `this` in the method (which we don't use right now, but it's always an annoying gotcha if you try and use it later).

::: browser/base/content/browser.js:2689
(Diff revision 6)
> -    let stringId;
> -    let updateButtonText;
> -    if (succeeded) {
> -      let brandBundle = document.getElementById("bundle_brand");
> +    let requestRestart = this.requestRestart;
> +    let action = {
> +      callback: () => { requestRestart(); }
> +    };

Same here.

::: browser/base/content/test/general/browser_panelUINotifications.js:1
(Diff revision 6)
> +add_task(function*() {

Nit: Please name this function and add a short docstring comment indicating what this test does.


Also, please add this in the customizableui dir instead of in browser/.../general/ [0]. CUI isn't the perfect place, but it's moderately topical considering these tests mostly revolve around the innards of panelUI.js, which lives there.

[0] I ask because that dir is a dumping ground and a haven for intermittents that are impossible to reproduce or fix because of the large number of tests that get run in a single dir.

::: browser/base/content/test/general/browser_panelUINotifications.js:2
(Diff revision 6)
> +add_task(function*() {
> +  yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:blank"},

I know it's not obvious from the doc, but this works if you just pass a string for a URL, instead of a full options object. :-)

::: browser/base/content/test/general/browser_panelUINotifications.js:10
(Diff revision 6)
> +
> +      let mainActionCalled = false;
> +      let mainAction = {
> +        callback: () => { mainActionCalled = true; }
> +      };
> +      PanelUI.showNotification("update-manual", mainAction, [], {});

Showing popups is async on non-Linux platforms, I think (and so I'm surprised this test passes as-is). That said, I guess if it works reliably on infra, I'll take it, esp. because I've also seen waiting for popups to show cause intermittents. :-\

::: browser/base/content/test/general/browser_panelUINotifications.js:68
(Diff revision 6)
> +      menuItem.click();
> +      ok(mainActionCalled, "Main action callback was called")
> +
> +      is(PanelUI.menuButton.getAttribute("badge-status"), "fxa-needs-authentication", "Fxa badge is shown on PanelUI button.");
> +    });
> +});

It would be nice if there were also tests for what happens if multiple (non-badge) notifications are added.

::: browser/components/customizableui/content/panelUI.inc.xul:422
(Diff revision 6)
> +       noautofocus="true"
> +       noautohide="true"

Pretty sure we don't want noautohide here? Or was that intentional?

::: browser/components/customizableui/content/panelUI.js:173
(Diff revision 6)
>          console.error("Error showing the PanelUI menu", reason);
>        });
>      });
>    },
>  
> +  showNotification: function(id, mainAction, secondaryActions, options) {

Can you default the options to `{}` and the `secondaryActions` to `[]` here, to avoid having to pass empty stuff from the callers? :-)

Also, here and below please use ES6 method syntax:

```js
showNotification(id, mainAction, secondaryActions = [], options = {})
```

::: browser/components/customizableui/content/panelUI.js:185
(Diff revision 6)
> +  showBadgeOnlyNotification: function(id) {
> +    return this.showNotification(id, null, null, { badgeOnly: true });
> +  },

Right now this means we can't show a badge without closing any open notifications, right? Is that intentional?

::: browser/components/customizableui/content/panelUI.js:191
(Diff revision 6)
> +    return this.showNotification(id, null, null, { badgeOnly: true });
> +  },
> +
> +  removeNotification: function(id) {
> +    let notifications;
> +    if (typeof(id) == "string") {

Nit: typeof is an operator, so you don't need braces.

::: browser/components/customizableui/content/panelUI.js:440
(Diff revision 6)
>  
> -      let iconAnchor =
> +      let anchor = this._getPanelAnchor(aAnchor);
> -        document.getAnonymousElementByAttribute(aAnchor, "class",
> -                                                "toolbarbutton-icon");
>  
> -      if (iconAnchor && aAnchor.id) {
> +      if ((aAnchor != anchor) && aAnchor.id) {

This doesn't need extra braces around the inner condition.

::: browser/components/customizableui/content/panelUI.js:720
(Diff revision 6)
> +    }
> +
> +    let dismiss = true;
> +    if (action) {
> +      try {
> +        action.callback.call(undefined);

Just action.callback() should do, I think? :-)

::: browser/components/customizableui/content/panelUI.js:783
(Diff revision 6)
>    } catch (ex) {
>      return "en-US";
>    }
>  }
> +
> +function Notification(id, mainAction, secondaryActions, options) {

We are rubbish at scoping so this is going to live in the global scope of browser windows, so naming it just `Notification` doesn't seem great. It shadows the builtin window.Notification (DOM) construtor, too. Can we make it more specific?

In more hip news, we do support ES6 class notation, so you could use that here if it helps at all.

::: browser/components/customizableui/content/panelUI.js:786
(Diff revision 6)
> +  this.secondaryActions = secondaryActions || [];
> +  this.options = options || {};

For these, please use defaults in the arguments instead of the `||`.

::: browser/components/customizableui/content/panelUI.js:792
(Diff revision 6)
> +  id: null,
> +  mainAction: null,
> +  secondaryActions: null,
> +  browser: null,
> +  owner: null,
> +  options: null,
> +  timeShown: null,
> +  dismissed: null,

Please remove this. The way prototype-based "inheritance" works, you're never actually changing these, you're always assigning to the instance which just shadows the properties on the prototype, which might as well be undefined (ie not present).

Some of these also seem to be unused?

::: browser/components/customizableui/content/panelUI.js:809
(Diff revision 6)
> +  while (parent && (parent = aElement.ownerDocument.getBindingParent(parent)))
> +    notificationEl = parent;

Nit: Please brace the loop.

::: browser/locales/en-US/chrome/browser/browser.dtd:888
(Diff revision 6)
>  <!ENTITY emeLearnMoreContextMenu.label            "Learn more about DRM…">
>  <!ENTITY emeLearnMoreContextMenu.accesskey        "D">
> +
> +<!ENTITY updateAvailable.message "Update your &brandShorterName; for the latest in speed and privacy.">
> +<!ENTITY updateAvailable.whatsnew.label "See what's new.">
> +<!ENTITY updateAvailable.whatsnew.href "http://www.mozilla.org/">

These links should not be in a locale file. We should probably use the URL formatter to factor in l10n bits, but they'll want to go to release notes or something, rather than hardcoded to mozilla.org (and even in that case, we should definitely be using https).

::: browser/locales/en-US/chrome/browser/browser.properties:636
(Diff revision 6)
>  appmenu.restartBrowserButton.label = Restart %S
> -appmenu.downloadUpdateButton.label = Download Update
> +appmenu.downloadUpdateButton.label = Download %S Update

Both of the button label strings are dead, AFAICT. Please remove them and update the localization note. Archaeology suggests this was a second patch in bug 1080406 that I wanted moved to another bug, which doesn't seem to ever have happened. :-(

::: browser/themes/shared/customizableui/panelUI.inc.css
(Diff revision 6)
>  #PanelUI-menu-button[badge-status] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    display: -moz-box;
>    height: 10px;
>    width: 10px;
>    background-size: contain;
> -  border: none;

I'm surprised to see this border: none disappear. Why are we doing that? Is it set elsewhere already (if so, where?)

::: browser/themes/shared/customizableui/panelUI.inc.css
(Diff revision 6)
>  }
>  
>  #PanelUI-menu-button[badge-status="download-success"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    display: none;
>  }
> -

Nit: please leave this blank line between the rules.

::: browser/themes/shared/customizableui/panelUI.inc.css:111
(Diff revision 6)
>  #PanelUI-menu-button[badge-status="update-succeeded"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    background: #74BF43 url(chrome://browser/skin/update-badge.svg) no-repeat center;
>    height: 13px;
>  }

This can be unified with the rule underneath, right?

::: browser/themes/shared/customizableui/panelUI.inc.css:132
(Diff revision 6)
>  
>  #PanelUI-menu-button[badge-status="download-warning"] > .toolbarbutton-badge-stack > .toolbarbutton-badge,
>  #PanelUI-menu-button[badge-status="fxa-needs-authentication"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    box-shadow: none;
>    filter: drop-shadow(0 1px 0 hsla(206, 50%, 10%, .15));
> +  border-radius: 2px;

Why the border-radius change here?

::: browser/themes/shared/customizableui/panelUI.inc.css:141
(Diff revision 6)
>  #PanelUI-menu-button[badge-status="download-warning"] > .toolbarbutton-badge-stack > .toolbarbutton-badge,
>  #PanelUI-menu-button[badge-status="download-severe"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    width: 7px;
>    height: 7px;
>    min-width: 0;
> -  border-radius: 50%;
> +  min-height: 0;

And this change?

::: browser/themes/shared/customizableui/panelUI.inc.css:449
(Diff revision 6)
>    background-color: var(--arrowpanel-dimmed);
>  }
>  
>  #PanelUI-multiView[viewtype="subview"] #PanelUI-mainView > #PanelUI-contents-scroller > #PanelUI-contents > .panel-wide-item,
>  #PanelUI-multiView[viewtype="subview"] #PanelUI-mainView > #PanelUI-contents-scroller > #PanelUI-contents > .toolbarbutton-1:not([panel-multiview-anchor="true"]),
> -#PanelUI-multiView[viewtype="subview"] #PanelUI-mainView > #PanelUI-footer > #PanelUI-update-status,
> +#PanelUI-multiView[viewtype="subview"] #PanelUI-mainView > #PanelUI-footer > #PanelUI-update-available-menu-item,

Here and in a number of other cases, the id selectors will just multiply happily, and we'd have to remember all the places where we need to do that if we add another notification with a menuitem.

Can we instead make the object that creates these menu-item elements add a class, and select for the class? That will reduce churn, simplify the rules, and provide warm fuzzies etc. :-)

Note that this will lower specificity. For the rule where I'm commenting, that likely won't matter because it's through the roof already (heck, given the descendant selector, why are we bothering selecting for mainView and footer - those elements never move!), but for other rules, you *may* (please check!) need to add a single child selector with the ID of the #PanelUI-footer to ensure you override other rules. In such cases, please add a comment.

::: browser/themes/shared/notification-icons.inc.css:43
(Diff revision 6)
>  %endif
>    }
>  }
>  
> -#notification-popup > .panel-arrowcontainer > .panel-arrowcontent {
> +#notification-popup > .panel-arrowcontainer > .panel-arrowcontent,
> +#PanelUI-notification-popup > .panel-arrowcontainer > .panel-arrowcontent {

This is sad (shouldn't really be in this file), but I don't see a better way - duplicating everything will lead to divergence, and I don't know of a better file to keep this in, off-hand.

::: browser/themes/shared/notification-icons.inc.css:349
(Diff revision 6)
> +.popup-notification-icon[popupid="update-available"],
> +.popup-notification-icon[popupid="update-manual"],
> +.popup-notification-icon[popupid="update-restart"] {
> +  background: #74BF43 url(chrome://browser/skin/notification-icons.svg#update) no-repeat center;
> +  border-radius: 50%;
> +}

I don't think this is used?

::: browser/themes/shared/notification-icons.svg:49
(Diff revision 6)
> +    #update-icon {
> +      stroke: #fff;
> +      stroke-width: 3px;
> +      stroke-linecap: round;
> +    }

I don't think we use this icon anywhere? These icons are for the location bar, and I don't think we'll have an icon there, so I think the change to this file can be reverted?

::: toolkit/themes/osx/global/toolbarbutton.css:84
(Diff revision 6)
>  .toolbarbutton-badge {
>    background-color: #d90000;
>    font-size: 9px;
>    padding: 1px 2px;
>    color: #fff;
> -  border-radius: 2px;
> +  border-radius: 50%;

I'm not going to review this file (and its x-platform friends) in detail, I hope Neil or Dão will if we keep it this way. I'll just say that this affects add-ons as well, so please test carefully. I suspect just cropping all the icons (because of the border-radius) add-ons provide is likely not OK. See bug 1188001 for some of the history. If we're only intending to modify the style of the badges we control, this might want to be living in more specific rules in browser/ CSS files instead.

Comment 39

[Romain Testard [:RT]]

> Regarding the UI, do we want this popup to autohide when the user clicks
> somewhere else in the screen, or do we want to require them to click "Not
> now"?

I think we want to persist the panel until the user clicks one of the buttons given how easy it is to dismiss it without noticing (I recall the same issue existed on WebRTC prompts). Bram what do you think? Again we should probably align practices with existing door hangers? (bug 1267604)

Also regarding "Download a fresh copy of Firefox" through the panel or the hamburger menu - the UX shows that the green arrow goes away as the user clicks and reaches the download page. This implies that if the user does not download or install Firefox the notification is not displayed anymore - can we change that so the arrow persists until the user installs a fresh copy?

Comment 40

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 893505 - clean up js and simplify css for PanelUI notifications

Review commit: https://reviewboard.mozilla.org/r/102302/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102302/

Comment 41

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8823803 [details]
Bug 893505 - clean up js and simplify css for PanelUI notifications

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102302/diff/1-2/

Comment 42

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8823803 [details]
Bug 893505 - clean up js and simplify css for PanelUI notifications

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102302/diff/2-3/

Comment 43

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8823803 [details]
Bug 893505 - clean up js and simplify css for PanelUI notifications

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102302/diff/3-4/

Comment 44

[Alex Thayer [:alexical] (she/her)]

> I haven't tested this patch - these are just code comments. I'll try to
> remember to look in more detail in the next round of reviews.
> 
> This generally looks pretty good already, so please don't be put off by the
> comments. :-)

Of course! Just happy to be getting my first review :)

> Note though that the current badging system is preffed off except on
> nightly/devedition. Do we want to use it everywhere? And if so do we want to
> remove / turn off the dialog stuff?

Yeah that was the intent. Details of that are still to be sorted out.

> Showing popups is async on non-Linux platforms, I think (and so I'm
> surprised this test passes as-is). That said, I guess if it works reliably
> on infra, I'll take it, esp. because I've also seen waiting for popups to
> show cause intermittents. :-\

Yeah - I think it works because it's just testing for the state "closed", and if the popup is in the process of showing it will have a state of "showing".

> ::: browser/components/customizableui/content/panelUI.inc.xul:422
> (Diff revision 6)
> > +       noautofocus="true"
> > +       noautohide="true"
> 
> Pretty sure we don't want noautohide here? Or was that intentional?

This is intentional - I can make this an option in the API for each notification rather than a universal property of the panel, but we want to make sure in our use case (per Romain's comment above) that we don't automatically hide the doorhanger when a user clicks outside of it.

> Also, here and below please use ES6 method syntax:

I was debating whether I should use ES6 and update the whole file to that syntax, or just do it for my own work, or stay consistent with the rest of the file. My new review updates my own work, but should I apply that change to the rest of the file as well?

> ::: browser/locales/en-US/chrome/browser/browser.dtd:888
> (Diff revision 6)
> >  <!ENTITY emeLearnMoreContextMenu.label            "Learn more about DRM…">
> >  <!ENTITY emeLearnMoreContextMenu.accesskey        "D">
> > +
> > +<!ENTITY updateAvailable.message "Update your &brandShorterName; for the latest in speed and privacy.">
> > +<!ENTITY updateAvailable.whatsnew.label "See what's new.">
> > +<!ENTITY updateAvailable.whatsnew.href "http://www.mozilla.org/">
> 
> These links should not be in a locale file. We should probably use the URL
> formatter to factor in l10n bits, but they'll want to go to release notes or
> something, rather than hardcoded to mozilla.org (and even in that case, we
> should definitely be using https).

Updated to use the same mechanism that the current about page uses (a preprocessor directive). Does that seem reasonable?

Comment 45

[Alex Thayer [:alexical] (she/her)]

@rstrong: How do we want to approach disabling the old update mechanism and enabling the new one? Do you want me to dive into that or is this awaiting work from you? I'm happy to dive into it, but I'm unfamiliar with how to simulate updates on my machine in order to produce that workflow from start to finish.

Comment 46

[:Gijs (he/him)]

Comment on attachment 8823803 [details]
Bug 893505 - clean up js and simplify css for PanelUI notifications

https://reviewboard.mozilla.org/r/102302/#review102896

I see I got confused about the icons, thanks for clarifying. The comment can probably be removed, I was just wrong, we do use those icons. Sorry!

This covers most of my comments. We'll need to sort out the URL, let me know (here or on IRC) if you need more pointers - https://dxr.mozilla.org/mozilla-central/source/browser/base/content/aboutDialog-appUpdater.js#53-56 is another example in the updater code that uses formatURLPref. We might even be able to reuse some of the existing URL prefs for this - Rob would know, I suspect.

Unlike github (I know, inconsistencies... they're annoying) on mozreview we generally fold/amend and repush the amended cset so we get multiple versions of the same commit, rather than "fixup" commits that are rolled together before landing. If you keep the weird moz commit id string in the commit description[0] of the commit you're changing (the first of the two commits on this review now), mozreview knows you're updating. Unlike with git, you won't need --force to repush or anything. Mozreview will re-request review on the commit. See https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/firefoxworkflow.html#changing-code-after-reviews for a description. So, please fold/"roll" these two commits (and any further changes) when you next push. Thanks!

[0] and I think even if you don't keep it, it can do the math in most cases, but there are edgecases with history rewriting where it'll lose track.

::: browser/base/content/browser.css:1147
(Diff revision 4)
>    -moz-box-flex: 1;
>    width: 10em;
>    max-width: 10em;
>  }
>  
> -#main-window[customizing=true] #PanelUI-update-available-menu-item,
> +#main-window[customizing=true] #PanelUI-notification-menu-item {

I guess this should be a class selector?

::: browser/components/customizableui/test/browser_panelUINotifications.js:1
(Diff revision 4)
> -add_task(function*() {
> -  yield BrowserTestUtils.withNewTab({ gBrowser, url: "about:blank"},
> -    function*(browser) {
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Hm, why did you add this? :-)

No license indicates it's licensed public domain, which is our default ( https://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/ ).

::: browser/components/customizableui/test/browser_panelUINotifications.js:254
(Diff revision 4)
> +    secondaryActionButton.click();
> +
> +    is(PanelUI.notificationPanel.state, "closed", "update-manual doorhanger is closed.");
> +    is(PanelUI.menuButton.getAttribute("badge-status"), "update-restart", "update-restart badge is displaying on PanelUI button.");
> +
> +    let menuItem

Nit: missing semicolon

::: browser/components/customizableui/content/panelUI.inc.xul:439
(Diff revision 4)
> +#ifndef NIGHTLY_BUILD
>        <label class="text-link" value="&updateAvailable.whatsnew.label;"
> -             href="&updateAvailable.whatsnew.href;"/>
> +#expand      href="https://www.mozilla.org/firefox/__MOZ_APP_VERSION__/releasenotes/"/>
> +#endif

Unfortunately, no, I don't think we should do this. We should add code that sets the .href of the label at runtime, probably based on a pref and formatURLPref. I realize that's going to be messy because of the XBL involved, which is unfortunate, but I think we may want the ability to configure this for e.g. partner builds or funnelcakes.

::: browser/components/customizableui/content/panelUI.js:452
(Diff revision 4)
>        CustomizableUI.addPanelCloseListeners(tempPanel);
>        tempPanel.addEventListener("popuphidden", panelRemover);
>  
>        let anchor = this._getPanelAnchor(aAnchor);
>  
> -      if ((aAnchor != anchor) && aAnchor.id) {
> +      if ((aAnchor != anchor) && aAnchor.id)

Sorry, I think I was unclear. I meant that operator precedence means that your if condition can be written as:

```js
if (aAnchor != anchor && aAnchor.id) {
```

(without the inner () )

and I don't think that's unclear. If you think it's easier to read, you could move the aAnchor.id check first (aAnchor.id && aAnchor != anchor) which would evaluate the same independent of operator precedence anyway. :-)

::: browser/components/customizableui/content/panelUI.js:679
(Diff revision 4)
>      let menuItemId = this._getMenuItemId(notification);
>      let menuItem = document.getElementById(menuItemId);
>      if (menuItem) {
>        menuItem.notification = notification;
>        menuItem.setAttribute("oncommand", "PanelUI._onNotificationMenuItemSelected(event)");
> +      menuItem.className += " PanelUI-notification-menu-item";

Yay for modern browsers!

```js
menuItem.classList.add("PanelUI-notification-menu-item");
```

:-)

::: browser/themes/shared/customizableui/panelUI.inc.css:101
(Diff revision 4)
>  
>  #nav-bar[brighttext] > #PanelUI-button {
>    border-image-source: linear-gradient(transparent, rgba(100%,100%,100%,.2) 20%, rgba(100%,100%,100%,.2) 80%, transparent);
>  }
>  
>  #PanelUI-menu-button[badge-status] > .toolbarbutton-badge-stack > .toolbarbutton-badge {

This block is still missing the border: none styling that was there earlier.

::: browser/themes/shared/customizableui/panelUI.inc.css
(Diff revision 4)
>  }
>  
>  #PanelUI-menu-button[badge-status="download-success"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    display: none;
>  }
> -#PanelUI-menu-button[badge-status="update-succeeded"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {

Shouldn't this selector be appended to the rule underneath it, or did this just go away entirely?

::: browser/themes/shared/customizableui/panelUI.inc.css:119
(Diff revision 4)
> -#PanelUI-menu-button[badge-status="update-failed"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> -  background: #D90000 url(chrome://browser/skin/update-badge-failed.svg) no-repeat center;
> +  margin: -9px 0 0 !important;
> +  margin-inline-end: -6px !important;

!important is generally frowned upon... can you clarify why it's necessary here, in a comment?

::: browser/themes/shared/customizableui/panelUI.inc.css:135
(Diff revision 4)
>  
>  #PanelUI-menu-button[badge-status="download-warning"] > .toolbarbutton-badge-stack > .toolbarbutton-badge,
>  #PanelUI-menu-button[badge-status="download-severe"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
>    width: 7px;
>    height: 7px;
>    min-width: 0;

This is still missing the border-radius: 50% that was removed in the previous cset, I think?

Comment 47

[Alex Thayer [:alexical] (she/her)]

Pushing updates now. I also made one change that wasn't requested in your reviews: I pulled out some of the update-specific css to use the id selectors again. Pretty much just the icons, since future notifications are unlikely to be using the same icons as us. Other than that it should all be expected stuff.


> This covers most of my comments. We'll need to sort out the URL, let me know
> (here or on IRC) if you need more pointers -
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/
> aboutDialog-appUpdater.js#53-56 is another example in the updater code that
> uses formatURLPref. We might even be able to reuse some of the existing URL
> prefs for this - Rob would know, I suspect.

Sounds good. There's already an app.releaseNotesURL pref which is used in mobile, so I just added it to desktop. For the Nightly release notes, we didn't have a design and :bram is on PTO for a while(?), so I just went with the nightly blog, because it seems to act as informal release notes. The other option would be to just not display the link, but I like having the consistency.
 
> Unlike github (I know, inconsistencies... they're annoying) on mozreview we
> generally fold/amend and repush the amended cset so we get multiple versions
> of the same commit, rather than "fixup" commits that are rolled together
> before landing.

Woops. That's my bad for reading that workflow doc too quickly, because I completely misread that section. Sorry about that.

> Unfortunately, no, I don't think we should do this. We should add code that
> sets the .href of the label at runtime, probably based on a pref and
> formatURLPref. I realize that's going to be messy because of the XBL
> involved, which is unfortunate, but I think we may want the ability to
> configure this for e.g. partner builds or funnelcakes.

I went with a solution specific to our use case, which was fairly simple and didn't require any XBL fiddling. Hopefully that seems reasonable? I'm not sure a more general solution would be used by anything else.

> Sorry, I think I was unclear. I meant that operator precedence means that
> your if condition can be written as:
> 
> ```js
> if (aAnchor != anchor && aAnchor.id) {
> ```
> 
> (without the inner () )
> 
> and I don't think that's unclear. If you think it's easier to read, you
> could move the aAnchor.id check first (aAnchor.id && aAnchor != anchor)
> which would evaluate the same independent of operator precedence anyway. :-)

Ah, yeah I'm fine with that. I don't mind relying on operator precedence in clearer cases like this. Just happy to be learning the conventions :)

> ```js
> menuItem.classList.add("PanelUI-notification-menu-item");
> ```
> 
> :-)

Oh gosh. Sane ways of doing things. I'll get there eventually, I promise.

> ::: browser/themes/shared/customizableui/panelUI.inc.css
> (Diff revision 4)
> >  }
> >  
> >  #PanelUI-menu-button[badge-status="download-success"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> >    display: none;
> >  }
> > -#PanelUI-menu-button[badge-status="update-succeeded"] > .toolbarbutton-badge-stack > .toolbarbutton-badge {
> 
> Shouldn't this selector be appended to the rule underneath it, or did this
> just go away entirely?

Sorry, I forgot to mention that these badges are dead with this change. They were only used by the gMenuButtonUpdateBadge, which I've rewritten to use the new badges/notifications.

Comment 48

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/6-7/

Comment 49

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/7-8/

Comment 50

[:Gijs (he/him)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review103556

With the below addressed, this looks pretty much OK to me. Probably a good idea for Rob and/or Neil to have a look, too, though.

One more thing besides the code things below: please update the commit msg of the single commit to only have the description of what the total commit does, and its funky mozreview commit id. So just:

```
Bug 893505 - Simplify the application update UI

MozReview-Commit-ID: 24sESMTosNX
```

would do.

Thanks for working on this and all the work that went into the test!

::: browser/base/content/browser.js:2672
(Diff revisions 6 - 8)
>        let action = {
> -        callback: () => { openManualUpdateUrl(); }
> +        callback: () => { gMenuButtonUpdateBadge.openManualUpdateUrl(); }
>        };
>  
> -      PanelUI.showNotification("update-manual", action, [], {});
> +      let releaseNotesURL = Services.urlFormatter.formatURLPref("app.releaseNotesURL");
> +      document.getElementById("update-manual-whats-new").href = releaseNotesURL;

This only affects the manual one, not the "update-available" one. Is that intentional?

::: browser/app/profile/firefox.js:82
(Diff revision 8)
>  pref("browser.dictionaries.download.url", "https://addons.mozilla.org/%LOCALE%/firefox/dictionaries/");
>  
> +// When we display a doorhanger to update firefox, there will be a "What's new"
> +// link which will point to this URL.
> +#ifdef RELEASE_OR_BETA
> +pref("app.releaseNotesURL", "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/releasenotes/");

This will make this link appear on the page that shows up if you load "about:" in the browser. Probably not a bad thing, but just an FYI (we don't expose a link to "about:" anywhere, I think).

::: browser/app/profile/firefox.js:84
(Diff revision 8)
> +// When we display a doorhanger to update firefox, there will be a "What's new"
> +// link which will point to this URL.
> +#ifdef RELEASE_OR_BETA
> +pref("app.releaseNotesURL", "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/releasenotes/");
> +#else
> +#ifdef MOZ_DEV_EDITION

Use NIGHTLY_BUILD (maybe with ifndef, or swap the if/else), because MOZ_DEV_EDITION is a branding thing rather than a channel thing. It's confusing, I know! Basically, aurora used to just be an alpha branch, and then we started marketing it as devedition...

Comment 51

[Alex Thayer [:alexical] (she/her)]

> Thanks for working on this and all the work that went into the test!

Of course. Thank you for your time on this, Gijs! :) I really appreciate it.

> ::: browser/base/content/browser.js:2672
> (Diff revisions 6 - 8)
> >        let action = {
> > -        callback: () => { openManualUpdateUrl(); }
> > +        callback: () => { gMenuButtonUpdateBadge.openManualUpdateUrl(); }
> >        };
> >  
> > -      PanelUI.showNotification("update-manual", action, [], {});
> > +      let releaseNotesURL = Services.urlFormatter.formatURLPref("app.releaseNotesURL");
> > +      document.getElementById("update-manual-whats-new").href = releaseNotesURL;
> 
> This only affects the manual one, not the "update-available" one. Is that
> intentional?

It is. There's still no place where we display the update-available notification. I had a design question about that one up above, since it's not altogether clear to me how that notification fits into the whole picture. If there's a clear place where it should be triggered then we'll do the same thing with it, but I was waiting on rstrong for that one.

> This will make this link appear on the page that shows up if you load
> "about:" in the browser. Probably not a bad thing, but just an FYI (we don't
> expose a link to "about:" anywhere, I think).

Yeah I saw that guy - I couldn't find any links to it so I just left it, since it seemed like it made sense to have the release notes there anyway.

> Use NIGHTLY_BUILD (maybe with ifndef, or swap the if/else), because
> MOZ_DEV_EDITION is a branding thing rather than a channel thing. It's
> confusing, I know! Basically, aurora used to just be an alpha branch, and
> then we started marketing it as devedition...

Ah, weird. Alright, I'll make sure to get that in! Thanks so much again!

Comment 52

[Alex Thayer [:alexical] (she/her)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #31)
> (In reply to Doug Thayer [:dthayer] from comment #30)
> > I'm just realizing this, sorry it's a bit late, but there's an infinite UI
> > loop in this diagram if downloads consistently fail:
> > https://pageshot.net/osmBeugOW0Mhmh32/www.lucidchart.com. What do we want to
> > do about this? I also don't really understand the user flow in this loop. If
> > the download failed, what reason do we have to believe that the user
> > manually clicking something to trigger the same download is going to work
> > any better? I understand if we were directing them to a manual download
> > page, like in the "Download a fresh copy" flow, but in this case we're just
> > re-triggering what we tried already. It seems like we should automatically
> > retry, and if we fail enough times, then show them the "Download a fresh
> > copy" flow. In this case, we don't need the "Update available" notification
> > at all.
> If all mar downloads fail it should be the Download a fresh copy UI. Note:
> some updates have both a partial and complete mar and others will only have
> a complete mar.

I'm still not certain when we want to show the Update Firefox tour from this. If all mar downloads fail, then we go to Download a Fresh Copy, but if only some of them fail, do we want to show anything at all? It seems like we shouldn't prompt the user for anything until we've exhausted all options, and so I don't see where the Update Firefox tour fits in.

Comment 53

[Bram Pitoyo [:bram]]

Answering Doug’s questions:

> The "Restyle doorhanger notifications" bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1267604) seems to overlap with this a bit, in that we're using those designs for the doorhangers for this. However, they don't _quite_ match up with the design you linked at https://mozilla.invisionapp.com/share/Y776FIBWS#/screens/155743946.

> Another way the design doesn't match up is that the top label ("Firefox can't update to the latest version"), isn't any larger than the bottom description ("Download a fresh copy..."). Again, if we were to align this with your design, we'd probably want to align all doorhangers. Should I also make the change to do that?

We should try to match and reuse the restyled doorhanger as much as we can instead of designing/tweaking our own. If my styles don’t match, let’s change them to fit. It means more UI consistency across Firefox.


> Should we display the green arrow _before_ the doorhanger (maybe after a few days of not restarting rather than a whole week)? I wasn't totally sure of this based on the design. It might be nice because they have the opportunity to voluntarily engage with it and not have to be disrupted by the doorhanger when they didn't realize they even needed to restart, but it also could be worse because the ideal scenario is they never had to notice anything at all.

The reason why I’ve put the green arrow so long after not restarting was because we know that most users close their browsers every day (so the update will be installed the next day). In other words, most people will never see this green arrow, which is great, because the arrow is supposed to only be shown when something important needs your attention. That is, if we show a notification too early or too many times, we may cause attention fatigue and the user may not pay as much attention to other life-threatening notifications (like password inputted on HTTP sites).


Answering Romain’s questions:

> I think we want to persist the panel until the user clicks one of the buttons given how easy it is to dismiss it without noticing (I recall the same issue existed on WebRTC prompts). Bram what do you think? Again we should probably align practices with existing door hangers? (bug 1267604)

Yes. The new doorhanger dialogues was designed to persist until clicked and not so easily dismissed like before. We should follow this behaviour.


> Also regarding "Download a fresh copy of Firefox" through the panel or the hamburger menu - the UX shows that the green arrow goes away as the user clicks and reaches the download page. This implies that if the user does not download or install Firefox the notification is not displayed anymore - can we change that so the arrow persists until the user installs a fresh copy?

We can do this, but I think the message should be tweaked a little:

IF this is the first time you see the dialogue, THEN print “Download a fresh copy of Firefox”.

IF you have clicked the “Download fresh copy” button, THEN keep the green arrow showing, THEN when you open the hamburger menu, print something like “Install the Firefox that you have just downloaded, or click here to re-download a fresh copy of Firefox”.


I think we should rewrite the second message because it’s too wordy and a bit confusing, but the idea is there. What do you think?

Comment 54

[Alex Thayer [:alexical] (she/her)]

> The reason why I’ve put the green arrow so long after not restarting was
> because we know that most users close their browsers every day (so the
> update will be installed the next day). In other words, most people will
> never see this green arrow, which is great, because the arrow is supposed to
> only be shown when something important needs your attention. That is, if we
> show a notification too early or too many times, we may cause attention
> fatigue and the user may not pay as much attention to other life-threatening
> notifications (like password inputted on HTTP sites).

I totally understand that - I wasn't so much trying to say that we should show the arrow *earlier*, just that it might be a good idea to show it *before* the doorhanger, since it's less intrusive than the doorhanger. This way they have an opportunity to interact with the notification before it pops into their screen space demanding immediate attention. For instance, we could show the green arrow at 7 days, and then pop up the doorhanger on day 8.

> > Also regarding "Download a fresh copy of Firefox" through the panel or the hamburger menu - the UX shows that the green arrow goes away as the user clicks and reaches the download page. This implies that if the user does not download or install Firefox the notification is not displayed anymore - can we change that so the arrow persists until the user installs a fresh copy?
> 
> We can do this, but I think the message should be tweaked a little:
> 
> IF this is the first time you see the dialogue, THEN print “Download a fresh
> copy of Firefox”.
> 
> IF you have clicked the “Download fresh copy” button, THEN keep the green
> arrow showing, THEN when you open the hamburger menu, print something like
> “Install the Firefox that you have just downloaded, or click here to
> re-download a fresh copy of Firefox”.
> 
> 
> I think we should rewrite the second message because it’s too wordy and a
> bit confusing, but the idea is there. What do you think?

In theory we should be able to look at their downloads and see that the fresh copy is still fresh and is valid. If that's the case, giving them the a button to keep downloading fresh copies of Firefox ad infinitum seems like we're ignoring that the user doesn't know what to do with the thing they just downloaded. It might be better to just give them a button which simply runs the installer that they downloaded from the previous step. Thoughts?

Comment 55

[Bram Pitoyo [:bram]]

(In reply to Doug Thayer [:dthayer] from comment #54)
> […] it might be a good idea to show it [the green arrow] *before* the doorhanger, since it's less intrusive […]
> For instance, we could show the green arrow at 7 days, and then pop up the doorhanger on day 8.

Showing something less intrusive before popping something in your face is an interesting idea. It would be the equivalent of an unread message label on your home screen icon.

Here’s a proposal: if we know that most of our users restarts Firefox every day, why not show the green arrow at day 2 instead of day 7? I don’t have a good sense of exactly when to show the arrow, but in principle, I want most users not to see or have to worry about it. So this may help determine our day counter.


> In theory we should be able to look at their downloads and see that the fresh copy is still fresh and is valid.

I designed the flow to not be so smart or involve things like file detection. That way, it’s easy to implement, test and refine. If it’s easy to do this, I would love for us to be able to say “Hey, you have downloaded Firefox successfully. Click here to run the installer!”.

Comment 56

[Robert Strong (they/them - no direct email)]

I suspect that showing the green arrow earlier wouldn't hurt but I don't think it will help very much either since most users restart much sooner. I am fairly confident this is a very small number of users but I'm not against it. If this is added it should be based off of the prompt to restart delay since that can be controlled from the server so if there is ever a major issue we can ask users to restart prior to the 7 day default.

One thing that might be nice is to only show the popup when there has been some user interaction. That way it wouldn't show up on kiosks, etc around offices unless someone interacted with the system and this person could then perform the restart.

Comment 58

[Robert Strong (they/them - no direct email)]

(In reply to Doug Thayer [:dthayer] from comment #45)
> @rstrong: How do we want to approach disabling the old update mechanism and
> enabling the new one? Do you want me to dive into that or is this awaiting
> work from you? I'm happy to dive into it, but I'm unfamiliar with how to
> simulate updates on my machine in order to produce that workflow from start
> to finish.
Doug, this is difficult to do manually and I planned on doing that part. If you have time then perhaps the existing tests that simulate updates can help guide you?
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/chrome

Comment 59

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/8-9/

Comment 60

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/9-10/

Comment 61

[Alex Thayer [:alexical] (she/her)]

I just pushed a change which does a few things:

- Heuristically detects if they've downloaded the installer after being prompted to do so, and raises a different message in that case.

- Only shows popups if there is no full screen element (like a video).

Watching downloads for things that look like our installer wasn't too difficult, but the strategy isn't bullet proof. Basically I just looked for the first download that came from the same domain that we directed them toward (minus the subdomain. I.e., if we send them to https://nightly.mozilla.org, and they get redirected to https://www.mozilla.org/en-US/firefox/channel/desktop/?v=b#nightly, then we'll still survive, because it will just match on the "mozilla.org" part. Unfortunately I'm not sure of a better way, but advice is welcome. We can of course scrap this strategy entirely, but I think in 99% of cases it will work just fine.

Additionally, after experimenting with the workflow of downloading a fresh copy for a little bit I landed on the following:

- Direct them to the fresh installer page. On nightly, that is https://nightly.mozilla.org
- Immediately remove the badge once they've been directed there, since it felt confusing that the badge was still there after I interacted with the popup.
- If the user hasn't downloaded the installer within a minute, then bring the badge back, because the user isn't done, and likely got sidetracked
- Once the installer is downloading, show a badge which will give them an option to launch the installer (or launch it once it's done downloading)
- If the user has not launched the installer within five minutes, escalate the badge into a popup.

Does that all seem reasonable?

Another point regarding the "Download a fresh copy" workflow, is that I'm realizing the "See what's new" link will be a bit difficult, since I'm not sure we have enough information to direct them to what's new. The release notes URL requires a version number of the version we're upgrading to, which we may not have depending on where things broke down. I assume the best course of action is to just only display that link if we know what version we're directing them to download.

Regarding full-screen mode, I realized that this popup would be annoying if it showed up while I was watching a video, so I just opted to not display these popups if there's no full-screen element. I was debating whether to hide the popup if we're in full-screen mode at all (i.e., via F11). I don't know what the most common use of full-screen mode is, but I know people use it for things like self-sign-in kiosks, where for instance customers might come and sign themselves into a class or something. If we had the ability to not show the doorhanger in these cases, that would be great, since those customers would not want to upgrade Firefox for the business they're visiting, but I'm not sure how much we care about that or if it's worth it. Just a thought.

Lastly, Gijs mentioned after his r+ that it would still be a good idea to get a review from you or neil, rstrong, so I just thought I'd poke you about this. After my most recent changes there are a few TODO's left over, which are noted in the code, but I think it's close to being in a good state to land. In any case, once it is, and after I run the front-end tests in try, what's the process for getting it landed? Does the reviewer take care of that?

Comment 62

[Alex Thayer [:alexical] (she/her)]

One last thing:

> Doug, this is difficult to do manually and I planned on doing that part.
> If you have time then perhaps the existing tests that simulate updates
> can help guide you?
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/chrome

I'm looking into it now because I don't have much else on my plate. If there are things I can't overcome or too many questions I can't answer myself I'll let you know, but from what I can see there should be enough for me to figure it out.

Comment 63

[Bram Pitoyo [:bram]]

So far, our strategy has been to open a Firefox download page in a new tab and have the user click the download button. This is simple for us to do, but risky for the user:

* User might accidentally close the tab
* User might get distracted and forgot what he originally came there to do, navigating away from the tab
* User might not be able to find the download button (this is probably an artificial risk, as we’ve designed the page to have prominent buttons)
* User might not click “Save” or “OK” on the download prompt

Now that we’re experimenting with a few behaviours, I wonder if it would be possible to directly trigger the installer download (even better if the user won’t have to click “Save” or “OK”) so there would be very little chance of failure?

Comment 64

[Alex Thayer [:alexical] (she/her)]

(In reply to Bram Pitoyo [:bram] from comment #63)
> Now that we’re experimenting with a few behaviours, I wonder if it would be
> possible to directly trigger the installer download (even better if the user
> won’t have to click “Save” or “OK”) so there would be very little chance of
> failure?

I like the idea, but I have a few concerns:

* If the reason we're in the "Download a fresh copy" workflow is because update checks failed repeatedly, then I think the chances are high that automatically downloading a fresh copy is going to fail as well. One benefit of opening a download page in a new tab is we get to piggy back on existing error handling and troubleshooting documentation to help people figure out why the download is failing. Because of this, if we did automatically download the fresh copy and run it, we'd probably just end up failing and falling back to opening up a new tab anyway.

* We'd have to make it clear that we're going to launch an application for the user, and the user will have to initiate that, because (speculating) it might be disconcerting to the user to have an executable run without their direct consent.

* Automatically downloading the installer might require work in different places than you'd expect. For instance, if you navigate to https://nightly.mozilla.org, the "Download" button links to https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/firefox-53.0a1.en-US.win32.installer-stub.exe. Now, we can easily initiate a download and launch the resulting executable if we know what that URL is beforehand, but that would involve knowing the version that we are trying to download, which isn't necessarily available to us if the update checks failed, since it could be a major version bump or a minor version bump, and if we can't contact the server then we wouldn't know.

* If there are little hiccups like a user's antivirus not allowing the user to run the exe, then following the typical workflow that a user is used to might help them troubleshoot. Once we've hit this point, it's likely that something unexpected is going on on the user's machine, and the more we do automatically, the less the user understands, and the less likely it is that they'll be able to help us out.


These aren't show stoppers, but I wanted to make sure to surface them. Sorry for starting us down this rabbit hole, I know this might be overthinking something that we're not expecting to hit very frequently at all anyway.

Comment 65

[Bram Pitoyo [:bram]]

(In reply to Doug Thayer [:dthayer] from comment #64)
> (In reply to Bram Pitoyo [:bram] from comment #63)
> […] One benefit
> of opening a download page in a new tab is we get to piggy back on existing
> error handling and troubleshooting documentation to help people figure out
> why the download is failing.
> 
> […] it
> might be disconcerting to the user to have an executable run without their
> direct consent.
> 
> […] the more we do
> automatically, the less the user understands, and the less likely it is that
> they'll be able to help us out.

All these reasons make sense, and I agree that the positives (giving users a chance to troubleshoot when all automatic processes had failed) outweigh the negatives (users need to manually find the download button, click it, then click “Save”).

Let’s go with your initial proposal, then: open download page on a new tab, then focus on that tab. If user hasn’t downloaded the file or run the executable, we’ll escalate the doorhanger message into something stronger.

Comment 66

[:Gijs (he/him)]

(In reply to Doug Thayer [:dthayer] from comment #61)
> I just pushed a change which does a few things:

Do I need to review this again?


> - Heuristically detects if they've downloaded the installer after being
> prompted to do so, and raises a different message in that case.
> 
> - Only shows popups if there is no full screen element (like a video).
> 
> Watching downloads for things that look like our installer wasn't too
> difficult, but the strategy isn't bullet proof. Basically I just looked for
> the first download that came from the same domain that we directed them
> toward (minus the subdomain. I.e., if we send them to
> https://nightly.mozilla.org, and they get redirected to
> https://www.mozilla.org/en-US/firefox/channel/desktop/?v=b#nightly, then
> we'll still survive, because it will just match on the "mozilla.org" part.
> Unfortunately I'm not sure of a better way, but advice is welcome. We can of
> course scrap this strategy entirely, but I think in 99% of cases it will
> work just fine.

It looks like you're doing some manual string splitting. We have the eTLD service to do this correctly for funny things like foo.co.uk and the like.

> <snip>

> Regarding full-screen mode, I realized that this popup would be annoying if
> it showed up while I was watching a video, so I just opted to not display
> these popups if there's no full-screen element. I was debating whether to
> hide the popup if we're in full-screen mode at all (i.e., via F11). I don't
> know what the most common use of full-screen mode is, but I know people use
> it for things like self-sign-in kiosks, where for instance customers might
> come and sign themselves into a class or something. If we had the ability to
> not show the doorhanger in these cases, that would be great, since those
> customers would not want to upgrade Firefox for the business they're
> visiting, but I'm not sure how much we care about that or if it's worth it.
> Just a thought.

I think this is a good idea. There are issues with what happens to popups generally if the anchor disappears (off-screen or otherwise), so I don't think it would work well when the toolbars are hidden (which we don't do on OS X but you should be able to repro on Windows or Linux). I forget how we deal with this for the URL-bar based notifications; :florian or :past might know.

Comment 67

[Alex Thayer [:alexical] (she/her)]

(In reply to :Gijs from comment #66)
> I think this is a good idea. There are issues with what happens to popups
> generally if the anchor disappears (off-screen or otherwise), so I don't
> think it would work well when the toolbars are hidden (which we don't do on
> OS X but you should be able to repro on Windows or Linux). I forget how we
> deal with this for the URL-bar based notifications; :florian or :past might
> know.

I tested it on Windows and it seems to work as you would hope: it's anchored to the top of the screen and sits where it normally would horizontally. I'm still on the fence about disabling it - since while self-sign-in kiosks are troubling, another use case for full-screen mode would be a kiosk operated by an employee. I imagine these kinds of kiosks are prime examples of Firefox instances which never get restarted and thus never receive updates, which I think would mean we want to make sure we reach them. The badges of course won't work - we could however hover a badge in the top right of the screen while in full-screen mode if we wanted to. Just a thought.

Regarding the eTLD service, thank you! I was looking for something like that but was having difficulty figuring out what to search for.

Comment 68

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/10-11/

Comment 69

[Alex Thayer [:alexical] (she/her)]

Talked about this with Gijs on IRC. If you enter full-screen mode while a doorhanger is showing, it doesn't behave very well. You can observe the behavior by going to https://addons.mozilla.org, downloading an add-on, and then hitting F11. Though the bug is present with add-ons, I think it's simpler to just side-step it in our case and not show the notification.

I pushed these changes to reviewboard, along with using the eTLD service instead of manual string splitting.

There's still more to come though, since I'm working through the process of tying this to the back-end.

Comment 70

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/11-12/

Comment 71

[Alex Thayer [:alexical] (she/her)]

Just pushed the patch with the back-end wired up. I decided to write new tests for this adapted from the old tests, rather than just modifying the old tests for a few reasons:

- The old tests were testing in some cases substantially different workflows in which they, for instance, have a download progress page they're waiting for.
- These changes currently only targeted at Firefox. I don't know how much would be required to get it to work with Thunderbird, etc. because it's fairly tied up with, for instance, panelUI.js, which is under browser. Accordingly, preserving the old application update code for now, and thus the corresponding tests, seems like the simplest solution.
- I wanted to get my hands dirty with some of the testing and update code, and the best way to do that seemed to be to rewrite some of the framework and see what the tests really required.

I'm open to reverting this decision though, I just wanted to run with it for now.

Additionally, it was a bit tricky deciding exactly where to add events off from nsUpdateService.js, and where it should exit early if we're using the new UI, so I would appreciate any feedback dealing with that.

rstrong: I believe this is ready for you to fully review, so when you have some time I would greatly appreciate it.

Gijs: If you have the time and think it's warranted I also would appreciate further review from you, since it was tremendously helpful last time :)

Comment 72

[Bram Pitoyo [:bram]]

(In reply to Doug Thayer [:dthayer] from comment #71)

Additionally, if you’ve got any screenshot of the process that you’re able to share, I’d love to see them and do some UI review!

Comment 73

[Johann Hofmann [:johannh]]

For screenshots please consider creating a mozscreenshots[0] configuration for this. Makes cross platform UI review much easier and allows us to catch visual regressions that would otherwise go unnoticed.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots

(You can do that in a separate bug)

Comment 74

[Alex Thayer [:alexical] (she/her)]

(In reply to Johann Hofmann [:johannh] from comment #73)
> For screenshots please consider creating a mozscreenshots[0] configuration
> for this. Makes cross platform UI review much easier and allows us to catch
> visual regressions that would otherwise go unnoticed.
> 
> [0] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Browser_screenshots
> 
> (You can do that in a separate bug)

Ah okay - yeah I'll be sure to file a bug and do that. For now while it's in development I assume manual screenshots are okay?

Assuming so, then bram, here are those screenshots:

Badge - https://cl.ly/1g2D0v1j1B38

Available doorhanger - https://cl.ly/0F2q1z2l251I
Available menu item - https://cl.ly/0p2c37442X1s

Restart doorhanger - https://cl.ly/093N33042X07
Restart menu item - https://cl.ly/400z452R0520

Download doorhanger - https://cl.ly/1d3n2G0l0j0i
Download menu item - https://cl.ly/04001d2A2r0V

Launch doorhanger (probably needs better text) - https://cl.ly/0d1C033J3O3F
Launch menu item (probably needs better text) - https://cl.ly/2S44221m3U47

I can also grab Windows screenshots if you like. They're not significantly different other than Windows having sharp corners and the blue button being on the left.

Additionally, just calling it out that these are without any custom styles for our doorhangers, as mentioned above. Without custom styles the whitespace around the doorhanger icon isn't balanced, there's a little bit too much space below the text, and the header is the same size as the description.

Comment 75

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/12-13/

Comment 76

[:Gijs (he/him)]

(In reply to Doug Thayer [:dthayer] from comment #71)
> rstrong: I believe this is ready for you to fully review, so when you have
> some time I would greatly appreciate it.
> 
> Gijs: If you have the time and think it's warranted I also would appreciate
> further review from you, since it was tremendously helpful last time :)

I think for this bit, rstrong's review is likely to be more helpful. I'll wait for him to review this first.

Comment 77

[Alex Thayer [:alexical] (she/her)]

(In reply to :Gijs from comment #76)
> I think for this bit, rstrong's review is likely to be more helpful. I'll
> wait for him to review this first.

Sounds good!

@bram, I forgot, I never heard back on an earlier question I had - I didn't really explicitly address it to you though, so I think it just got overlooked. There's an infinite loop in the UI if downloads are failing. It seems to me that if all of our downloads fail (the partial download and the complete download), we should simply prompt them to download a fresh copy of the installer, for the reasons mentioned above about troubleshooting/etc. This is currently what's in my patch, but we could also do something similar to how update checks work (i.e.: only prompt the user after 10 failed downloads.) I'm also open to keeping the current design, I just didn't quite understand the history of it and whether that infinite UI loop was an intended property.

I'm still using the "Update Available" workflow in cases where Balrog's response has showPrompt="true" set (this path isn't in the user flow diagram - do we actively use this property for real updates?), but other than that it would go away.

Anyway, I'm just working on doing what I can to get everything all set for QA (https://wiki.mozilla.org/QA/Application_Update_UI), and just wanted to make sure everything matched up with your docs and that they are up-to-date :)

Comment 78

[Alex Thayer [:alexical] (she/her)]

Also @rstrong, a few questions about the doc @ (https://wiki.mozilla.org/QA/Application_Update_UI):

Risk Analysis:

- To mitigate the risks of this, I think a good strategy is to make sure that we can revert to the old system by just flipping a pref; does that sound reasonable? I believe the code should already be set up such that this is true, but if you agree with me then I can add automated tests to validate this.

- The doc mentions "We currently have a problem where incompatible addons can disable updates". Do you know where the bug is for this, and how it relates to this work? This is the first I'm hearing of it.


Scope:

- We talked about how this stuff is pretty tricky to manually test, so I just wanted to get your take on how QA should be manually testing this, since that's mentioned in the doc? Should they just be installing older versions of Nightly and tweaking their prefs to make the timings easier for manual validation?

- Specifically in regards to testing how the system reacts to various failures, what is the scope of the failures they should be testing? There are some easier things for them to test, like how the system behaves when you're without internet, or don't have sufficient permissions, etc., but what about more exotic failures like malformed XML from Balrog, or a failed signature validation? Are those out of scope?


There's an email thread

Comment 79

[Bram Pitoyo [:bram]]

(In reply to Doug Thayer [:dthayer] from comment #77)
> […] There's an infinite loop in the UI if downloads are failing. It
> seems to me that if all of our downloads fail (the partial download and the
> complete download), we should simply prompt them to download a fresh copy of
> the installer […] but we could also do something similar
> to how update checks work (i.e.: only prompt the user after 10 failed
> downloads.)

Sorry I’ve missed answering this question from over a month ago!

Thanks for pointing out the infinite loop in the flow.

My original thought: as long as your internet connection is active, re-downloading the update would most certainly fix any issue you have with downloading the MAR file and lead to success.

But you pointed out a good concern: if the download failed repeatedly for any reason, the user would have to keep clicking “Update Firefox” over and over again without any resolution.

So I’m updating the flow as follows:

* The first and second time you download an update, you’ll go through the standard flow
* The third time the update failed to download, show the “Download a fresh copy” dialogue

I thought that one click is worth retrying, but two clicks are enough to escalate it to the next level.

What do you think?

Comment 80

[Bram Pitoyo [:bram]]

(In reply to Doug Thayer [:dthayer] from comment #74)

> Badge - https://cl.ly/1g2D0v1j1B38

Looks good.

> Available doorhanger - https://cl.ly/0F2q1z2l251I
> Restart doorhanger - https://cl.ly/093N33042X07
> Download doorhanger - https://cl.ly/1d3n2G0l0j0i
>
> Additionally, just calling it out that these are without any custom styles
> for our doorhangers […]

Looks good. The text sizes I had used for the heading and description didn’t conform to the standard doorhanger size, so it’s good that we follow that standard here.

At this point, I’m more worried about the behaviour rather than the style, but thanks a lot for bringing up the style issue. At some point we’ll have to choose whether this notification should look closer to other notifications or to the main Control Center UI. But for now, let’s stick with our current approach.


> Available menu item - https://cl.ly/0p2c37442X1s
> Restart menu item - https://cl.ly/400z452R0520
> Download menu item - https://cl.ly/04001d2A2r0V

Looks good.


> Launch doorhanger (probably needs better text) - https://cl.ly/0d1C033J3O3F

Heading
Firefox is ready to install.

Description
Restart Firefox to install it. When it’s finished, Firefox will relaunch and restore all your open tabs and windows.

[Not Now] [Install and Restore]


> Launch menu item (probably needs better text) - https://cl.ly/2S44221m3U47

Description
Restart and install Firefox


One thing to add: in both these launch scenarios, I assume that we can launch the .exe or .msi on Windows, and open the .dmg file on Mac. Can we confirm this?

I’m not entirely clear what will happen on Linux. Maybe we don’t need to tackle this one, because the Package Manager will take care of the update?

Comment 81

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Doug Thayer [:dthayer] from comment #78)
> - We talked about how this stuff is pretty tricky to manually test, so I
> just wanted to get your take on how QA should be manually testing this,
> since that's mentioned in the doc? Should they just be installing older
> versions of Nightly and tweaking their prefs to make the timings easier for
> manual validation?

Updates are usually not manually tested by QA but our firefox-ui-update tests are used. Those you can run via "./mach firefox-ui-update --binary %path/to/old/firefox%", to let an update test from an older release to a current one. It also offers a couple of options you can see with --help. 

It means when the changes on this bug become active we will also have to update those tests, so that they can step through the new UI correctly, and automatically fallback to the old UI for former Firefox releases.

Comment 82

[Alex Thayer [:alexical] (she/her)]

https://www.mozilla.org/en-US/firefox/all/(In reply to Bram Pitoyo [:bram] from comment #79)
> So I’m updating the flow as follows:
> 
> * The first and second time you download an update, you’ll go through the
> standard flow
> * The third time the update failed to download, show the “Download a fresh
> copy” dialogue
> 
> I thought that one click is worth retrying, but two clicks are enough to
> escalate it to the next level.
> 
> What do you think?

Sounds good! Do we want to have any layers of automatic retrying of the downloads though? Seems like we might want to at least give that a shot after some delay before prompting the user.

(In reply to Bram Pitoyo [:bram] from comment #80)
> > Launch doorhanger (probably needs better text) - https://cl.ly/0d1C033J3O3F
> 
> Heading
> Firefox is ready to install.
> 
> Description
> Restart Firefox to install it. When it’s finished, Firefox will relaunch and
> restore all your open tabs and windows.
> 
> [Not Now] [Install and Restore]
> 
> 
> > Launch menu item (probably needs better text) - https://cl.ly/2S44221m3U47
> 
> Description
> Restart and install Firefox

I'm not sure it's going to be so simple to deliver on this text. On OSX, we can launch the .dmg, but we can't restart when they're done or do anything fancy like that. In fact, they can't install via the .dmg until we've shut Firefox down, so the best we can do is launch the .dmg and then immediately shut Firefox down, and just wait for them to launch the new install. I think we can restore their session in OSX, but it might take a bit of fiddling to do so automatically (if we were to trigger a restart, we'd get it for free, but I'm not 100% sure how much work it would take to automatically restore their session via a close/manual open.)

On Windows, we have a couple of options. If we use the stub installer that you'd find at https://www.mozilla.org/en-US/firefox/new/, then once they're all the way through the installer, they'll see a message that reads like this:


> Firefox is already running.
> 
> Please close Firefox prior to launching the version you have just installed.
> 
> [OK]

Our options for this are:

- Just accept that this message is there and the user has to click OK, and then restart Firefox once the installer closes, which will by default preserve their session.
- Sidestep this issue by directing them to download the full installer (located here: https://www.mozilla.org/en-US/firefox/all/) instead of the stub installer. This page might be too complicated for a regular user to understand and navigate.
- Close Firefox right after launching the installer - this way, they won't see the "Firefox is already running" message, but if they exit out of the installer early or anything, we won't automatically launch Firefox for them. This also has the same problem as the OSX side where I'll need to figure out what it will take the restore the session.


> One thing to add: in both these launch scenarios, I assume that we can
> launch the .exe or .msi on Windows, and open the .dmg file on Mac. Can we
> confirm this?
> 
> I’m not entirely clear what will happen on Linux. Maybe we don’t need to
> tackle this one, because the Package Manager will take care of the update?

And yes, with the caveats I mention above, we can launch the .exe and we can open the .dmg file. Linux is a good question - I haven't fully explored this on any Linux distros.

Comment 83

[Alex Thayer [:alexical] (she/her)]

Update: saving the session to be restored without requiring a restart turned out to be fairly simple, so disregard my previous comments about it potentially requiring work.

Should be as simple as:

```
      let didSave = Cc["@mozilla.org/supports-PRBool;1"]
                    .createInstance(Ci.nsISupportsPRBool);
      Services.obs.notifyObservers(didSave, "session-save", null);

      if (!didSave.data) {
        Cu.reportError("Tried to save session before restart for update and failed.");
      }

      // ... now we can quit the application

```

Comment 84

[Bram Pitoyo [:bram]]

(In reply to Doug Thayer [:dthayer] from comment #82)
> […] Do we want to have any layers of automatic retrying of the
> downloads though? Seems like we might want to at least give that a shot
> after some delay before prompting the user.

Is this method similar to automatically retrying to fetch the update file (early in the flow)? If yes, then I think it’s worth trying.

The way it might work could look something like this:

* Prompt for manual entry
* When “Download Firefox” is clicked, download file
* If download failed, retry automatically a few times
* If all automatic retry attempts failed, go back to start of flow


> […] the best we can do is launch the .dmg and then immediately
> shut Firefox down, and just wait for them to launch the new install. I think
> we can restore their session in OSX, but it might take a bit of fiddling to
> do so automatically.

This sounds a good compromise. Most Mac users – including those who hadn’t updated their apps in a long time – know where their apps are located and how to launch them. And when they launch, their session would be restored.

I recall clicking History > Restore Previous Session after quitting Firefox manually and then reopening it. If we can’t do the restore for them automatically, can we show an instructional page that tells them to click on that menu item, or show about:sessionrestore (which is normally shown after a crash, but we can modify the messaging)?


> On Windows, we have a couple of options.
>
> […]
> 
> - Just accept that this message is there and the user has to click OK […]
> - Sidestep this issue by directing them to download the full installer […]
> - Close Firefox right after launching the installer […]

I prefer going with option #3. If the user drops out of the installer for any reason and then reopen Firefox manually, the same update system can tell them that they still need to install Firefox.

If they click “Install”, we can redo what we just did: close Firefox and launch the installer.

Comment 85

[Alex Thayer [:alexical] (she/her)]

Sounds good! Then do we want to change the text below on OSX?

Restart Firefox to install it. When it’s finished, Firefox will relaunch and restore all your open tabs and windows.

(Since it won't relaunch automatically)

Comment 86

[Bram Pitoyo [:bram]]

Absolutely. Based on this new behaviour, the text on Windows should look like this:

Heading
Firefox is ready to install.

Description
Firefox will quit and a new window called Firefox Setup will open. Click the “Install” button. When it’s finished, Firefox will relaunch and restore all your open tabs and windows.

[Not Now] [Install Firefox]


And on Mac, it should look like this:

Heading
Firefox is ready to install.

Description
Firefox will quit and a new Finder window containing the Firefox application will pop open. Drag the Firefox icon on top of the Applications folder in order to copy it there. Finally, start Firefox by clicking on its icon on your dock. Firefox will launch and restore all your open tabs and windows.

[Not Now] [Install Firefox]


How does that sound?

Comment 87

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/13-14/

Comment 88

[Alex Thayer [:alexical] (she/her)]

Sounds great - just pushed a revision with all of these changes :)

Comment 89

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review112750

Sorry for taking so long on this.

Note: there are several new blank lines in nsUpdateService.js that aren't necessary that should be removed.

After the patch is updated I'll land it on oak for testing.

::: browser/app/profile/firefox.js:92
(Diff revision 14)
> +#ifdef NIGHTLY_BUILD
> +pref("app.releaseNotesURL", "https://blog.nightly.mozilla.org/");
> +#else
> +pref("app.releaseNotesURL", "https://www.mozilla.org/%LOCALE%/firefox/{targetVersion}/auroranotes/");
> +#endif
> +#endif

Instead of a new pref this should use the update's detailsURL and if that doesn't exist then the app.update.details.url preference.

The app.update.details.url preference is located in browser/branding/<channel>/pref/firefox-branding.js file

Both release and beta share the same firefox-branding.js file under browser/branding/official/pref. I could be wrong but I don't think this should be an issue since the version should differentiate it well enough for this.

::: browser/base/content/browser.js:15
(Diff revision 14)
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/ContextualIdentityService.jsm");
>  Cu.import("resource://gre/modules/NotificationDB.jsm");
> +Cu.import("resource://gre/modules/Subprocess.jsm");

I would prefer it if for this patch you just open the web page to download the installer and the new code for launching the installer, etc. was done in a separate bug. This way if there is a problem with launching the installer the new update notification UI won't have to be disabled or backed out.

::: browser/base/content/browser.js:2584
(Diff revision 14)
>  
> -  init() {
> -    PanelUI.panel.addEventListener("popupshowing", this, true);
> +  tryGetPref(type, name, defaultValue) {
> +    try {
> +      return Services.prefs[`get${type}Pref`](name);
> +    } catch (e) {
> +      return defaultValue;

nit: iirc this is a strict error for not always having a return value which can be avoided by returning the default outside of the catch.

::: browser/base/content/browser.js:2853
(Diff revision 14)
> +        break;
> +    }
>    },
>  
> -  displayBadge(succeeded) {
> -    let status = succeeded ? "succeeded" : "failed";
> +  handleUpdateStaged(update, status) {
> +    handleUpdateDownloaded(status);

Shouldn't this be |this.handleUpdateDownloaded(status);| ?

I saw handleUpdateDownloaded as undefined in the console when I manually tested the patch and changing it as above fixed lots of stuff. Since the existing tests didn't catch this can you add a test for this?

::: browser/base/content/test/simpleUpdate/browser.ini:17
(Diff revision 14)
> +[browser_updatesCompleteAndPartialPatchesWithBadCompleteSize.js]
> +[browser_updatesCompleteAndPartialPatchesWithBadPartialSize.js]
> +[browser_updatesCompleteAndPartialPatchesWithBadSizes.js]
> +[browser_updatesCompletePatchApplyFailure.js]
> +[browser_updatesCompletePatchWithBadCompleteSize.js]
> +[browser_updatesDownloadFailures.js]

browser_updatesDownloadFailures.js doesn't exist in this patch

::: browser/base/content/test/simpleUpdate/browser_noUpdates.js:1
(Diff revision 14)
> +add_task(function* testNoUpdates() {

Not certain this is still the case and I just didn't get the memo but we usually add the following license for test files.

/* Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/ */

::: browser/themes/shared/notification-icons.inc.css:261
(Diff revision 14)
>  }
>  
>  #notification-popup-box[hidden] {
>    /* Override display:none to make the pluginBlockedNotification animation work
>       when showing the notification repeatedly. */
> -  display: -moz-box;
> +  display: -moz-box;j

Typo?

::: browser/themes/shared/notification-icons.inc.css:331
(Diff revision 14)
> +.popup-notification-icon[popupid="update-manual"],
> +.popup-notification-icon[popupid="update-launch"],
> +.popup-notification-icon[popupid="update-restart"] {
> +  background: #74BF43 url(chrome://browser/skin/notification-icons.svg#update) no-repeat center;
> +  border-radius: 50%;
> +}

nit: please add a newline.

Comment 90

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/14-15/

Comment 91

[Alex Thayer [:alexical] (she/her)]

Pushed a new changeset.

Regarding the license - per Gijs' comment, no license now indicates that it's public domain, so we can omit the license declaration at the top: https://blog.gerv.net/2014/09/licensing-policy-change-tests-are-now-public-domain/

Comment 92

[Robert Strong (they/them - no direct email)]

Doug, this patch doesn't apply off of the latest m-c. Can you update it?

Comment 93

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review112958

Patch needs to be updated to latest m-c

Comment 94

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/15-16/

Comment 95

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/16-17/

Comment 96

[Alex Thayer [:alexical] (she/her)]

@rstrong - it's on the latest m-c now. Sorry, I think I missed a needinfo on you last time.

Comment 97

[Robert Strong (they/them - no direct email)]

Thanks! I just landed this on oak so it is easier to verify the functionality, etc.

https://hg.mozilla.org/projects/oak/rev/a603c73c70d4a58efed878aea7fee5528e4c5a8c

BTW: I think some unintentional changes to testing/web-platform/mozilla/meta/MANIFEST.json crept into your patch

Comment 98

[Robert Strong (they/them - no direct email)]

Test failures

TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_misused_characters_in_strings.js | jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/omni.ja!/chrome/en-US/locale/browser/browser.dtd with key=updateAvailable.whatsnew.label has a misused apostrophe. Strings with apostrophes should use foo’s instead of foo's. -

TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_misused_characters_in_strings.js | jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/omni.ja!/chrome/en-US/locale/browser/browser.dtd with key=updateManual.message has a misused apostrophe. Strings with apostrophes should use foo’s instead of foo's. - 

TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_misused_characters_in_strings.js | jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/omni.ja!/chrome/en-US/locale/browser/browser.dtd with key=updateManual.whatsnew.label has a misused apostrophe. Strings with apostrophes should use foo’s instead of foo's. - 

TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_misused_characters_in_strings.js | jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/omni.ja!/chrome/en-US/locale/browser/browser.dtd with key=updateManual.header.message has a misused apostrophe. Strings with apostrophes should use foo’s instead of foo's. - 

Also...

TEST-UNEXPECTED-FAIL | browser/base/content/test/simpleUpdate/browser_updatesBasicPrompt.js | Test timed out -
TEST-UNEXPECTED-FAIL | browser/base/content/test/simpleUpdate/browser_updatesBasicPromptNoStaging.js | A promise chain failed to handle a rejection: - at chrome://mochikit/content/browser-test.js:742 - TypeError: this.SimpleTest.isExpectingUncaughtException is not a function

https://archive.mozilla.org/pub/firefox/tinderbox-builds/oak-macosx64-debug/oak_yosemite_r7-debug_test-mochitest-e10s-browser-chrome-7-bm106-tests1-macosx-build0.txt.gz

Comment 99

[Robert Strong (they/them - no direct email)]

The staging tests shouldn't run on asan builds.

TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at ApplyUpdate, ProcessUpdates, nsUpdateProcessor::StartStagedUpdate, applyImpl

https://archive.mozilla.org/pub/firefox/tinderbox-builds/oak-win32-debug/1488266676/oak-win32-debug-bm70-build1-build0.txt.gz

Comment 100

[Robert Strong (they/them - no direct email)]

Treeherder link

https://treeherder.mozilla.org/#/jobs?repo=oak&tochange=a603c73c70d4a58efed878aea7fee5528e4c5a8c&fromchange=a603c73c70d4a58efed878aea7fee5528e4c5a8c

Comment 101

[Robert Strong (they/them - no direct email)]

I landed a couple of patches on oak for the simple fixes
https://hg.mozilla.org/projects/oak/rev/2f9a4435f40a6b186393a76048a2e948ade6d78f
https://hg.mozilla.org/projects/oak/rev/9ed312fb63da66b67d371370aa25955c6e1eac14

https://treeherder.mozilla.org/#/jobs?repo=oak&tochange=2f9a4435f40a6b186393a76048a2e948ade6d78f&fromchange=2f9a4435f40a6b186393a76048a2e948ade6d78f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

If you don't have the cycles and I get a chance I'll take a look at the remaining failures

TEST-UNEXPECTED-FAIL | browser/base/content/test/simpleUpdate/browser_updatesBasicPrompt.js | Test timed out -
TEST-UNEXPECTED-FAIL | browser/base/content/test/simpleUpdate/browser_updatesBasicPromptNoStaging.js | A promise chain failed to handle a rejection: - at chrome://mochikit/content/browser-test.js:742 - TypeError: this.SimpleTest.isExpectingUncaughtException is not a function

Comment 102

[Alex Thayer [:alexical] (she/her)]

Unless something comes up I should be able to get to those today. Thanks for tackling the other ones!

Comment 103

[Alex Thayer [:alexical] (she/her)]

Attachment: test updates

Not sure why but I'm consistently getting 500's trying to push to review, so I'm just putting this in here.

Regarding the patch: I was having a bit of difficulty reproducing the test failures, but I did eventually get them to happen and go away with my changes. I decided to just use the existing update tests' path for the test updater, rather than pulling in everything to bring it next to my test - hopefully that seems reasonable.

Comment 104

[Robert Strong (they/them - no direct email)]

Not entirely sure why there was a problem with pushing to review but it might be because you were basing the patch off of oak (I noticed it had some of the changes I made to oak) instead of m-c.

It would be simpler for me to review if you could split up the patches into a test patch and a client code patch. If that is a pain no worries.

Please incorporate the two simple fixes (apostrophe and skip asan) that I pushed to oak.

Next step is to get rid of the additional test files that are copied over from
toolkit/mozapps/update/tests/data

There are only a couple of constants that each test needs and you can just create new files for these and load the files. At most you will need (you might be able to get rid of one or two)
REL_PATH_DATA
URL_HOST
URL_PATH_UPDATE_XML
URL_HTTP_UPDATE_SJS

In browser/base you will need something along the lines of
TEST_HARNESS_FILES.testing.mochitest.browser.browser.base.content.test.simpleUpdate += [
    '../../toolkit/mozapps/update/tests/chrome/update.sjs',
    '../../toolkit/mozapps/update/tests/data/sharedUpdateXML.js',
    '../../toolkit/mozapps/update/tests/data/simple.mar',
]

You should be able to define gURLData, remove SERVICE_URL, and replace SERVICE_URL with null in
browser_updatesPartialPatchApplyFailureWithCompleteValidationFailure.js
browser_updatesPartialPatchApplyFailureWithCompleteAvailable.js

I think it would be a good thing to rename simpleUpdate to appUpdate or something similar since there are several different systems that provide updates and this will make it clear that this is just for application update.

Tests are packaged and run on different systems so depending on files from a different test harness (e.g. mochitest-browser-chrome and mochitest-chrome) could break and I suspect that is why the tests with this latest patch are still failing on oak Windows 8 x64 opt. I'll think about how to do this better and comment when I have a better method.

If you are busy with other work at this time let me know and I can split the additional work into a separate bug. Thanks!

Comment 105

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review118612

Some significant restructing per

https://bugzilla.mozilla.org/show_bug.cgi?id=893505#c104

Comment 106

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #104)
<snip>
> In browser/base you will need something along the lines of
The moz.build file in browser/base

> TEST_HARNESS_FILES.testing.mochitest.browser.browser.base.content.test.
> simpleUpdate += [
>     '../../toolkit/mozapps/update/tests/chrome/update.sjs',
>     '../../toolkit/mozapps/update/tests/data/sharedUpdateXML.js',
>     '../../toolkit/mozapps/update/tests/data/simple.mar',
> ]

Comment 107

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #106)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #104)
> <snip>
> > In browser/base you will need something along the lines of
> The moz.build file in browser/base
> 
> > TEST_HARNESS_FILES.testing.mochitest.browser.browser.base.content.test.
> > simpleUpdate += [
> >     '../../toolkit/mozapps/update/tests/chrome/update.sjs',
> >     '../../toolkit/mozapps/update/tests/data/sharedUpdateXML.js',
> >     '../../toolkit/mozapps/update/tests/data/simple.mar',
> > ]

Actually go with something like this
TEST_HARNESS_FILES.testing.mochitest.browser.browser.base.content.test.simpleUpdate += [
    '/toolkit/mozapps/update/tests/chrome/update.sjs',
    '/toolkit/mozapps/update/tests/data/sharedUpdateXML.js',
    '/toolkit/mozapps/update/tests/data/simple.mar',
]

Comment 108

[Robert Strong (they/them - no direct email)]

In downloadPage.html add charset to avoid a strict error

<head>
  <title>Download page</title>
+  <meta charset="utf-8">
</head>

Go ahead and copy the updater to the test directory as we do for other tests in the following file. The build changes will need to be reviewed by a build peer so if there is a better way to do this they can provide one at the time of review.

toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in

Comment 109

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review118944

::: browser/base/content/browser.js:2738
(Diff revision 17)
> -    this._showBadge();
>    },
>  
> -  addBadge(badgeId, badgeStatus) {
> -    if (!badgeStatus) {
> -      Cu.reportError("badgeStatus must be defined");
> +  get badgeWaitTime() {
> +    return this.tryGetPref("Int",
> +                           "app.update.promptBadgeWaitTime",

Both update.promptWaitTime and app.update.promptWaitTime can be less than badgeWaitTime. Would it make sense to make this value the same or less when it is?

::: browser/base/content/browser.js:2742
(Diff revision 17)
> -      return;
> +                           2 * 24 * 3600 * 1000); // 2 days
> -    }
> -    this._changeBadge(badgeId, badgeStatus);
>    },
>  
> -  removeBadge(badgeId) {
> +  get redownloadPromptAttempts() {

Instead of calling this redownload* just call it download*.

For the pref name go app.update.download.promptAttempts.

Better still would be to move the setting of this pref to nsUpdateService.js and naming the pref app.update.download.attempts

::: browser/base/content/test/simpleUpdate/head.js:15
(Diff revision 17)
> +const PREF_APP_UPDATE_DOORHANGER                     = "app.update.doorhanger";
> +const PREF_APP_UPDATE_CHANNEL                        = "app.update.channel";
> +const PREF_APP_UPDATE_ENABLED                        = "app.update.enabled";
> +const PREF_APP_UPDATE_IDLETIME                       = "app.update.idletime";
> +const PREF_APP_UPDATE_URL_DETAILS                    = "app.update.url.details";
> +const PREF_APP_RELEASE_NOTES_URL                     = "app.releaseNotesURL";

This is no longer used and the calls to backup and restore this pref should be removed

::: browser/base/content/test/simpleUpdate/head.js:21
(Diff revision 17)
> +const PREF_APP_UPDATE_URL_MANUAL                     = "app.update.url.manual";
> +const PREF_APP_UPDATE_STAGING_ENABLED                = "app.update.staging.enabled";
> +const PREF_APP_UPDATE_BACKGROUNDMAXERRORS            = "app.update.backgroundMaxErrors";
> +const PREF_APP_UPDATE_REDOWNLOADPROMPTATTEMPTS       = "app.update.redownloadPromptAttempts";
> +const PREF_APP_UPDATE_REDOWNLOADPROMPTMAXATTEMPTS    = "app.update.redownloadPromptMaxAttempts";
> +const PREF_APP_UPDATE_REPROMPTDOWNLOADWAITTIME       = "app.update.repromptDownloadWaitTime";

This is unused

::: browser/base/content/test/simpleUpdate/head.js:22
(Diff revision 17)
> +const PREF_APP_UPDATE_STAGING_ENABLED                = "app.update.staging.enabled";
> +const PREF_APP_UPDATE_BACKGROUNDMAXERRORS            = "app.update.backgroundMaxErrors";
> +const PREF_APP_UPDATE_REDOWNLOADPROMPTATTEMPTS       = "app.update.redownloadPromptAttempts";
> +const PREF_APP_UPDATE_REDOWNLOADPROMPTMAXATTEMPTS    = "app.update.redownloadPromptMaxAttempts";
> +const PREF_APP_UPDATE_REPROMPTDOWNLOADWAITTIME       = "app.update.repromptDownloadWaitTime";
> +const PREF_APP_UPDATE_LAUNCHINSTALLERPROMPTWAITTIME  = "app.update.launchInstallerPromptWaitTime";

This is unused

::: browser/base/content/test/simpleUpdate/head.js:23
(Diff revision 17)
> +const PREF_APP_UPDATE_BACKGROUNDMAXERRORS            = "app.update.backgroundMaxErrors";
> +const PREF_APP_UPDATE_REDOWNLOADPROMPTATTEMPTS       = "app.update.redownloadPromptAttempts";
> +const PREF_APP_UPDATE_REDOWNLOADPROMPTMAXATTEMPTS    = "app.update.redownloadPromptMaxAttempts";
> +const PREF_APP_UPDATE_REPROMPTDOWNLOADWAITTIME       = "app.update.repromptDownloadWaitTime";
> +const PREF_APP_UPDATE_LAUNCHINSTALLERPROMPTWAITTIME  = "app.update.launchInstallerPromptWaitTime";
> +const PREF_APP_SECURITY_DIALOGENABLEDELAY            = "security.dialog_enable_delay";

This is unused

::: browser/base/content/test/simpleUpdate/head.js:28
(Diff revision 17)
> +const PREF_APP_SECURITY_DIALOGENABLEDELAY            = "security.dialog_enable_delay";
> +
> +const PREFBRANCH_APP_PARTNER         = "app.partner.";
> +const PREF_DISTRIBUTION_ID           = "distribution.id";
> +const PREF_DISTRIBUTION_VERSION      = "distribution.version";
> +const PREF_TOOLKIT_TELEMETRY_ENABLED = "toolkit.telemetry.enabled";

These four are unused

::: browser/components/customizableui/content/panelUI.inc.xul:504
(Diff revision 17)
> +                   checkboxhidden="true"
> +                   hidden="true">
> +  <popupnotificationcontent id="update-restart-notification-content" orient="vertical">
> +    <description id="update-restart-description">&updateRestart.message;</description>
> +  </popupnotificationcontent>
> +</popupnotification>

Add a newline to the end of this file.

::: browser/locales/en-US/chrome/browser/browser.dtd:896
(Diff revision 17)
>  
>  <!ENTITY emeLearnMoreContextMenu.label            "Learn more about DRM…">
>  <!ENTITY emeLearnMoreContextMenu.accesskey        "D">
> +
> +<!ENTITY updateAvailable.message "Update your &brandShorterName; for the latest in speed and privacy.">
> +<!ENTITY updateAvailable.whatsnew.label "See what's new.">

apostrophes as noted previously

Comment 110

[Robert Strong (they/them - no direct email)]

Also, telemetry will need to be added for the notifications as well as whether the client acted on the notification. If you like we can discuss this beforehand.

Comment 111

[Robert Strong (they/them - no direct email)]

Observation: I've been trying to figure out why the Windows 8 x64 opt tests are failing and noticed that the staging pref isn't seen as true in nsUpdateService.js when running browser_updatesBasicPrompt.js... strange

Comment 112

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #111)
> Observation: I've been trying to figure out why the Windows 8 x64 opt tests
> are failing and noticed that the staging pref isn't seen as true in
> nsUpdateService.js when running browser_updatesBasicPrompt.js... strange

I think I see what is going on at least in part and will provide an update later.

Comment 113

[Robert Strong (they/them - no direct email)]

So, the Windows 8 x64 opt browser_noUpdates.js test isn't finishing before moving on to the next test. The reason that the test_0013_check_no_updates.xul test doesn't have this problem is because it calls the UpdatePrompt version of checkForUpdates which has a listener to determine that the check has finished.

It would probably be a good thing to go over all the uses of setTimeout along with similar calls such as yield sleep(100) where the code is trying to deal with race conditions and remove as many as possible. For the ones that can't be removed it would be a good thing to find conditions to check for before proceeding.

Comment 114

[Robert Strong (they/them - no direct email)]

Since the browser_noUpdates.js test is testing not showing the ui when there are no updates found I think it is best to just remove the test for now and file a followup bug to add a test for this case so this bug can move forward. I've removed the test from oak to see if there are any other failures.

Comment 115

[Robert Strong (they/them - no direct email)]

Removing browser_noUpdates.js made the tests pass.

I verified that the following error is caused by the patch and I think it is the only remaining failure *fingers crossed*

https://archive.mozilla.org/pub/firefox/tinderbox-builds/oak-win64-debug/1488757637/oak_win8_64-debug_test-mochitest-browser-chrome-7-bm127-tests1-windows-build1.txt.gz

Comment 116

[Robert Strong (they/them - no direct email)]

FYI: I backed out the current patches on oak and updated it to the latest m-c in preparation for the new patch.

Comment 117

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/17-18/

Comment 118

[Alex Thayer [:alexical] (she/her)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #115)
> Removing browser_noUpdates.js made the tests pass.
> 
> I verified that the following error is caused by the patch and I think it is
> the only remaining failure *fingers crossed*
> 
> https://archive.mozilla.org/pub/firefox/tinderbox-builds/oak-win64-debug/
> 1488757637/oak_win8_64-debug_test-mochitest-browser-chrome-7-bm127-tests1-
> windows-build1.txt.gz

To clarify, do you mean the failure in browser_devices_get_user_media_screen.js? What did you do to validate that it is caused by the patch? I can't get it to go away on m-c even without my patch, but I brought my patch into oak on my machine and that test runs just fine. I suspect it's just a mozconfig difference, but I haven't validated that yet.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #109)
> Both update.promptWaitTime and app.update.promptWaitTime can be less than
> badgeWaitTime. Would it make sense to make this value the same or less when
> it is?

Where it's used, it behaves as if they're the same if promptWaitTime is less. I think it makes most sense to keep this logic at the usage site, rather than complicating the getter too much, but I can change that if you'd like!

Comment 119

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/18-19/

Comment 120

[Robert Strong (they/them - no direct email)]

(In reply to Doug Thayer [:dthayer] from comment #118)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #115)
> > Removing browser_noUpdates.js made the tests pass.
> > 
> > I verified that the following error is caused by the patch and I think it is
> > the only remaining failure *fingers crossed*
> > 
> > https://archive.mozilla.org/pub/firefox/tinderbox-builds/oak-win64-debug/
> > 1488757637/oak_win8_64-debug_test-mochitest-browser-chrome-7-bm127-tests1-
> > windows-build1.txt.gz
> 
> To clarify, do you mean the failure in
> browser_devices_get_user_media_screen.js? What did you do to validate that
> it is caused by the patch? I can't get it to go away on m-c even without my
> patch, but I brought my patch into oak on my machine and that test runs just
> fine. I suspect it's just a mozconfig difference, but I haven't validated
> that yet.

In this push I removed the browser_noUpdates.js test. When the failure showed up I triggered the test a few more times and each time it failed.
https://treeherder.mozilla.org/#/jobs?repo=oak&revision=99916dcd485b0b83115f7abd71279a2bfba3f649
https://hg.mozilla.org/projects/oak/rev/99916dcd485b0b83115f7abd71279a2bfba3f649

In this push I disabled the tests on Win debug and the test didn't fail
https://treeherder.mozilla.org/#/jobs?repo=oak&revision=1e886bb979ca7dcc7ce688deea23eb069c7865c5
https://hg.mozilla.org/projects/oak/rev/1e886bb979ca7dcc7ce688deea23eb069c7865c5

In this push I backed out the patch that disabled the tests on win debug and the tests failed again with a couple of retriggers as well.
https://treeherder.mozilla.org/#/jobs?repo=oak&revision=c25ff734f7fffe810d44f6e722266698215acf35
https://hg.mozilla.org/projects/oak/rev/c25ff734f7fffe810d44f6e722266698215acf35

Also, the test that failed is immediately after these new tests.

Comment 121

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #120)
> (In reply to Doug Thayer [:dthayer] from comment #118)
> > (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> > comment #115)
> > > Removing browser_noUpdates.js made the tests pass.
> > > 
> > > I verified that the following error is caused by the patch and I think it is
> > > the only remaining failure *fingers crossed*
> > > 
> > > https://archive.mozilla.org/pub/firefox/tinderbox-builds/oak-win64-debug/
> > > 1488757637/oak_win8_64-debug_test-mochitest-browser-chrome-7-bm127-tests1-
> > > windows-build1.txt.gz
> > 
> > To clarify, do you mean the failure in
> > browser_devices_get_user_media_screen.js? What did you do to validate that
> > it is caused by the patch? I can't get it to go away on m-c even without my
> > patch, but I brought my patch into oak on my machine and that test runs just
> > fine. I suspect it's just a mozconfig difference, but I haven't validated
> > that yet.
> 
> In this push I removed the browser_noUpdates.js test. When the failure
> showed up I triggered the test a few more times and each time it failed.
> https://treeherder.mozilla.org/#/
> jobs?repo=oak&revision=99916dcd485b0b83115f7abd71279a2bfba3f649
> https://hg.mozilla.org/projects/oak/rev/
> 99916dcd485b0b83115f7abd71279a2bfba3f649
> 
> In this push I disabled the tests on Win debug and the test didn't fail
For clarity, I disabled these new tests on Win debug and then the existing test didn't fail

Comment 122

[Robert Strong (they/them - no direct email)]

Often when there are debug only test failures the failure is often a race condition and when it is windows there is also the possibility of it being a file in use condition. These at times will only show up on tinderbox builds and not on the systems developers use.

Comment 123

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review119920

Just did a fairly quick run through of the patch to see if I could identify why the tests are failing now so I also did a quick review.

::: browser/app/profile/firefox.js:117
(Diff revision 19)
>  pref("app.update.backgroundMaxErrors", 10);
>  
>  // Whether or not app updates are enabled
>  pref("app.update.enabled", true);
>  
> +// Whether or not to use the simplified doorhanger application update UI.

nit: no need for the word simplified.

::: browser/base/content/test/appUpdate/head.js:298
(Diff revision 19)
> + *         The text to log.
> + * @param  aCaller (optional)
> + *         An optional Components.stack.caller. If not specified
> + *         Components.stack.caller will be used.
> + */
> +function debugDump(aText, aCaller) {

Any reason not to import the shared.js file as well instead of copying these functions?

::: browser/base/content/test/appUpdate/head.js:690
(Diff revision 19)
> + * @param  aPreviousAppVersion (optional)
> + *         The application version prior to applying the update.
> + *         If not specified it will not be present.
> + * @return The string representing an update element for an update xml file.
> + */
> +function getLocalUpdateString(aPatches, aType, aName, aDisplayVersion,

This is copied code from sharedUpdateXML.js which this file imports.

::: browser/base/content/test/appUpdate/head.js:724
(Diff revision 19)
> +                   "foregroundDownload=\"" + foregroundDownload + "\">" +
> +              aPatches +
> +         "  </update>";
> +}
> +
> +function checkWhatsNewLink(id, url) {

Please add a javadoc comment for this function.

::: toolkit/components/telemetry/Histograms.json:11083
(Diff revision 19)
>      "kind": "categorical",
>      "labels": ["installAmoAccepted", "installAmoRejected", "installLocalAccepted", "installLocalRejected", "installWebAccepted", "installWebRejected", "sideloadAccepted", "sideloadRejected", "updateAccepted", "updateRejected"],
>      "description": "Results of displaying add-on installation notifications.",
>      "releaseChannelCollection": "opt-out"
> +  },
> +  "UPDATER_NOTIFICATION_SHOWN": {

Please place these along with the other app update histograms and prefix it with UPDATE_

::: toolkit/components/telemetry/Histograms.json:11084
(Diff revision 19)
>      "labels": ["installAmoAccepted", "installAmoRejected", "installLocalAccepted", "installLocalRejected", "installWebAccepted", "installWebRejected", "sideloadAccepted", "sideloadRejected", "updateAccepted", "updateRejected"],
>      "description": "Results of displaying add-on installation notifications.",
>      "releaseChannelCollection": "opt-out"
> +  },
> +  "UPDATER_NOTIFICATION_SHOWN": {
> +    "alert_emails": ["[e-mail]"],

Note: I'm going to need to think about the cases some more as well.

::: toolkit/mozapps/update/nsUpdateService.js:41
(Diff revision 19)
>  const PREF_APP_UPDATE_POSTUPDATE           = "app.update.postupdate";
>  const PREF_APP_UPDATE_PROMPTWAITTIME       = "app.update.promptWaitTime";
>  const PREF_APP_UPDATE_SERVICE_ENABLED      = "app.update.service.enabled";
>  const PREF_APP_UPDATE_SERVICE_ERRORS       = "app.update.service.errors";
>  const PREF_APP_UPDATE_SERVICE_MAXERRORS    = "app.update.service.maxErrors";
> +const PREF_APP_UPDATE_DOORHANGER           = "app.update.doorhanger";

These are sorted by name. Please place the new one so the sorting is preserved.

::: toolkit/mozapps/update/nsUpdateService.js:1220
(Diff revision 19)
>                          update.state = STATE_PENDING_ELEVATE);
>        }
>        update.statusText = gUpdateBundle.GetStringFromName("elevationFailure");
>        update.QueryInterface(Ci.nsIWritablePropertyBag);
>        update.setProperty("patchingFailed", "elevationFailure");
> +

Please don't add empty lines unrelated to the code changes in this patch.

::: toolkit/mozapps/update/nsUpdateService.js:1292
(Diff revision 19)
>                   getService(Ci.nsIApplicationUpdateService).
>                   downloadUpdate(update, !postStaging);
>      if (status == STATE_NONE)
>        cleanupActiveUpdate();
>    } else {
> +    Services.obs.notifyObservers(update, "update-error", "unknown");

Please put this after the call to LOG and no need for the empty line.

::: toolkit/mozapps/update/nsUpdateService.js:2179
(Diff revision 19)
> +      Services.obs.notifyObservers(update, "update-error", "check-attempts-exceeded");
>        prompter.showUpdateError(update);
>        AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_GENERAL_ERROR_PROMPT);
>      } else {
> +      Services.obs.notifyObservers(update, "update-error", "check-attempt-failed");
> +

No need for the empty line.

::: toolkit/mozapps/update/nsUpdateService.js:2539
(Diff revision 19)
>              "update is not supported for this system");
>          this._showPrompt(update);
>        }
> +
> +      Services.obs.notifyObservers(null, "update-available", "unsupported");
> +

No need for the empty line after the new code.

::: toolkit/mozapps/update/nsUpdateService.js:2549
(Diff revision 19)
>      if (!getCanApplyUpdates()) {
>        LOG("UpdateService:_selectAndInstallUpdate - the user is unable to " +
>            "apply updates... prompting");
> +
> +      Services.obs.notifyObservers(null, "update-available", "cant-apply");
> +

No need for the empty line after the new code.

::: toolkit/mozapps/update/nsUpdateService.js:2577
(Diff revision 19)
>        LOG("UpdateService:_selectAndInstallUpdate - prompting because the " +
>            "update snippet specified showPrompt");
>        AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_SHOWPROMPT_SNIPPET);
> +
> +      Services.obs.notifyObservers(update, "update-available", "show-prompt");
> +

No need for the empty line after the new code.

::: toolkit/mozapps/update/nsUpdateService.js:2588
(Diff revision 19)
>        LOG("UpdateService:_selectAndInstallUpdate - prompting because silent " +
>            "install is disabled");
>        AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_SHOWPROMPT_PREF);
> +
> +      Services.obs.notifyObservers(update, "update-available", "show-prompt");
> +

No need for the empty line after the new code.

::: toolkit/mozapps/update/nsUpdateService.js:3367
(Diff revision 19)
>      LOG("Checker:onLoad - request completed downloading document");
>  
>      try {
>        // Analyze the resulting DOM and determine the set of updates.
>        var updates = this._updates;
> +

Please don't add empty lines unrelated to the code changes in this patch.

::: toolkit/mozapps/update/nsUpdateService.js:3847
(Diff revision 19)
>                                     : DOWNLOAD_FOREGROUND_INTERVAL;
>  
> -    var uri = Services.io.newURI(this._patch.URL);
> +    LOG("Downloader:downloadUpdate - url: " + this._patch.URL + ", path: " +
> -    LOG("Downloader:downloadUpdate - url: " + uri.spec + ", path: " +
>          patchFile.path + ", interval: " + interval);
> +    var uri = Services.io.newURI(this._patch.URL);

What purpose does this change serve?

::: toolkit/mozapps/update/nsUpdateService.js:4164
(Diff revision 19)
>          }
>        }
>  
>        if (allFailed) {
>          LOG("Downloader:onStopRequest - all update patch downloads failed");
> +

No need for the empty line before the new code.

::: toolkit/mozapps/update/tests/chrome/testConstants.js:1
(Diff revision 19)
> +const REL_PATH_DATA = "chrome/toolkit/mozapps/update/tests/data/";

I think these names can be cleaned up further and either you or I can figure that out after the tests are passing again and these changes have been made.

::: toolkit/mozapps/update/tests/chrome/update.sjs:9
(Diff revision 19)
>  
>  /**
>   * Server side http server script for application update tests.
>   */
>  
> -const { classes: Cc, interfaces: Ci } = Components;
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

Is Cu used? If not, please remove.

::: toolkit/mozapps/update/tests/chrome/update.sjs:34
(Diff revision 19)
>  }
> -loadHelperScript();
>  
> -const URL_HOST = "http://example.com";
> -const URL_PATH_UPDATE_XML = "/chrome/toolkit/mozapps/update/tests/chrome/update.sjs";
> -const URL_HTTP_UPDATE_SJS = URL_HOST + URL_PATH_UPDATE_XML;
> +var scriptFile = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsILocalFile);
> +scriptFile.initWithPath(getState("__LOCATION__"));
> +dump("\n" + scriptFile.path + "\n\n");

Extraneous call to dump... please remove.

::: toolkit/mozapps/update/tests/data/shared.js:27
(Diff revision 19)
>  const PREF_APP_UPDATE_SILENT               = "app.update.silent";
>  const PREF_APP_UPDATE_SOCKET_MAXERRORS     = "app.update.socket.maxErrors";
>  const PREF_APP_UPDATE_STAGING_ENABLED      = "app.update.staging.enabled";
>  const PREF_APP_UPDATE_URL                  = "app.update.url";
>  const PREF_APP_UPDATE_URL_DETAILS          = "app.update.url.details";
> +const PREF_APP_UPDATE_DOORHANGER           = "app.update.doorhanger";

These names are sorted so please place this new pref in the appropriate place to retain the sorting.

::: toolkit/mozapps/update/tests/data/xpcshellUtilsAUS.js:37
(Diff revision 19)
>  /* eslint-disable no-undef */
>  
>  const { classes: Cc, interfaces: Ci, manager: Cm, results: Cr,
>          utils: Cu } = Components;
>  
> +const URL_HTTP_UPDATE_SJS = "http://test_details/";

I think these names can be cleaned up further and either you or I can figure that out after the tests are passing again and these changes have been made.

Comment 124

[Robert Strong (they/them - no direct email)]

It also looks like several of the dir provider names you copied over aren't used. If you import shared.js then they can all be removed or if there is a reason not to import it then please only add the ones that are used.

Comment 125

[Alex Thayer [:alexical] (she/her)]

Quick heads up: I found the issue with the OSX/Linux failures. It was my own sloppiness, so apologies for that. Somehow two switch cases ("applied" and "pending") were removed in browser.js. In Windows we would always hit the "pending-service" case since this fires even with the pref off.

Working on the review now.

Comment 126

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/19-20/

Comment 127

[Alex Thayer [:alexical] (she/her)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #123)
> ::: toolkit/mozapps/update/nsUpdateService.js:1220
> (Diff revision 19)
> >                          update.state = STATE_PENDING_ELEVATE);
> >        }
> >        update.statusText = gUpdateBundle.GetStringFromName("elevationFailure");
> >        update.QueryInterface(Ci.nsIWritablePropertyBag);
> >        update.setProperty("patchingFailed", "elevationFailure");
> > +
> 
> Please don't add empty lines unrelated to the code changes in this patch.

Sorry! I add logging statements in sometimes to debug certain things and sometimes the newlines get left after I clean them up. Working on getting more methodical with cleaning up that kind of stuff.

> ::: toolkit/mozapps/update/nsUpdateService.js:3847
> (Diff revision 19)
> >                                     : DOWNLOAD_FOREGROUND_INTERVAL;
> >  
> > -    var uri = Services.io.newURI(this._patch.URL);
> > +    LOG("Downloader:downloadUpdate - url: " + this._patch.URL + ", path: " +
> > -    LOG("Downloader:downloadUpdate - url: " + uri.spec + ", path: " +
> >          patchFile.path + ", interval: " + interval);
> > +    var uri = Services.io.newURI(this._patch.URL);
> 
> What purpose does this change serve?

This was to help diagnose an issue where the newURI call was failing because this._patch.URL was garbage. Moving the log call first ensures that we get to see what the URL is first.

Comment 128

[Robert Strong (they/them - no direct email)]

All tests are now passing on all platforms!

Comment 129

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #128)
> All tests are now passing on all platforms!

Robert, I wonder when it will be the right time when we have to take a look at the firefox-ui update tests, and to get those updated regarding all the upcoming changes. I don't want to see them completely broken once the feature lands on mozilla-central. Thanks.

Comment 130

[Robert Strong (they/them - no direct email)]

We won't be removing the existing ui (not sure when we will) and there is a pref to switch between them so you should be able to change the pref to make it so your tests continue to work. I am hoping that it will be around a week when the app update code and tests are in shape to land.

Comment 131

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That sounds great and actually gives us the time to gracefully move over. Thanks for the quick feedback!

Comment 132

[Robert Strong (they/them - no direct email)]

dthayer, I am going to be testing out how the oak nightly behaves using today's (3/9/17) build and updating to the next days build in case you'd like to try it out as well. Today's builds are located at
https://archive.mozilla.org/pub/firefox/nightly/2017/03/2017-03-09-04-02-02-oak/

Comment 133

[Robert Strong (they/them - no direct email)]

Just as the value of app.update.promptWaitTime is different per channel the value of app.update.promptBadgeWaitTime should be different per channel. For example, on one channel we might want users to update sooner so we can get telemetry or crash reports sooner (e.g. nightly) while on another channel we want to be less intrusive (e.g. aurora or release).

BTW: app.update.promptWaitTime controls the period of time to wait before adding the green circle with the arrow in it to the hamburger menu which is very unobtrusive and this is not about the time to wait until the doorhanger is displayed. Here is a screenshot
https://cl.ly/1g2D0v1j1B38

Also the time before displaying the doorhanger will be the same as the time before we currently display the big UI.

Here are the current channel values for app.update.promptWaitTime and values I *think* would be appropriate for the app.update.promptBadgeWaitTime preference for the channels.

Nightly:
app.update.promptWaitTime       12 hours
app.update.promptBadgeWaitTime   1 hour
Note: I'm tempted to just go with it displaying immediately since the community of Nightly clients often manually check for updates.

Aurora:
app.update.promptWaitTime       192 hours (e.g. 8 days)
app.update.promptBadgeWaitTime   24 hours

Beta:
app.update.promptWaitTime       192 hours (e.g. 8 days)
app.update.promptBadgeWaitTime   48 hours

Release:
app.update.promptWaitTime       192 hours (e.g. 8 days)
app.update.promptBadgeWaitTime   48 hours

Note: Release and Beta share the same preferences ever since release drivers pushed for release and beta to be as similar to each other as possible so it isn't really an option to have different values for Release and Beta. If this is desired it should be done in a separate bug.

Benjamin, since you provided the values for promptWaitTime in bug 1268340 could you provide your input on these new values or let me know who I should ask? Thanks!

Comment 134

[Robert Strong (they/them - no direct email)]

dthayer, just noticed that the preferences for app.update.badge haven't been removed but the code that checks that pref has been removed.
https://dxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#127
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/config.py#177
possibly others

Comment 135

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #133)
> Just as the value of app.update.promptWaitTime is different per channel the
> value of app.update.promptBadgeWaitTime should be different per channel. For
> example, on one channel we might want users to update sooner so we can get
> telemetry or crash reports sooner (e.g. nightly) while on another channel we
> want to be less intrusive (e.g. aurora or release).
> 
> BTW: app.update.promptWaitTime controls the period of time to wait before
> adding the green circle with the arrow in it to the hamburger menu which is
> very unobtrusive and this is not about the time to wait until the doorhanger
> is displayed. Here is a screenshot
> https://cl.ly/1g2D0v1j1B38
> 
> Also the time before displaying the doorhanger will be the same as the time
> before we currently display the big UI.
> 
> Here are the current channel values for app.update.promptWaitTime and values
> I *think* would be appropriate for the app.update.promptBadgeWaitTime
> preference for the channels.
> 
> Nightly:
> app.update.promptWaitTime       12 hours
> app.update.promptBadgeWaitTime   1 hour
> Note: I'm tempted to just go with it displaying immediately since the
> community of Nightly clients often manually check for updates.
> 
> Aurora:
> app.update.promptWaitTime       192 hours (e.g. 8 days)
> app.update.promptBadgeWaitTime   24 hours
> 
> Beta:
> app.update.promptWaitTime       192 hours (e.g. 8 days)
> app.update.promptBadgeWaitTime   48 hours
> 
> Release:
> app.update.promptWaitTime       192 hours (e.g. 8 days)
> app.update.promptBadgeWaitTime   48 hours
I just noticed that this is currently set to 0 hours on nightly and 96 hours on aurora. Beta and Release have this disabled though they are also set to 0 hours.

Comment 136

[Robert Strong (they/them - no direct email)]

dthayer, the existing pref for the badge wait time is app.update.badgeWaitTime. Any reason not to reuse that preference? If it isn't reused please remove it.
https://dxr.mozilla.org/mozilla-central/search?q=app.update.badgeWaitTime&redirect=false

Also, app.update.promptBadgeWaitTime will need to be added in its place though the values haven't been determined for all channels as of yet.

Comment 137

[Robert Strong (they/them - no direct email)]

Bram, when in fullscreen no notification is displayed. Are there plans to provide some form of notification?

When in fullscreen it is possible to display the normal UI by placing the mouse pointer on the top of the screen. When this is done neither the doorhanger or the badge is displayed if the doorhanger is displayed. If only the badge is displayed then the badge is displayed for this case. I personally think that the doorhanger should be displayed for this case. What do you think should be done for this case?

Comment 138

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #137)
> Bram, when in fullscreen no notification is displayed. Are there plans to
> provide some form of notification?
> 
> When in fullscreen it is possible to display the normal UI by placing the
> mouse pointer on the top of the screen. When this is done neither the
> doorhanger or the badge is displayed if the doorhanger is displayed. If only
> the badge is displayed then the badge is displayed for this case.
<snip>
For clarity: I personally think that the doorhanger should be displayed for this case is in reference to neither the doorhanger or the badge is displayed if the doorhanger is displayed.  What do you think should be done for this case?

Comment 139

[Bram Pitoyo [:bram]]

Hi Robert, if I’m understanding you correctly, then I think I agree.

– When you place the mouse pointer on top of the screen, the toolbar will scroll down, followed immediately by the doorhanger animating in
– Simply hovering your mouse pointer away from the toolbar will not dismiss the doorhanger or the toolbar. Clicking on the webpage won’t dismiss them, either. Simply put, the doorhanger is ‘blocking’ the toolbar from hiding. 
– Simply interact with one of the buttons on the doorhanger, and it will be dismissed. Without the doorhanger ‘blocking’, the toolbar can now scroll back up properly.

Does this animation demonstrate the behaviour you’re thinking of?
https://d3uepj124s5rcx.cloudfront.net/items/190h3c1v1e3426282c1p/update-doorhanger-fullscreen.gif

Comment 140

[Robert Strong (they/them - no direct email)]

That works for me... though I don't think blocking should be necessary I'm fine with it blocking.

Comment 141

[Bram Pitoyo [:bram]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #140)
> though I don't think blocking should be necessary I'm fine with it blocking.

Ideally, we’d behave just like every other doorhanger. So if Firefox already has something prescribed, let’s follow that. What do you think?

Comment 142

[Robert Strong (they/them - no direct email)]

I'm fine with it either way. Unless I'm mistaken we typically don't prevent clients from doing whatever they are doing except for instances where we require input from the client to perform an action that they initiate. So in this instance we'd display it and not require the client to interact with this UI to go back into fullscreen or anything else they might want to do?

Comment 143

[Robert Strong (they/them - no direct email)]

Bram, what about the first question? When in fullscreen without either exiting fullscreen or placing the mouse pointer at the top of the screen there is no notification displayed. Are there plans to provide some form of notification? I don't think there is anything that does so currently and there is no real estate to place a notification. I'm mainly calling this out to make sure that this is acceptable at least for landing and to find out if there will need to be additional work. Thanks!

Comment 144

[Bram Pitoyo [:bram]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #142)
> […] So in this instance we'd display it and not require the client to interact
> with this UI to go back into fullscreen or anything else they might want to do?

This works for me. Let’s do that.


(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #143)
> When in fullscreen without either  exiting fullscreen or placing the mouse
> pointer at the top of the screen there is no notification displayed. Are there
> plans to provide some form of notification? I don't think there is anything
> that does so currently and there is no real estate to place a notification.

Thanks for clarifying this specific scenario. I don’t think we need to show any notification here, because the user will already see the doorhanger at one point or another.

For instance, if you quit Firefox using Alt+F4, you’ll see the doorhanger the next time the browser opens (Firefox doesn’t open in fullscreen mode by default). Of course, if you try to access the toolbar at any point, you’re also going to see the doorhanger.

Ultimately, there’s no way to avoid the doorhanger. Knowing this, I’d be okay with not showing any update notification when the user is fullscreen mode with minimised toolbar.

Comment 145

[Benjamin Smedberg]

Now that RT is the product manager for install& update I think he should make the timing decisions.

Comment 146

[Robert Strong (they/them - no direct email)]

A couple of notes in an email from RT.
- the button on that panel are displayed in the opposite order 
- the "Download Firefox update" entry in the Hamburger menu does not display the "up" arrow or Firefox logo

Comment 147

[Alex Thayer [:alexical] (she/her)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #146)
> A couple of notes in an email from RT.
> - the button on that panel are displayed in the opposite order

This is likely due to viewing it on Windows / Linux. The mock-up is on OSX, where our doorhanger styles have the button order reversed to better approximate OSX's native styling of things (AFAICT). In either case, this is general doorhanger styling and not anything that I specifically touched.

> - the "Download Firefox update" entry in the Hamburger menu does not display
> the "up" arrow or Firefox logo

Hmm, that must be a regression from one of the patches since the first. I'll look into it.

Comment 148

[Robert Strong (they/them - no direct email)]

(In reply to Doug Thayer [:dthayer] from comment #147)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #146)
> > A couple of notes in an email from RT.
> > - the button on that panel are displayed in the opposite order
> 
> This is likely due to viewing it on Windows / Linux. The mock-up is on OSX,
> where our doorhanger styles have the button order reversed to better
> approximate OSX's native styling of things (AFAICT). In either case, this is
> general doorhanger styling and not anything that I specifically touched.
I thought as much and thanks!

> > - the "Download Firefox update" entry in the Hamburger menu does not display
> > the "up" arrow or Firefox logo
> 
> Hmm, that must be a regression from one of the patches since the first. I'll
> look into it.
Note: I see them on my system so this is likely specific to the platform that RT is using.

Comment 149

[Robert Strong (they/them - no direct email)]

Romain, comment #133 is what we need your input on. Please note that on Nightly the badge currently is display immediately and that on Aurora it is currently displayed after 96 hours. The badge is not displayed on release / beta (which share the same settings per comment #133) so there is no existing amount of time for those channels.

Comment 150

[Robert Strong (they/them - no direct email)]

Doug, go ahead and set the value for this in firefox.js
app.update.download.promptMaxAttempts

I don't think we want to just show the manual update notification for the first elevation that was cancelled though I'm not certain what a good number of attempts should be so I'll think about it. Wouldn't hurt to set this up similar to download attempts.

      case "elevation-canceled":
      case "unknown":
        // Background update has failed, let's show the UI responsible for
        // prompting the user to update manually.
        this.clearCallbacks();
        this.showManualUpdateNotification(update, false);

Comment 151

[Robert Strong (they/them - no direct email)]

BTW: if there are other possible prefs that haven't been added to the pref files please add them. That is just how prefs are done in Firefox.

Comment 152

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review121858

Please set promptBadgeWaitTime to 0 on nightly and 96 hours on aurora, beta, and release since this is equivelent to badgeWaitTime which is 0 on nightly and 96 hours on aurora. If the feedback from RT changes it then just change it to those values then.

This is really close to being done. Thanks!

::: browser/base/content/browser.js:2735
(Diff revision 20)
> -      PanelUI.menuButton.removeAttribute("badge-status");
> -    }
>    },
>  
> -  _changeBadge(badgeId, badgeStatus = null) {
> -    if (badgeId == this.BADGEID_APPUPDATE) {
> +  get enabled() {
> +    return Services.prefs.getBoolPref("app.update.doorhanger");

Please handle the case where this pref isn't defined as well. It will have to default to false for now since nsUpdateService.js has to default to false otherwise it would break other apps such as Thunderbird.

::: browser/base/content/browser.js:2740
(Diff revision 20)
> -    this._showBadge();
>    },
>  
> -  addBadge(badgeId, badgeStatus) {
> -    if (!badgeStatus) {
> -      Cu.reportError("badgeStatus must be defined");
> +  get badgeWaitTime() {
> +    return this.tryGetPref("Int",
> +                           "app.update.promptBadgeWaitTime",

Please store this in seconds instead of milliseconds.

::: browser/base/content/browser.js:2741
(Diff revision 20)
>    },
>  
> -  addBadge(badgeId, badgeStatus) {
> -    if (!badgeStatus) {
> -      Cu.reportError("badgeStatus must be defined");
> -      return;
> +  get badgeWaitTime() {
> +    return this.tryGetPref("Int",
> +                           "app.update.promptBadgeWaitTime",
> +                           2 * 24 * 3600 * 1000); // 2 days

Most values like this have a min and a max value enforced so things like preference can't effectively disable app update when that isn't the purpose of the preference, etc. I don't think this is necessary for any of the new preferences except perhaps app.update.download.promptMaxAttempts. I'll look through them again and I'd like you to do so as well in case I miss something.

::: browser/base/content/browser.js:2756
(Diff revision 20)
> -  }
> -};
>  
> -// Setup the hamburger button badges for updates, if enabled.
> -var gMenuButtonUpdateBadge = {
> -  enabled: false,
> +  get downloadPromptMaxAttempts() {
> +    return this.tryGetPref("Int",
> +                           "app.update.download.promptMaxAttempts",

Please add the app.update.download.promptMaxAttempts preference to firefox.js

::: browser/base/content/browser.js:2871
(Diff revision 20)
> +          update.selectedPatch.selected = false;
> +          this.showUpdateAvailableNotification(update, false);
> -    }
> +        }
> -    if (status == "failed") {
> +        break;
> +      case "check-attempts-exceeded":
> +      case "elevation-canceled":

I don't think we want to just show the manual update notification for the first elevation that was cancelled though I'm not certain what a good number of attempts should be so I'll think about it. Wouldn't hurt to set this up similar to download attempts.

::: browser/base/content/browser.js:2915
(Diff revision 20)
> +  },
>  
> -    // Give the user badgeWaitTime seconds to react before prompting.
> -    this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> -    this.timer.initWithCallback(this, this.badgeWaitTime * 1000,
> -                                this.timer.TYPE_ONE_SHOT);
> +  handleUpdateAvailable(update, status) {
> +    switch (status) {
> +      case "show-prompt":
> +        // Background update has failed, let's show the UI responsible for

This comment should be for the show prompt case and not the failure case.

::: browser/base/content/test/appUpdate/browser.ini:2
(Diff revision 20)
> +[DEFAULT]
> +support-files =

Please add immediately after [DEFAULT]
tags = appupdate

::: browser/base/content/test/appUpdate/head.js:290
(Diff revision 20)
> +    }
> +  }, topic, false))
> +}
> +
> +/* Triggers post-update processing */
> +function testPostUpdateProcessing() {

This is already available in shared.js

::: toolkit/components/telemetry/Histograms.json:4966
(Diff revision 20)
>      "kind": "enumerated",
>      "n_values": 30,
>      "releaseChannelCollection": "opt-out",
>      "description": "Update: the update wizard page displayed when the UI was closed (mapped in toolkit/mozapps/update/UpdateTelemetry.jsm)"
>    },
> +  "UPDATE_NOTIFICATION_SHOWN": {

These should all have
"releaseChannelCollection": "opt-out",

::: toolkit/components/telemetry/Histograms.json:4966
(Diff revision 20)
>      "kind": "enumerated",
>      "n_values": 30,
>      "releaseChannelCollection": "opt-out",
>      "description": "Update: the update wizard page displayed when the UI was closed (mapped in toolkit/mozapps/update/UpdateTelemetry.jsm)"
>    },
> +  "UPDATE_NOTIFICATION_SHOWN": {

After you make the following changes to the histograms they are going to need a review from one of the data stewards. I think it would be a good thing to either go with having these also set to "expires_in_version": "never" or to change them all to expire in the same version so please ask them which they would prefer.

The module owner and peers are listed here
https://wiki.mozilla.org/Firefox/Data_Collection

::: toolkit/components/telemetry/Histograms.json:4967
(Diff revision 20)
>      "n_values": 30,
>      "releaseChannelCollection": "opt-out",
>      "description": "Update: the update wizard page displayed when the UI was closed (mapped in toolkit/mozapps/update/UpdateTelemetry.jsm)"
>    },
> +  "UPDATE_NOTIFICATION_SHOWN": {
> +    "alert_emails": ["[e-mail]"],

Please use the following:
[e-mail]

This is a distribution list and if you'd like to be added to it just let me know.

::: toolkit/components/telemetry/Histograms.json:4971
(Diff revision 20)
> +  "UPDATE_NOTIFICATION_SHOWN": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",
> +    "kind": "categorical",
> +    "bug_numbers": [893505],
> +    "description": "Every time we show a doorhanger notification regarding application updates, we log the type of notification here.",

Please change the descript to:
Update: the doorhanger type that was displayed.

::: toolkit/components/telemetry/Histograms.json:4979
(Diff revision 20)
> +  "UPDATE_NOTIFICATION_DISMISSED": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",
> +    "kind": "categorical",
> +    "bug_numbers": [893505],
> +    "description": "Every time a user dismisses a doorhanger notification regarding application updates, we log the type of notification here.",

Please change the description to:
Update: the doorhanger type that was dismissed.

::: toolkit/components/telemetry/Histograms.json:4979
(Diff revision 20)
> +  "UPDATE_NOTIFICATION_DISMISSED": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",
> +    "kind": "categorical",
> +    "bug_numbers": [893505],
> +    "description": "Every time a user dismisses a doorhanger notification regarding application updates, we log the type of notification here.",

Please change the description to:
Update: the dismiss action was executed for this doorhanger type.

::: toolkit/components/telemetry/Histograms.json:4987
(Diff revision 20)
> +  "UPDATE_NOTIFICATION_MAIN_ACTION_DOORHANGER": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",
> +    "kind": "categorical",
> +    "bug_numbers": [893505],
> +    "description": "Every time a user clicks on the main action of doorhanger notification regarding application updates, we log the type of notification here.",

Please change the description to:
Update: the main action was initiated for this doorhanger type.

::: toolkit/components/telemetry/Histograms.json:4995
(Diff revision 20)
> +  "UPDATE_NOTIFICATION_MAIN_ACTION_MENU": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "60",
> +    "kind": "categorical",
> +    "bug_numbers": [893505],
> +    "description": "Every time a user interacts with the PanelUI menu item for a notification regarding application updates, we log the type of notification here.",

Please change the description to:
Update: the action was initiated from the PanelUI menu.

::: toolkit/mozapps/update/nsUpdateService.js:1194
(Diff revision 20)
>      writeStatusFile(getUpdatesDir(), update.state = STATE_PENDING);
>      return true;
>    }
>  
>    if (update.errorCode == ELEVATION_CANCELED) {
> +    Services.obs.notifyObservers(update, "update-error", "elevation-canceled");

Please add a call to LOG to includes that observers are being notified that includes the topic and the data.

::: toolkit/mozapps/update/nsUpdateService.js:1289
(Diff revision 20)
>                   getService(Ci.nsIApplicationUpdateService).
>                   downloadUpdate(update, !postStaging);
>      if (status == STATE_NONE)
>        cleanupActiveUpdate();
>    } else {
>      LOG("handleFallbackToCompleteUpdate - install of complete or " +

Please change this call to LOG to include that observers are being notified along with the topic and the data.

::: toolkit/mozapps/update/nsUpdateService.js:2170
(Diff revision 20)
>      let maxErrors = Math.min(getPref("getIntPref", PREF_APP_UPDATE_BACKGROUNDMAXERRORS, 10), 20);
>  
>      if (errCount >= maxErrors) {
>        let prompter = Cc["@mozilla.org/updates/update-prompt;1"].
>                       createInstance(Ci.nsIUpdatePrompt);
> +      Services.obs.notifyObservers(update, "update-error", "check-attempts-exceeded");

Please add a call to LOG to includes that observers are being notified that includes the topic and the data.

::: toolkit/mozapps/update/nsUpdateService.js:2174
(Diff revision 20)
>                       createInstance(Ci.nsIUpdatePrompt);
> +      Services.obs.notifyObservers(update, "update-error", "check-attempts-exceeded");
>        prompter.showUpdateError(update);
>        AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_GENERAL_ERROR_PROMPT);
>      } else {
> +      Services.obs.notifyObservers(update, "update-error", "check-attempt-failed");

Please add a call to LOG to includes that observers are being notified that includes the topic and the data.

::: toolkit/mozapps/update/nsUpdateService.js:2525
(Diff revision 20)
>      if (!update || update.elevationFailure) {
>        return;
>      }
>  
>      if (update.unsupported) {
>        LOG("UpdateService:_selectAndInstallUpdate - update not supported for " +

Please change this call to LOG to include that observers are being notified that includes the topic and the data.

::: toolkit/mozapps/update/nsUpdateService.js:2539
(Diff revision 20)
>        AUSTLMY.pingCheckCode(this._pingSuffix, AUSTLMY.CHK_UNSUPPORTED);
>        return;
>      }
>  
>      if (!getCanApplyUpdates()) {
>        LOG("UpdateService:_selectAndInstallUpdate - the user is unable to " +

Please change this call to LOG to include that observers are being notified that includes the topic and the data.

::: toolkit/mozapps/update/nsUpdateService.js:2565
(Diff revision 20)
>       * Update Type   Outcome
>       * Major         Notify
>       * Minor         Auto Install
>       */
>      if (update.showPrompt) {
>        LOG("UpdateService:_selectAndInstallUpdate - prompting because the " +

Please change this call to LOG to include that observers are being notified that includes the topic and the data.

::: toolkit/mozapps/update/nsUpdateService.js:2575
(Diff revision 20)
>        this._showPrompt(update);
>        return;
>      }
>  
>      if (!getPref("getBoolPref", PREF_APP_UPDATE_AUTO, true)) {
>        LOG("UpdateService:_selectAndInstallUpdate - prompting because silent " +

Please change this call to LOG to include that observers are being notified that includes the topic and the data.

::: toolkit/mozapps/update/nsUpdateService.js:3123
(Diff revision 20)
>        writeStatusFile(getUpdatesDir(), update.state = STATE_APPLIED_SERVICE);
>      }
>  
>      // Send an observer notification which the update wizard uses in
>      // order to update its UI.
>      LOG("UpdateManager:refreshUpdateStatus - Notifying observers that " +

Please change this call to LOG to include that observers are being notified that includes the topic and the data. Feel free to change the message to what you thinkk makes the most sense.

::: toolkit/mozapps/update/nsUpdateService.js:4153
(Diff revision 20)
>            allFailed = false;
>          }
>        }
>  
>        if (allFailed) {
>          LOG("Downloader:onStopRequest - all update patch downloads failed");

Please change this call to LOG to include that observers are being notified... something along the lines of the following but feel free to make it better:
"Downloader:onStopRequest - all update patch downloads failed. Notifying observers with topic = update-error and data = all-downloads-failed"

::: toolkit/mozapps/update/updater/updater-xpcshell/Makefile.in:41
(Diff revision 20)
> -	rsync -a -C $(XPCSHELLTESTROOT)/data/updater.app $(MOCHITESTROOT)/data/
> +	rsync -a -C $(XPCSHELLTESTDIR)/data/updater.app $(MOCHITESTCHROMEDIR)/data/
> +	rsync -a -C $(XPCSHELLTESTDIR)/data/updater.app $(MOCHITESTBROWSERDIR)/
>  else
> -	cp $(FINAL_TARGET)/updater-xpcshell$(BIN_SUFFIX) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)
> -	cp $(FINAL_TARGET)/updater-xpcshell$(BIN_SUFFIX) $(MOCHITESTROOT)/data/updater$(BIN_SUFFIX)
> +	cp $(FINAL_TARGET)/updater-xpcshell$(BIN_SUFFIX) $(XPCSHELLTESTDIR)/data/updater$(BIN_SUFFIX)
> +	cp $(FINAL_TARGET)/updater-xpcshell$(BIN_SUFFIX) $(MOCHITESTCHROMEDIR)/data/updater$(BIN_SUFFIX)
> +	cp $(FINAL_TARGET)/updater-xpcshell$(BIN_SUFFIX) $(MOCHITESTBROWSERDIR)/updater$(BIN_SUFFIX)

Please get a build peer to review this and other build changes. :gps or :ted and they can pass it on to someone else if they don't have the time.

Comment 153

[Robert Strong (they/them - no direct email)]

I think these are a little better after thinking about it.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #152)
> ::: toolkit/components/telemetry/Histograms.json:4966
> ::: toolkit/components/telemetry/Histograms.json:4967
<snip>
> > +  "UPDATE_NOTIFICATION_SHOWN": {
> > +    "alert_emails": ["[e-mail]"],
> 
> Please use the following:
> [e-mail]
> 
> This is a distribution list and if you'd like to be added to it just let me
> know.
> 
> ::: toolkit/components/telemetry/Histograms.json:4971
> (Diff revision 20)
> > +  "UPDATE_NOTIFICATION_SHOWN": {
> > +    "alert_emails": ["[e-mail]"],
> > +    "expires_in_version": "60",
> > +    "kind": "categorical",
> > +    "bug_numbers": [893505],
> > +    "description": "Every time we show a doorhanger notification regarding application updates, we log the type of notification here.",
> 
> Please change the descript to:
> Update: the doorhanger type that was displayed.
Update: the application update doorhanger type that was displayed.

> ::: toolkit/components/telemetry/Histograms.json:4979
> (Diff revision 20)
> > +  "UPDATE_NOTIFICATION_DISMISSED": {
> > +    "alert_emails": ["[e-mail]"],
> > +    "expires_in_version": "60",
> > +    "kind": "categorical",
> > +    "bug_numbers": [893505],
> > +    "description": "Every time a user dismisses a doorhanger notification regarding application updates, we log the type of notification here.",
> 
> Please change the description to:
> Update: the doorhanger type that was dismissed.
Update: the application update doorhanger type that was dismissed.

> ::: toolkit/components/telemetry/Histograms.json:4979
> (Diff revision 20)
> > +  "UPDATE_NOTIFICATION_DISMISSED": {
> > +    "alert_emails": ["[e-mail]"],
> > +    "expires_in_version": "60",
> > +    "kind": "categorical",
> > +    "bug_numbers": [893505],
> > +    "description": "Every time a user dismisses a doorhanger notification regarding application updates, we log the type of notification here.",
> 
> Please change the description to:
> Update: the dismiss action was executed for this doorhanger type.
Update: the dismiss action was executed for this application update doorhanger type.

> ::: toolkit/components/telemetry/Histograms.json:4987
> (Diff revision 20)
> > +  "UPDATE_NOTIFICATION_MAIN_ACTION_DOORHANGER": {
> > +    "alert_emails": ["[e-mail]"],
> > +    "expires_in_version": "60",
> > +    "kind": "categorical",
> > +    "bug_numbers": [893505],
> > +    "description": "Every time a user clicks on the main action of doorhanger notification regarding application updates, we log the type of notification here.",
> 
> Please change the description to:
> Update: the main action was initiated for this doorhanger type.
Update: the main update action was initiated for this application update doorhanger type.

> ::: toolkit/components/telemetry/Histograms.json:4995
> (Diff revision 20)
> > +  "UPDATE_NOTIFICATION_MAIN_ACTION_MENU": {
> > +    "alert_emails": ["[e-mail]"],
> > +    "expires_in_version": "60",
> > +    "kind": "categorical",
> > +    "bug_numbers": [893505],
> > +    "description": "Every time a user interacts with the PanelUI menu item for a notification regarding application updates, we log the type of notification here.",
> 
> Please change the description to:
> Update: the action was initiated from the PanelUI menu.
Update: the update action was initiated from the PanelUI application update menu item.

Comment 154

[Romain Testard [:RT]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #152)
> Comment on attachment 8822729 [details]
> Bug 893505 - Simplify the application update UI
> 
> https://reviewboard.mozilla.org/r/100222/#review121858
> 
> Please set promptBadgeWaitTime to 0 on nightly and 96 hours on aurora, beta,
> and release since this is equivelent to badgeWaitTime which is 0 on nightly
> and 96 hours on aurora. If the feedback from RT changes it then just change
> it to those values then.
>


Regarding app.update.promptWaitTime it seems to make sense to keep the same values as before if it was a good compromise.
Regarding app.update.promptBadgeWaitTime I agree with your comment that it makes sense to have it without delay on Nightly. For other channels it's pretty hard to comment without testing (and testing would be hard given the small number of people affected), I hear Bram's argument on Comment 53 about attention fatigue. Given that most users close their browser. 96 hours sounds like a good compromise to me. I'm trying to get a session length distribution report but redash keeps dying on me - I'll update the results here when I get it but likely it will confirm that most users close their browser within 24 hours.

Comment 155

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/20-21/

Comment 156

[Alex Thayer [:alexical] (she/her)]

For the peers I just requested review from, giving a bit of summary here since I know the patch is fairly large.

@rweiss:

We want to add several histograms for the new app update doorhangers we're showing. We just want to understand the basics of how many of each type are showing up and how users interact with them. If trends change quickly as a result of a build, these could let us know that something is wrong.

@gps:

We modified browser/base/moz.build to access several files of the test suite for the old app update popup. These tests are staying around, because we're not removing that system at this time, but we wanted to avoid simply duplicating those files. If there's a better way to accomplish this, please let us know. Additionally, in updater-xpcshell/Makefile.in, we are copying the resulting test updater to our test dir so we can have access to it. Again if there's a better way to accomplish this, feedback would be appreciated!

Comment 157

[Robert Strong (they/them - no direct email)]

Doug, I just tried out the patch and it appears that it implemented the original flow for fullscreen created by Bram and not the updated flow that we agreed on. Specifically, comment #142

Comment 158

[Robert Strong (they/them - no direct email)]

Note: bsmedberg said over irc that never expiring the telemetry is fine as long as there is "a commitment to use/monitor it" which would currently be done by myself and RT.

Comment 159

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/21-22/

Comment 160

[Alex Thayer [:alexical] (she/her)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #157)
> Doug, I just tried out the patch and it appears that it implemented the
> original flow for fullscreen created by Bram and not the updated flow that
> we agreed on. Specifically, comment #142

Should be working correctly now with the latest.

Comment 161

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/22-23/

Comment 162

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/23-24/

Comment 163

[Alex Thayer [:alexical] (she/her)]

I think it was due to me forgetting to remove the observer I added for the fullscreen chrome hide/show. The try build is looking good with that removal added: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba55edbb7eac07cf9da45e22256451efe3c5a46e

Neil, rstrong suggested that you review the UI portions of this before we land it in m-c. Would you mind taking a look?

Comment 164

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/24-25/

Comment 165

[Robert Strong (they/them - no direct email)]

At this point I think any new bugs should be fixed in followup bugs and code review along with changes due to code review is the only thing preventing this from landing.

Comment 166

[Neil Deakin]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review123878

I reviewed the changes in browser/base/content and browser/components/customizableui

Let me know if there's is more you'd like me to look at.

One general comment is that some update management code is happening in browser.js. Did you test this with multiple browser windows open?

::: browser/base/content/browser.js:2793
(Diff revision 25)
> -      Services.obs.removeObserver(this, "update-staged");
> -      Services.obs.removeObserver(this, "update-downloaded");
> -      this.enabled = false;
> +      this.kTopics.forEach(t => {
> +        Services.obs.removeObserver(this, t);
> +      });
>      }
> -    if (this.cancelObserverRegistered) {
> -      Services.obs.removeObserver(this, "update-canceled");
> +
> +    PanelUI.removeNotification(/^update-/);

Maybe just call reset() here.

::: browser/base/content/browser.js:2841
(Diff revision 25)
> +        if (fromDoorhanger) {
> +          Services.telemetry.getHistogramById("UPDATE_NOTIFICATION_MAIN_ACTION_DOORHANGER").add(type);
> -    } else {
> +        } else {
> -      // open the page for manual update
> -      let url = Services.urlFormatter.formatURLPref("app.update.url.manual");
> -      openUILinkIn(url, "tab");
> +          Services.telemetry.getHistogramById("UPDATE_NOTIFICATION_MAIN_ACTION_MENU").add(type);
> +        }
> +        callback();

I'm not clear what's going on with this nested reference to 'callback'. Maybe one should be renamed?

::: browser/base/content/browser.js:2925
(Diff revision 25)
> +
> +        if (badgeWaitTimeMs < doorhangerWaitTimeMs) {
> +          this.addTimeout(badgeWaitTimeMs, () => {
> +            this.showRestartNotification(true);
> +
> +            let remainingTime = doorhangerWaitTimeMs - badgeWaitTimeMs;

The duration being computed here seems to imply that addTimeout takes an interval between subsequent callbacks but it doesn't work that way.

::: browser/base/content/browser.js:2950
(Diff revision 25)
> +        this.showUpdateAvailableNotification(update, false);
> +        break;
> +    }
>    },
>  
> -  notify() {
> +  handleUpdateStaged(update, status) {

Is handleUpdateStaged going to be something different than just redirecting to another function?

::: browser/base/content/test/appUpdate/head.js:182
(Diff revision 25)
> +    const topic = yield Promise.race(promises);
> +
> +    is(topic, `panelUI-${notificationId}`, "The right notification showed up.");
> +    if (topic != `panelUI-${notificationId}`) {
> +      if (cleanup) {
> +        yield Task.spawn(cleanup);

You shouldn't need a task here. Just yield cleanup() or maybe yield* cleanup()

::: browser/base/content/test/appUpdate/head.js:198
(Diff revision 25)
> +    let buttonEl = document.getAnonymousElementByAttribute(notification, "anonid", button);
> +
> +    buttonEl.click();
> +
> +    if (cleanup) {
> +      yield Task.spawn(cleanup);

Same here.

::: browser/base/content/test/appUpdate/head.js:216
(Diff revision 25)
> + * @param  value
> + *         The value to temporarily give the preference.
> + * @param  prefBranch
> + *         Optional pref branch to use.
> + */
> +function pushPref(type, name, value, prefBranch) {

You should just use SpecialPowers.pushPrefEnv here. You also don't need the popping since pushPrefEnv does that at the end of the test automatically.

::: browser/base/content/test/appUpdate/head.js:298
(Diff revision 25)
> + *         The url to wait for.
> + * @return A promise which will resolve when a window with the specified url
> + *         is loaded. If another url is loaded, this will result in an
> + *         assertion failure.
> + */
> +function waitForWindow(url) {

I don't see where this is used.

::: browser/components/customizableui/content/panelUI.js:59
(Diff revision 25)
>      this.menuButton.addEventListener("mousedown", this);
>      this.menuButton.addEventListener("keypress", this);
>      this._overlayScrollListenerBoundFn = this._overlayScrollListener.bind(this);
> +
> +    Services.obs.addObserver(this, "fullscreen-nav-toolbox", false);
> +    window.addEventListener("fullscreen", this);

This listener doesn't seem to get removed in uninit.

::: browser/components/customizableui/content/panelUI.js:187
(Diff revision 25)
> +    let existingIndex = this.notifications.findIndex(n => n.id == id);
> +    if (existingIndex != -1) {
> +      this.notifications.splice(existingIndex, 1);
> +    }
> +
> +    // we don't want to clobber doorhanger notifications just to show a badge,

Capitalize comments sentences in a few places here.

::: browser/components/customizableui/content/panelUI.js:295
(Diff revision 25)
> +    let panelState = this.notificationPanel.state;
> +
> +    return panelState == "showing" || panelState == "open";
> +  },
> +
> +  get activeNotification() {

The activeNotification doesn't seem to be used in this patch.

::: browser/components/customizableui/content/panelUI.js:645
(Diff revision 25)
> +    }
> +
> +    let doorhangers =
> +      this.notifications.filter(n => !n.dismissed && !n.options.badgeOnly);
> +
> +    if (this.panel.state == "showing" || this.panel.state == "open") {

I'm not sure I completely understand the logic here. _updateNotifications is called when fullscreen is entered or a button is pressed; we want to close any open popup notification in all these cases?

So if two notifications come in, we don't show either as popups?

::: browser/components/customizableui/content/panelUI.js:704
(Diff revision 25)
> +    popupnotification.setAttribute("buttoncommand", "PanelUI._onNotificationButtonEvent(event, 'buttoncommand');");
> +    popupnotification.setAttribute("secondarybuttoncommand", "PanelUI._onNotificationButtonEvent(event, 'secondarybuttoncommand');");
> +
> +    popupnotification.notification = notification;
> +
> +    this.notificationPanel.appendChild(popupnotification);

popupnotification is already inside the panel, no?

::: browser/components/customizableui/content/panelUI.js:727
(Diff revision 25)
> +      menuItem.notification = notification;
> +      menuItem.setAttribute("oncommand", "PanelUI._onNotificationMenuItemSelected(event)");
> +      menuItem.classList.add("PanelUI-notification-menu-item");
> +      menuItem.hidden = false;
> +      menuItem.fromPanelUINotifications = true;
> +      this._notify(notification.id, "menu-item-shown");

You have a lot of observer notifications (_notify) being added that are not being listened to. Unless there is a reason to add them, they shouldn't be here.

You should generally only use observers when some global state has changed.

::: browser/components/customizableui/content/panelUI.js:733
(Diff revision 25)
> +    }
> +
> +    this._activeNotification = notification;
> +  },
> +
> +  _clearBadges() {

For consistency with _showBadge, call this _clearBadge.

::: browser/components/customizableui/content/panelUI.js:783
(Diff revision 25)
> +    let dismiss = true;
> +    if (action) {
> +      try {
> +        if (action === notification.mainAction) {
> +          const fromDoorhanger = true;
> +          action.callback(fromDoorhanger);

Why is this written this way instead of just passing true?

::: browser/components/customizableui/content/panelUI.js:813
(Diff revision 25)
> +
> +    event.stopPropagation();
> +
> +    try {
> +      const fromDoorhanger = false;
> +      target.notification.mainAction.callback(fromDoorhanger);

Similar here

::: browser/components/customizableui/content/panelUI.js:827
(Diff revision 25)
> +
> +  _getPopupId(notification) { return "PanelUI-" + notification.id + "-notification"; },
> +
> +  _getBadgeStatus(notification) { return notification.id; },
> +
> +  _getMenuItemId(notification) { return "PanelUI-" + notification.id + "-menu-item"; },

These aren't actually menuitems though so the naming here confused me a bit.

Comment 167

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review124164

Ted asked me to take a look at this, so stealing his review for the moz.build and Makefile.in changes, which both look good to me. 

Duplicating the test updater via that Makefile.in is sort of unfortunate, but I looked into it recently, and didn't see a clean replacement for all that in moz.build.

Comment 168

[[e-mail]]

For data review:

1) How long will these histograms be collected?

2) Are you intending for these to be opt-out on release?

3) Who will be looking at this data and making decisions? (Who is the data analyst, essentially)  

4) What tooling will you be using to look at this data? t.m.o, s.t.m.o, a.t.m.o., something else, etc.

(In reply to Doug Thayer [:dthayer] from comment #156)
> For the peers I just requested review from, giving a bit of summary here
> since I know the patch is fairly large.
> 
> @rweiss:
> 
> We want to add several histograms for the new app update doorhangers we're
> showing. We just want to understand the basics of how many of each type are
> showing up and how users interact with them. If trends change quickly as a
> result of a build, these could let us know that something is wrong.
> 
> @gps:
> 
> We modified browser/base/moz.build to access several files of the test suite
> for the old app update popup. These tests are staying around, because we're
> not removing that system at this time, but we wanted to avoid simply
> duplicating those files. If there's a better way to accomplish this, please
> let us know. Additionally, in updater-xpcshell/Makefile.in, we are copying
> the resulting test updater to our test dir so we can have access to it.
> Again if there's a better way to accomplish this, feedback would be
> appreciated!

(In reply to Doug Thayer [:dthayer] from comment #156)
> For the peers I just requested review from, giving a bit of summary here
> since I know the patch is fairly large.
> 
> @rweiss:
> 
> We want to add several histograms for the new app update doorhangers we're
> showing. We just want to understand the basics of how many of each type are
> showing up and how users interact with them. If trends change quickly as a
> result of a build, these could let us know that something is wrong.
> 
> @gps:
> 
> We modified browser/base/moz.build to access several files of the test suite
> for the old app update popup. These tests are staying around, because we're
> not removing that system at this time, but we wanted to avoid simply
> duplicating those files. If there's a better way to accomplish this, please
> let us know. Additionally, in updater-xpcshell/Makefile.in, we are copying
> the resulting test updater to our test dir so we can have access to it.
> Again if there's a better way to accomplish this, feedback would be
> appreciated!

Comment 169

[Alex Thayer [:alexical] (she/her)]

(In reply to Rebecca Weiss from comment #168)
> For data review:
> 
> 1) How long will these histograms be collected?
> 
> 2) Are you intending for these to be opt-out on release?
> 
> 3) Who will be looking at this data and making decisions? (Who is the data
> analyst, essentially)  
> 
> 4) What tooling will you be using to look at this data? t.m.o, s.t.m.o,
> a.t.m.o., something else, etc.

I'm going to NI rstrong to validate this, but:

1) They should be collected indefinitely. Since many things can conceivably go wrong with update, and it's a very critical system, data about what users are actually seeing is vital and will continue to be vital. If we see a sharp difference in the number of update prompts shown between two releases that should be similar, for instance, then that's a good sign that something is wrong.

2) Correct.

3) Someone on the Firefox Core Engineering team, Most likely rstrong.

4) Most likely a.t.m.o., though they may be a part of an update dashboard on t.m.o.

Comment 170

[Robert Strong (they/them - no direct email)]

(In reply to Rebecca Weiss from comment #168)
> For data review:
> 
> 1) How long will these histograms be collected?
Indefinitely. I ok'd this with bsmedberg already. This is in support update orphaning and our goals are very difficult to reach for a multitude of reasons with this being one of the many.

> 2) Are you intending for these to be opt-out on release?
Definitely. The same as with other app update histograms.
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json#4649

> 
> 3) Who will be looking at this data and making decisions? (Who is the data
> analyst, essentially)  
developers that work on app update. rstrong mhowell
> 
> 4) What tooling will you be using to look at this data? t.m.o, s.t.m.o,
> a.t.m.o., something else, etc.
I typically use a.t.m.o. but I use all of the ones you listed. This *might* end up in a dashboard as well such as
https://telemetry.mozilla.org/update-orphaning/

Comment 171

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/25-26/

Comment 172

[Alex Thayer [:alexical] (she/her)]

Thanks for the review, Neil!

(In reply to Neil Deakin from comment #166)
> One general comment is that some update management code is happening in
> browser.js. Did you test this with multiple browser windows open?

Added tests to cover this in a few locations. Required adding some observers and notifications to panelUI.js to coordinate, and I moved the update management code in browser.js to nsUpdateService.js.

> The duration being computed here seems to imply that addTimeout takes an
> interval between subsequent callbacks but it doesn't work that way.

Added a comment in the code since the intent may have not been clear.

> ::: browser/components/customizableui/content/panelUI.js:645
> (Diff revision 25)
> > +    }
> > +
> > +    let doorhangers =
> > +      this.notifications.filter(n => !n.dismissed && !n.options.badgeOnly);
> > +
> > +    if (this.panel.state == "showing" || this.panel.state == "open") {
> 
> I'm not sure I completely understand the logic here. _updateNotifications is
> called when fullscreen is entered or a button is pressed; we want to close
> any open popup notification in all these cases?
> 
> So if two notifications come in, we don't show either as popups?

I think you might be misreading this.panel as this.notificationPanel. This is checking to see if the hamburger menu is dropped down.

> ::: browser/components/customizableui/content/panelUI.js:827
> (Diff revision 25)
> > +
> > +  _getPopupId(notification) { return "PanelUI-" + notification.id + "-notification"; },
> > +
> > +  _getBadgeStatus(notification) { return notification.id; },
> > +
> > +  _getMenuItemId(notification) { return "PanelUI-" + notification.id + "-menu-item"; },
> 
> These aren't actually menuitems though so the naming here confused me a bit.

I'm not sure what you mean? They are entries in the hamburger menu panel. Menu item was the best term I could come up with. Is it taken by something else?

Comment 173

[Robert Strong (they/them - no direct email)]

I landed the updated patch and some tests are failing
https://treeherder.mozilla.org/#/jobs?repo=oak

Comment 174

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/26-27/

Comment 175

[Robert Strong (they/them - no direct email)]

Looks like the tests are passing with the new patch.

Comment 176

[Robert Strong (they/them - no direct email)]

With chmanchester's review you can remove the review request for Ted.

Comment 177

[Robert Strong (they/them - no direct email)]

Rebecca, are the responses in comment #170 enough for you to finish the review. I am sure bsmedberg can confirm over irc that it is ok for these probes to never expire. Thanks!

Comment 178

[Neil Deakin]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review127628

::: browser/base/content/test/appUpdate/downloadPage.html:1
(Diff revision 27)
> +<!DOCTYPE html>

The files:

browser/base/content/test/appUpdate/downloadPage.html
browser/base/content/test/appUpdate/head.js
browser/components/customizableui/content/panelUI.inc.xul

don't end in a blank line. This shows as a warning when you view the diff itself.

::: browser/base/content/test/appUpdate/head.js:151
(Diff revision 27)
> +      set: [
> +        [PREF_APP_UPDATE_DOWNLOADPROMPTATTEMPTS, 0],
> +        [PREF_APP_UPDATE_ENABLED, true],
> +        [PREF_APP_UPDATE_IDLETIME, 0],
> +        [PREF_APP_UPDATE_URL_MANUAL, URL_MANUAL_UPDATE],
> +        [PREF_APP_UPDATE_ENABLED, true],

setting PREF_APP_UPDATE_ENABLED is duplicated

::: browser/components/customizableui/content/panelUI.js:265
(Diff revision 27)
> +    switch (topic){
> +      case "fullscreen-nav-toolbox":
> +        this._updateNotifications();
> +        break;
> +      case "panelUI-notification-main-action":
> +        if (subject != this.uuid) {

OK, so this is meant to dismiss the notifications in all other windows except for this one?

I think you could just use the panelUI object itself or the window rather than needing a uuid.

::: browser/components/customizableui/content/panelUI.js:286
(Diff revision 27)
> +      // It's possible for the doorhanger to not exist by the time we receive this event.
> +      // This is very unlikely in actual usage scenarios, but it shows up in tests with
> +      // multiple windows since dismissing a doorhanger in one window will by design
> +      // dismiss it in all windows.
> +      if (doorhanger && aEvent.type == "popupshown") {
> +        this._notify(doorhanger.id, "doorhanger-shown");

This appears to only be used for a test. Why don't you just listen for the popupshown event in the test?

::: browser/themes/shared/customizableui/panelUI.inc.css:461
(Diff revision 27)
>  }
>  
>  #PanelUI-multiView[viewtype="subview"] > .panel-viewcontainer > .panel-viewstack > .panel-mainview >  #PanelUI-mainView {
>    background-color: var(--arrowpanel-dimmed);
>  }
>  

I suspect that this was written this way as some optimization. (The 'a b' selector is slower than 'a > b'). Might ask the original author here.

Comment 179

[Neil Deakin]

> I'm not sure what you mean? They are entries in the hamburger menu panel.
> Menu item was the best term I could come up with. Is it taken by something
> else?

It doesn't matter too much but menuitems refer to items on the menubar or contextmenu. It confused me into what UI was being referred to.

Comment 180

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/27-28/

Comment 181

[Alex Thayer [:alexical] (she/her)]

(In reply to Neil Deakin from comment #179)
> > I'm not sure what you mean? They are entries in the hamburger menu panel.
> > Menu item was the best term I could come up with. Is it taken by something
> > else?
> 
> It doesn't matter too much but menuitems refer to items on the menubar or
> contextmenu. It confused me into what UI was being referred to.

Hmm, alright. I'm happy to change it, though I'm struggling to come up with a good name. panel-item, panel-row, menu-row?

Comment 182

[Robert Strong (they/them - no direct email)]

Doug, could you update the patch to current m-c?

Comment 183

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/28-29/

Comment 184

[Neil Deakin]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review128062

::: browser/base/content/test/appUpdate/head.js:231
(Diff revision 29)
> + * @param  eventName
> + *         String name of the event.
> + * @return A promise which will resolve the first time an event occurs on the
> + *         specified target.
> + */
> +function waitForEventOnTarget(target, eventName) {

Just use BrowserTestUtils.waitForEvent

Comment 185

[Neil Deakin]

> Hmm, alright. I'm happy to change it, though I'm struggling to come up with
> a good name. panel-item, panel-row, menu-row?

I wouldn't worry about changing it right now. Maybe just add a comment somewhere describing which UI it is referring to and where it physically appears.

Comment 186

[:Gijs (he/him)]

I'm fairly sure Neil didn't mean to close this as fixed.

Comment 187

[Neil Deakin]

Indeed. It is the irritating 1350863.

Comment 188

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/29-30/

Comment 189

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Changing the review request to bsmedberg since rweiss is on PTO

Comment 190

[Benjamin Smedberg]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review129038

data-r=me (I did not review the code)

Comment 191

[Benjamin Smedberg]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

argh RB

Comment 192

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review129846

I think anything else can be handled in followup bugs. It would be a good thing if you went through the open issues in review board to make sure they've all been addressed or don't need addressing.

::: browser/base/moz.build:19
(Diff revision 30)
>      'content/test/chrome/chrome.ini',
>  ]
>  
>  BROWSER_CHROME_MANIFESTS += [
>      'content/test/alerts/browser.ini',
> +    'content/test/appUpdate/browser.ini',

These tests should only run when MOZ_UPDATER is defined as follows

if CONFIG['MOZ_UPDATER']:
    BROWSER_CHROME_MANIFESTS += ['content/test/appUpdate/browser.ini']

Comment 193

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/30-31/

Comment 194

[Alex Thayer [:alexical] (she/her)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #192)
> I think anything else can be handled in followup bugs. It would be a good
> thing if you went through the open issues in review board to make sure
> they've all been addressed or don't need addressing.

There's one issue left about rethinking the names for some of the variables in testConstants.js (like REL_PATH_DATA). Have any further thoughts on that? Otherwise everything was addressed.

Comment 195

[Sylvestre Ledru [:Sylvestre]]

Is there a test plan for this change? If yes, could you share the link?
Thanks!

Comment 196

[Sylvestre Ledru [:Sylvestre]]

Ryan told me about https://wiki.mozilla.org/QA/Application_Update_UI
sorry :/

Comment 197

[Alex Thayer [:alexical] (she/her)]

Oh no problem, there's also an informal test plan that rstrong wrote up in an email which might be more helpful:

> 1 "Not Now" button
> 1.01 Install a Firefox build that contains the patches for bug 893505 and has an update
> 1.02 Launch Firefox
> 1.03 Open about:config
> 1.04 Change app.update.badgeWaitTime to 10
> 1.05 Change app.update.lastUpdateTime.background-update-timer to 1
> 1.06 Change app.update.promptWaitTime to 30
> 1.07 Change app.update.timerMinimumDelay to 10
> 1.08 Exit Firefox
> 1.09 Launch Firefox
> 1.10 In approximately a minute or two the hanburger menu badge should be displayed
> 1.11 In approximately 20 more seconds the doorhanger ui should be displayed
> 1.12 Clicking "Not Now" in the doorhanger ui should dismiss the doorhanger ui
> 1.13 The hamburger menu should display the badge
> 1.14 Clicking the hamburger menu should display "Restart appname to apply update" towards the bottom
> 1.15 Clicking the "Restart Appname to apply update" in the hamburger menu should restart and apply the update
> 
> 2 "Restart and Restore" button
> 2.01 Install a Firefox build that contains the patches for bug 893505 and has an update
> 2.02 Launch Firefox
> 2.03 Open about:config
> 2.04 Change app.update.badgeWaitTime to 10
> 2.05 Change app.update.lastUpdateTime.background-update-timer to 1
> 2.06 Change app.update.promptWaitTime to 30
> 2.07 Change app.update.timerMinimumDelay to 10
> 2.08 Exit Firefox
> 2.09 Launch Firefox
> 2.10 In approximately a minute or two the hanburger menu badge should be displayed
> 2.11 In approximately 20 more seconds the doorhanger ui should be displayed
> 2.12 Clicking the "Restart and Restore" in the doorhanger ui should restart and apply the update
> 
> 3 Fullscreen
> 3.01 Install a Firefox build that contains the patches for bug 893505 and has an update
> 3.02 Launch Firefox
> 3.03 Open about:config
> 3.04 Change app.update.badgeWaitTime to 10
> 3.05 Change app.update.lastUpdateTime.background-update-timer to 1
> 3.06 Change app.update.promptWaitTime to 30
> 3.07 Change app.update.timerMinimumDelay to 10
> 3.08 Exit Firefox
> 3.09 Launch Firefox
> 3.10 Press F11 on Windows (other platforms may be different) as soon as possible
> 3.11 Wait the approximate amount of time it has taken for the doorhanger ui to display in the previous tests
> 3.12 Move the mouse pointer at the top of the screen
> 3.13 The browser should display additional ui
> 3.14 The doorhanger ui should be displayed. If it isn't then start from the beginning and wait for a longer period in step 3.11
> 3.15 Click the "Not Now" button (see Note below)
>      Note: For now click the hanburger menu since this is currently not possible. https://bugzilla.mozilla.org/show_bug.cgi?id=1348396
> 3.16 Move the mouse pointer away from the top of the screen so it goes back into fullscreen
> 3.17 Move the mouse pointer at the top of the screen
> 3.18 The hamburger menu badge should be displayed
> 
> 4 "Download Appname" - failed to apply update
> 4.01 Install a Firefox build that contains the patches for bug 893505 and has an update
> 4.02 Launch Firefox
> 4.03 Open about:config
> 4.04 Change app.update.badgeWaitTime to 10
> 4.05 Change app.update.lastUpdateTime.background-update-timer to 1
> 4.06 Change app.update.promptWaitTime to 30
> 4.07 Change app.update.staging.enabled to false
> 4.08 Change app.update.timerMinimumDelay to 10
> 4.09 Change devtools.chrome.enabled to true
> 4.10 Exit Firefox
> 4.11 Launch Firefox
> 4.12 Open Tools -> Web Developer -> Browser Console
> 4.13 Copy and paste the line below that starts with 'Services.prefs' into the textbox at the bottom of the Browser Console and press return
>      Note: this must be done as soon as possible and the textbox should already have focus so just pasting should work.
>      Services.prefs.getDefaultBranch(null).setCharPref("app.update.url", "http://exchangecode.com/robert/work/snippets/doorhanger.xml");
> 4.14 In approximately a minute or two the hanburger menu badge should be displayed
> 4.15 In approximately 20 more seconds the doorhanger ui should be displayed
> 4.16 Clicking the "Restart and Restore" in the doorhanger ui should restart and the doorhanger should display with "Appname can't update to the latest version"
> 4.17 Clicking "Download Appname" should open a web page that has a download for this channel
> 4.18 The doorhanger ui and hamburger menu badge should not be displayed
> 
> 5 "Download Appname" - failed to elevate multiple times
>   Note: this requires that Firefox is installed into a location that requires elevation such as C:\Program Files\Nightly\ or C:\Program Files\Nightly (x86)\ which are the defaults, that the system has UAC enabled which is the default, and the Windows account must be a member of the Adminstrators group which is the default.
> 5.01 Install a Firefox build that contains the patches for bug 893505 and has an update
> 5.02 Launch Firefox
> 5.03 Open about:config
> 5.04 Change app.update.badgeWaitTime to 10
> 5.05 Change app.update.lastUpdateTime.background-update-timer to 1
> 5.06 Change app.update.promptWaitTime to 30
> 5.07 Change app.update.service.enabled to false
> 5.08 Change app.update.timerMinimumDelay to 10
> 5.09 Exit Firefox
> 5.10 Launch Firefox
> 5.11 In approximately a minute or two the hanburger menu badge should be displayed
> 5.12 In approximately 20 more seconds the doorhanger ui should be displayed
> 5.13 Clicking the "Restart and Restore" in the doorhanger ui should restart
> 5.14 Click No in the Windows UAC prompt
> 5.15 Repeat steps 6.16 and 6.17 until the doorhanger ui is displayed
> 5.16 The doorhanger should display with "Appname can't update to the latest version"
> 5.17 Clicking "Not Now" should dismiss the doorhanger
> 5.18 Clicking the hamburger menu should display "Download a fresh copy of Appname" towards the bottom
> 5.19 Clicking "Download a fresh copy of Appname" should open a web page that has a download for this channel
> 5.20 The doorhanger ui and hamburger menu badge should not be displayed
> 
> 6 "Download Appname" - no permissions to update
>   Note: this requires that Firefox is installed into a location that requires elevation such as C:\Program Files\Nightly\ or C:\Program Files\Nightly (x86)\ which are the defaults, that the system has UAC enabled which is the default, and the Windows account must be a member of Users and not a member of the Adminstrators group which is not the default.
> 6.01 Install a Firefox build that contains the patches for bug 893505 and has an update
> 6.02 Launch Firefox
> 6.03 Open about:config
> 6.04 Change app.update.badgeWaitTime to 10
> 6.05 Change app.update.lastUpdateTime.background-update-timer to 1
> 6.06 Change app.update.promptWaitTime to 30
> 6.07 Change app.update.service.enabled to false
> 6.08 Change app.update.timerMinimumDelay to 10
> 6.09 Exit Firefox
> 6.10 Launch Firefox
> 6.11 In approximately a minute or two the hanburger menu badge should be displayed
> 6.12 In approximately 20 more seconds the doorhanger ui should be displayed with "Appname can't update to the latest version"
> 6.13 Clicking "Download Appname" should open a web page that has a download for this channel
> 6.14 The doorhanger ui and hamburger menu badge should not be displayed
> 
> 7 Video Fullscreen
> 7.01 Install a Firefox build that contains the patches for bug 893505 and has an update
> 7.02 Launch Firefox
> 7.03 Open about:config
> 7.04 Change app.update.badgeWaitTime to 10
> 7.05 Change app.update.lastUpdateTime.background-update-timer to 1
> 7.06 Change app.update.promptWaitTime to 30
> 7.07 Change app.update.timerMinimumDelay to 10
> 7.08 Exit Firefox
> 7.09 Launch Firefox
> 7.10 Open a youtube video (e.g. https://www.youtube.com/watch?v=mjMLbb-RaRI& ) and click the fullscreen video button as soon as possible
> 7.11 Wait the approximate amount of time it has taken for the doorhanger ui to display in the previous tests
> 7.12 Press escape to exit fullscreen video
> 7.13 The doorhanger ui should be displayed. If it isn't then start from the beginning and wait for a longer period in step 3.11
> 7.15 Click the "Not Now" button
> 7.16 The doorhanger ui should no longer be displayed and the hamburger menu badge should be displayed
> 7.17 Click the fullscreen video button
> 7.18 Press escape to exit fullscreen video
> 7.19 The hamburger menu badge should be displayed

Comment 198

[Robert Strong (they/them - no direct email)]

(In reply to Doug Thayer [:dthayer] from comment #194)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #192)
> > I think anything else can be handled in followup bugs. It would be a good
> > thing if you went through the open issues in review board to make sure
> > they've all been addressed or don't need addressing.
> 
> There's one issue left about rethinking the names for some of the variables
> in testConstants.js (like REL_PATH_DATA). Have any further thoughts on that?
> Otherwise everything was addressed.
That is fine to leave as is and if I come up with something I'll handle it in a different bug.

Just noticed this still has a review request for rweiss which is not needed with bsmedberg's review so please remove it.

Comment 199

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/31-32/

Comment 200

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3246d7cb2729
Simplify the application update UI. r=Gijs, r=enndeakin, r=rstrong, data-r=bsmedberg

Comment 201

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5880d471d75c
Followup eslint fix for Bug 893505. r=me

Comment 202

[Robert Strong (they/them - no direct email)]

Had to push a followup fix for recent eslint changes
https://hg.mozilla.org/integration/mozilla-inbound/rev/5880d471d75caf7
ca6ce8d0848737a78131151da

Comment 203

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This still failed after rstrong's follow-up, so it got backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f3b59dd91acce050b9a3aa096731b49c8d9bfd7

Follow-up with failure: https://treeherder.mozilla.org/logviewer.html#?job_id=89338648&repo=mozilla-inbound
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=89338648&repo=mozilla-inbound

[task 2017-04-06T22:56:08.059572Z] An error occurred running eslint. Please check the following error messages:
[task 2017-04-06T22:56:08.059611Z] 
[task 2017-04-06T22:56:08.059645Z] ENOENT: no such file or directory, open '/home/worker/checkouts/gecko/browser/base/content/test/appUpdate/sharedUpdateXML.js'
[task 2017-04-06T22:56:08.059679Z] Error: ENOENT: no such file or directory, open '/home/worker/checkouts/gecko/browser/base/content/test/appUpdate/sharedUpdateXML.js'
[task 2017-04-06T22:56:08.059695Z]     at Error (native)
[task 2017-04-06T22:56:08.059710Z]     at Object.fs.openSync (fs.js:640:18)
[task 2017-04-06T22:56:08.059726Z]     at Object.fs.readFileSync (fs.js:508:33)
[task 2017-04-06T22:56:08.059757Z]     at Object.getGlobalsForFile (/home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js:150:22)
[task 2017-04-06T22:56:08.059787Z]     at Object.BlockComment (/home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js:98:27)
[task 2017-04-06T22:56:08.059820Z]     at helpers.walkAST (/home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js:186:39)
[task 2017-04-06T22:56:08.059851Z]     at sendCommentEvents (/home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:130:9)
[task 2017-04-06T22:56:08.059881Z]     at Controller.enter (/home/worker/checkouts/gecko/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js:144:9)
[task 2017-04-06T22:56:08.059918Z]     at Controller.__execute (/home/worker/checkouts/gecko/node_modules/estraverse/estraverse.js:397:31)
[task 2017-04-06T22:56:08.059949Z]     at Controller.traverse (/home/worker/checkouts/gecko/node_modules/estraverse/estraverse.js:501:28)
[task 2017-04-06T22:56:08.064580Z]

Comment 204

[Robert Strong (they/them - no direct email)]

Aryx, thanks for the backout.

Doug, it looks like recent changes to eslint are breaking this patch :(

Comment 205

[Alex Thayer [:alexical] (she/her)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #204)
> Aryx, thanks for the backout.
> 
> Doug, it looks like recent changes to eslint are breaking this patch :(

Dang. Is there documentation of the eslint changes somewhere just so I can understand what's going on? Why is it looking for appUpdate/sharedUpdateXML.js?

Comment 206

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/32-33/

Comment 207

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review130102

lgtm and also tested locally

Comment 208

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4b635aa0c2e
Simplify the application update UI. r=Gijs, r=enndeakin, r=rstrong, data-r=bsmedberg

Comment 209

[Carsten Book [:Tomcat]]

had to backout this for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=89547914&repo=mozilla-inbound after the m-c to m-i

Comment 210

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7178a5e18eb7
Backed out changeset d4b635aa0c2e for eslint failure

Comment 211

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100222/diff/33-34/

Comment 212

[Robert Strong (they/them - no direct email)]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review130520

Comment 213

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/898a2071de2d
Simplify the application update UI r=chmanchester,enndeakin+6102,Gijs,rstrong

Comment 214

[Francesco Lodolo [:flod]]

Comment on attachment 8822729 [details]
Bug 893505 - Simplify the application update UI

https://reviewboard.mozilla.org/r/100222/#review130562

::: browser/locales/en-US/chrome/browser/browser.dtd:904
(Diff revision 34)
>  <!ENTITY emeLearnMoreContextMenu.label            "Learn more about DRM…">
>  <!ENTITY emeLearnMoreContextMenu.accesskey        "D">
> +
> +<!ENTITY updateAvailable.message "Update your &brandShorterName; for the latest in speed and privacy.">
> +<!ENTITY updateAvailable.whatsnew.label "See what’s new.">
> +<!ENTITY updateAvailable.whatsnew.href "http://www.mozilla.org/">

This really shouldn't be http://

More important, why are you exposing this as a localizable value? I'm quite sure you don't want people to change it

::: browser/locales/en-US/chrome/browser/browser.dtd:914
(Diff revision 34)
> +<!ENTITY updateAvailable.cancelButton.accesskey "N">
> +<!ENTITY updateAvailable.panelUI.label "Download &brandShorterName; update">
> +
> +<!ENTITY updateManual.message "Download a fresh copy of &brandShorterName; and we’ll help you to install it.">
> +<!ENTITY updateManual.whatsnew.label "See what’s new.">
> +<!ENTITY updateManual.whatsnew.href "http://www.mozilla.org/">

Same issue here

::: browser/locales/en-US/chrome/browser/browser.dtd:923
(Diff revision 34)
> +<!ENTITY updateManual.cancelButton.label "Not Now">
> +<!ENTITY updateManual.cancelButton.accesskey "N">
> +<!ENTITY updateManual.panelUI.label "Download a fresh copy of &brandShorterName;">
> +
> +<!ENTITY updateRestart.message "After a quick restart, &brandShorterName; will restore all your open tabs and windows.">
> +<!ENTITY updateRestart.header.message "Restart &brandShorterName; to apply update.">

As a non native English speaker, this sounds strange (would expect "to apply the update"). Same later.

Comment 215

[Robert Strong (they/them - no direct email)]

Filed bug 1354774 for the issue with the urls.

Bram, can you comment on the last issue in comment #214 regarding "Restart &brandShorterName; to apply update." If you think this should be changed either you or I can file a new bug.

Comment 216

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/898a2071de2d

Comment 217

[Jorg K (CEST = GMT+2)]

This has busted Thunderbird builds. As Gijs pointed out in bug 1354866 comment #20, there is this strange mix of a toolkit/ makefile manipulating browser/ tests:
https://hg.mozilla.org/mozilla-central/rev/898a2071de2d#l50.15

Comment 218

[Bram Pitoyo [:bram]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #215)
> Bram, can you comment on the last issue in comment #214 regarding "Restart
> &brandShorterName; to apply update." If you think this should be changed
> either you or I can file a new bug.

I agree with :flod. We should change the string to “Restart &brandShorterName; to apply the update.”

I’ve filed bug 1354949 to resolve this issue.

Comment 219

[Sylvestre Ledru [:Sylvestre]]

Doug, well done for this great work!
bravo!

Comment 220

[Pascal Chevrel:pascalc]

Release Note Request (optional, but appreciated)
[Why is this notable]: The application update UI was modernized
[Affects Firefox for Android]: no
[Suggested wording]: The application update UI was modernized to be less intrusive and more informative for users using an outdated version of Firefox.
[Links (documentation, blog post, etc)]:

Comment 221

[Julien Cristau [:jcristau]]

adding to 55beta release notes