1599360 - Factor out appUpdater from aboutDialog-appUpdater.js into a jsm module

Status: RESOLVED FIXED

(Toolkit :: Application Update, task, P1)

Description

[Drew Willcoxon :adw]

We're writing an experiment add-on that needs to check for updates and get the current update status. There doesn't seem to be a nice robust way to do that except for this nice appUpdater class in aboutDialog-appUpdater.js. But it's a little coupled to the specific UI in the About window and preferences. So I've factored it out into a jsm that we can use, and I updated aboutDialog-appUpdater.js to use it. Instead of calling selectPanel, the jsm class has a listener system. It's pretty straightforward. The existing tests in https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/tests/browser seem to cover this well, so I'm relying on those and I didn't write any new ones.

Comment 1

[Drew Willcoxon :adw]

Attachment: Bug 1599360 - Factor out appUpdater from aboutDialog-appUpdater.js into a jsm module

@rstrong Would you mind reviewing or redirecting please? We're writing an experiment add-on that needs to check for updates and get the current update status. There doesn't seem to be a nice robust way to do that except for this nice appUpdater class in aboutDialog-appUpdater.js. But it's a little coupled to the specific UI in the About window and preferences. So I've factored it out into a jsm that we can use, and I updated aboutDialog-appUpdater.js to use it. Instead of calling selectPanel, the jsm class has a listener system. It's pretty straightforward. The existing tests in https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/tests/browser seem to cover this well, so I'm relying on those and I didn't write any new ones.

We're running our experiment on 72, so we need to uplift this. Would that be OK? If not, then we could put this new appUpdater implementation (which uses this new AppUpdater jsm) behind a pref, and pref it off for 72. We would want to run the tests in https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/tests/browser against the new implementation on 72 though, so we would need to add a new browser.ini that would run the same tests with the pref on.

Comment 2

[Drew Willcoxon :adw]

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

Comment 3

[Marco Bonardo [:mak]]

We are in 72 yet, the question is more likely whether we need this in 71, and I think not? My original question was whether we needed an uplift at all.
That said, this may still be risky for 72. If the approach looks good, we could land only the module in 72, so we can use it in the experiments; as soon as we merge to 73 we land the remaining part of the patch that changes consumers to use it.

Comment 4

[Drew Willcoxon :adw]

You're right, I misread the calendar. We don't need this in 71. I commented in phabricator to make sure it's clear.

We could land only the jsm in 72. That would be nice and easy. But we should have test coverage imo, and I don't mind doing the work to pref off a new implementation if the updater team is OK with that.

Another try push that should fix the "UpdateUtils is not defined" errors on try (which I can't reproduce locally...): https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbe4f63396509fe53385cb15c5a5ad6bac16aded

Comment 5

[Marco Bonardo [:mak]]

Just thinking; if we have test coverage in 73, and we only have the jsm in 72, the risk is limited to our experiment, you could keep a patch to apply to 72 and run tests there for our purposes.

Comment 6

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

If the changes can be limited to only the experiment for 72 I would very much prefer that since it is late in the cycle.

Comment 7

[Drew Willcoxon :adw]

Attachment: Bug 1599360 - Use AppUpdater.jsm in aboutDialog-appUpdater.js when preffed on, and use the current implementation when preffed off

Summary of changes:

• Copy aboutDialog-appUpdater.js to a new aboutDialog-appUpdater-legacy.js file
• Update aboutDialog-appUpdater.js: Rewrite it to use the new AppUpdater.jsm when the browser.aboutDialogNewAppUpdater pref is true, and load aboutDialog-appUpdater-legacy.js otherwise
• In toolkit/mozapps/update/tests/browser, add new browser.legacy.ini and browser.legacy.bits.ini files that do not set browser.aboutDialogNewAppUpdater to true, so that the old implementation is still tested
• Update browser.ini and browser.bits.ini files to set browser.aboutDialogNewAppUpdater to true so that the new implementation is tested

If all this is OK, I'll file another bug for removing the legacy stuff once we merge.

Comment 8

[Drew Willcoxon :adw]

Try with the two latest revisions (first one just adds AppUpdater.jsm, second one updates aboutDialog-appUpdater.js to use it when preffed on): https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a26a1b2a2be21ad52aea039dc2b611a185eb1b1

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8127c15116e1
Factor out appUpdater from aboutDialog-appUpdater.js into a jsm module r=rstrong
https://hg.mozilla.org/integration/autoland/rev/653923cc1453
Use AppUpdater.jsm in aboutDialog-appUpdater.js when preffed on, and use the current implementation when preffed off r=rstrong

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8127c15116e1
https://hg.mozilla.org/mozilla-central/rev/653923cc1453

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 9111500 [details]
Bug 1599360 - Factor out appUpdater from aboutDialog-appUpdater.js into a jsm module

Beta/Release Uplift Approval Request

• User impact if declined: We need these patches in order to run the urlbar interventions experiment (bug 1564506) on 72 as planned.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The first patch only adds a new jsm, which we need for the experiment. The second patch adds a new implementation of the app-update code for the About window and preferences that uses this new jsm. The new implementation is preffed off and will remain preffed off on 72, so these patches don't actually change any functionality in 72. The automated test flips the pref on so that the new jsm is tested on 72, so that we can be confident it will work correctly when used by the experiment.
• String changes made/needed:

Comment 12

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9111500 [details]
Bug 1599360 - Factor out appUpdater from aboutDialog-appUpdater.js into a jsm module

some refactoring to prepare for an experiment, approved for 72.0b3

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/849518df51c9
https://hg.mozilla.org/releases/mozilla-beta/rev/76bfe47a9689