Closed Bug 1454202 Opened 2 years ago Closed 2 years ago

Replace uses of callback-based AddonManager APIs with promise-based versions

Categories

(Toolkit :: Add-ons Manager, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
59 bytes, text/x-review-board-request
aswan
: review+
Details
No description provided.
Comment on attachment 8968012 [details]
Bug 1454202: Part 1c - Manually fix non-eslint issues after auto-rewrite.

https://reviewboard.mozilla.org/r/236690/#review242450


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/mozapps/extensions/test/browser/browser_discovery.js:110
(Diff revision 1)
>    isnot(hash, null, "There should be a hash");
>    try {
>      var data = JSON.parse(hash);
>    } catch (e) {
>      ok(false, "Hash should have been valid JSON: " + e);
>      aCallback();

Error: 'acallback' is not defined. [eslint: no-undef]
Comment on attachment 8968094 [details]
Bug 1454202: Part 3c - Convert add-on providers to use promise-based methods.

https://reviewboard.mozilla.org/r/236770/#review242492


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:636
(Diff revision 1)
>      }
>  
>      let plugin = this._plugins.get(aId);
>      if (plugin && !GMPUtils.isPluginHidden(plugin)) {
> -      aCallback(plugin.wrapper);
> +      return plugin.wrapper;
>      } else {

Error: Unnecessary 'else' after 'return'. [eslint: no-else-return]
Comment on attachment 8968094 [details]
Bug 1454202: Part 3c - Convert add-on providers to use promise-based methods.

https://reviewboard.mozilla.org/r/236770/#review242502


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/mozapps/extensions/internal/GMPProvider.jsm:636
(Diff revision 2)
>      }
>  
>      let plugin = this._plugins.get(aId);
>      if (plugin && !GMPUtils.isPluginHidden(plugin)) {
> -      aCallback(plugin.wrapper);
> +      return plugin.wrapper;
>      } else {

Error: Unnecessary 'else' after 'return'. [eslint: no-else-return]
Comment on attachment 8968010 [details]
Bug 1454202: Part 1a - Auto-replace uses of callback-based AddonManager APIs with Promise-based versions.

https://reviewboard.mozilla.org/r/236686/#review242784

Having a bunch of local variables with `aFoo` names is unforunate :(
I think all the sub-parts for part 1 should be folded together for landing...

::: browser/components/preferences/in-content/tests/browser_extension_controlled.js:23
(Diff revision 2)
>  function installAddon(xpiName) {
>    let filePath = getSupportsFile(`addons/${xpiName}`).file;
> -  return new Promise((resolve, reject) => {
> -    AddonManager.getInstallForFile(filePath, install => {
> +  return new Promise(async (resolve, reject) => {
> +    let install = await AddonManager.getInstallForFile(filePath);
> -      if (!install) {
> +    if (!install) {
> -        throw new Error(`An install was not created for ${filePath}`);
> +      throw new Error(`An install was not created for ${filePath}`);
> -      }
> +    }
> -      install.addListener({
> +    install.addListener({
> -        onDownloadFailed: reject,
> +      onDownloadFailed: reject,
> -        onDownloadCancelled: reject,
> +      onDownloadCancelled: reject,
> -        onInstallFailed: reject,
> +      onInstallFailed: reject,
> -        onInstallCancelled: reject,
> +      onInstallCancelled: reject,
> -        onInstallEnded: resolve
> +      onInstallEnded: resolve
> -      });
> +    });
> -      install.install();
> +    install.install();
> -    });
> +  });
> -  });
>  }

this could be ripped out and replaced with `AddonTestUtils.promiseInstallFile()`

::: browser/tools/mozscreenshots/mozscreenshots/extension/bootstrap.js:35
(Diff revision 2)
> -  AddonManager.getAddonByID(data.id, function(addon) {
> +  let addon = await AddonManager.getAddonByID(data.id);
> -    let extensionPath = addon.getResourceURI();
> +  let extensionPath = addon.getResourceURI();

I think this can just be `data.resourceURI`, no async anythign needed

::: devtools/client/aboutdebugging/test/head.js:436
(Diff revision 2)
>  function installAddonWithManager(filePath) {
> -  return new Promise((resolve, reject) => {
> -    AddonManager.getInstallForFile(filePath, install => {
> +  return new Promise(async (resolve, reject) => {
> +    let install = await AddonManager.getInstallForFile(filePath);
> -      if (!install) {
> +    if (!install) {
> -        throw new Error(`An install was not created for ${filePath}`);
> +      throw new Error(`An install was not created for ${filePath}`);
> -      }
> +    }
> -      install.addListener({
> +    install.addListener({
> -        onDownloadFailed: reject,
> +      onDownloadFailed: reject,
> -        onDownloadCancelled: reject,
> +      onDownloadCancelled: reject,
> -        onInstallFailed: reject,
> +      onInstallFailed: reject,
> -        onInstallCancelled: reject,
> +      onInstallCancelled: reject,
> -        onInstallEnded: resolve
> +      onInstallEnded: resolve
> -      });
> +    });
> -      install.install();
> +    install.install();
> -    });
> +  });
> -  });
>  }

another application for AddonTestUtils

::: toolkit/components/extensions/parent/ext-runtime.js:97
(Diff revision 2)
>            if (extension.upgrade) {
>              // If there is a pending update, install it now.
>              extension.upgrade.install();
>            } else {
>              // Otherwise, reload the current extension.
> -            AddonManager.getAddonByID(extension.id, addon => {
> +            let addon = await AddonManager.getAddonByID(extension.id);

meh this should really be using instanceID

::: toolkit/mozapps/extensions/test/xpcshell/test_blocklist_regexp.js
(Diff revision 2)
> -      // Blocklist contains two entries that will match this addon - ensure
> -      // that the first one is applied.

why drop the comment?
Attachment #8968010 - Flags: review?(aswan) → review+
Comment on attachment 8968011 [details]
Bug 1454202: Part 1b - Manually fix eslint errors after auto-rewrite.

https://reviewboard.mozilla.org/r/236688/#review242802
Attachment #8968011 - Flags: review?(aswan) → review+
Comment on attachment 8968012 [details]
Bug 1454202: Part 1c - Manually fix non-eslint issues after auto-rewrite.

https://reviewboard.mozilla.org/r/236690/#review242804
Attachment #8968012 - Flags: review?(aswan) → review+
Comment on attachment 8968022 [details]
Bug 1454202: Part 2a - Add promise variants for more AOM test "helper" functions.

https://reviewboard.mozilla.org/r/236712/#review242808

::: toolkit/mozapps/extensions/test/browser/head.js:351
(Diff revision 1)
>  function wait_for_view_load(aManagerWindow, aCallback, aForceWait, aLongerTimeout) {
> +  let p = new Promise(resolve => {
> -  requestLongerTimeout(aLongerTimeout ? aLongerTimeout : 2);
> +    requestLongerTimeout(aLongerTimeout ? aLongerTimeout : 2);
>  
> -  if (!aForceWait && !aManagerWindow.gViewController.isLoading) {
> +    if (!aForceWait && !aManagerWindow.gViewController.isLoading) {
> -    log_exceptions(aCallback, aManagerWindow);
> +      log_exceptions(resolve, aManagerWindow);

Does `log_exceptions()` here actually do anything any more?
Comment on attachment 8968022 [details]
Bug 1454202: Part 2a - Add promise variants for more AOM test "helper" functions.

https://reviewboard.mozilla.org/r/236712/#review242808

> Does `log_exceptions()` here actually do anything any more?

Nope.
Comment on attachment 8968022 [details]
Bug 1454202: Part 2a - Add promise variants for more AOM test "helper" functions.

https://reviewboard.mozilla.org/r/236712/#review242808

> Nope.

so lets just get rid of it (and in the similar lines below)
Comment on attachment 8968022 [details]
Bug 1454202: Part 2a - Add promise variants for more AOM test "helper" functions.

https://reviewboard.mozilla.org/r/236712/#review242858

r=me with the no-op log_exceptions() calls removed
Attachment #8968022 - Flags: review?(aswan) → review+
Comment on attachment 8968025 [details]
Bug 1454202: Part 2d - More scripted rewrites.

https://reviewboard.mozilla.org/r/236718/#review242868

::: toolkit/mozapps/extensions/test/browser/browser_bug562797.js:549
(Diff revision 2)
>  
> -  clickLink(aManager, "link-good", function() {
> +  await clickLink(aManager, "link-good");
> -    info("2");
> +  info("2");
> -    is_in_discovery(aManager, SECOND_URL, true, false);
> +  is_in_discovery(aManager, SECOND_URL, true, false);
>  
> -    waitForLoad(aManager, function() {
> +  Promise.resolve().then(() => {

is the Promise.resolve() necessary?
if it is, how about something like:

```
// comment explaining why we need a run through the event loop
await Promise.resolve();
...
```
Comment on attachment 8968025 [details]
Bug 1454202: Part 2d - More scripted rewrites.

https://reviewboard.mozilla.org/r/236718/#review242868

> is the Promise.resolve() necessary?
> if it is, how about something like:
> 
> ```
> // comment explaining why we need a run through the event loop
> await Promise.resolve();
> ...
> ```

It may or may not be necessary, but the `await` version wouldn't help. The whole point is to make the go_back() call run after waitForLoad() has added its listeners. I have no idea if that's actually necessary, since most of the other code in this test that follows this pattern does not do any such thing, but there were a handful of cases that did.
Comment on attachment 8968025 [details]
Bug 1454202: Part 2d - More scripted rewrites.

https://reviewboard.mozilla.org/r/236718/#review242868

> It may or may not be necessary, but the `await` version wouldn't help. The whole point is to make the go_back() call run after waitForLoad() has added its listeners. I have no idea if that's actually necessary, since most of the other code in this test that follows this pattern does not do any such thing, but there were a handful of cases that did.

Okay then how about something like
```
let loadPromise = waitForLoad(...);
go_back();
await loadPromise;
```
Ugh.

How about executeSoon(go_back)?
We use that pattern lots of places.  If you really don't like it, please at least at a comment so its clear what's happening even while reading quickly through the test.
Comment on attachment 8968024 [details]
Bug 1454202: Part 2c - Manually fix issues in auto-rewrite.

https://reviewboard.mozilla.org/r/236716/#review243086

::: toolkit/mozapps/extensions/test/browser/browser_discovery.js:445
(Diff revision 1)
>  
>    EventUtils.synthesizeMouse(gCategoryUtilities.get("discover"), 2, 2, { }, gManagerWindow);
>  
> +  Promise.resolve().then(() => {
> +     ok(isLoading(), "Should be loading");
> +     // This will actually stop the about:blank load

I don't understand this, is this comment still correct?
Comment on attachment 8968023 [details]
Bug 1454202: Part 2b - Auto-rewrite AOM UI tests to use promise variants of helpers.

https://reviewboard.mozilla.org/r/236714/#review243210

Please squash 2b and 2c together for landing
Attachment #8968023 - Flags: review?(aswan) → review+
Comment on attachment 8968024 [details]
Bug 1454202: Part 2c - Manually fix issues in auto-rewrite.

https://reviewboard.mozilla.org/r/236716/#review243212

r=me if all the `Promise.resolve().then(...` stuff either gets re-written or gets comments explaining the exact sequence of operations required.
Attachment #8968024 - Flags: review?(aswan) → review+
Comment on attachment 8968025 [details]
Bug 1454202: Part 2d - More scripted rewrites.

https://reviewboard.mozilla.org/r/236718/#review243214

Same comment as part 2c about the `Promise.resolve().then` pattern.
Also, this could use a better comment, I don't think the scripting matters, how about just "modernize some addons manager browser tests"
Attachment #8968025 - Flags: review?(aswan) → review+
Comment on attachment 8968092 [details]
Bug 1454202: Part 3a - Promisify more internal add-on manager methods.

https://reviewboard.mozilla.org/r/236766/#review243216

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1896
(Diff revision 2)
>                                                 this.wrapper);
>        flushJarCache(this.file);
>        return;
>      }
>  
> -    let addon = await new Promise(resolve => {
> +    let addon = await XPIDatabase.getVisibleAddonForID(this.addon.id);

this should be in part 3b?
or just squash 3a and 3b together...

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:664
(Diff revision 2)
>        addonDB => {
> -        getRepositoryAddon(_findAddon(addonDB, aFilter), makeSafe(aCallback));
> +        return getRepositoryAddon(_findAddon(addonDB, aFilter));
>        })

nit: can just become `addonDB => getRepositoryAddon(...)`
Comment on attachment 8968092 [details]
Bug 1454202: Part 3a - Promisify more internal add-on manager methods.

https://reviewboard.mozilla.org/r/236766/#review243216

> this should be in part 3b?
> or just squash 3a and 3b together...

Yeah, I tried to rebase a typo fix and the whole change wound up in this revision.
Comment on attachment 8968092 [details]
Bug 1454202: Part 3a - Promisify more internal add-on manager methods.

https://reviewboard.mozilla.org/r/236766/#review243452
Attachment #8968092 - Flags: review?(aswan) → review+
Comment on attachment 8968093 [details]
Bug 1454202: Part 3b - Rewrite callers to use promise variants of internal APIs.

https://reviewboard.mozilla.org/r/236768/#review243454

I would prefer rolling this into 3a before landing
Attachment #8968093 - Flags: review?(aswan) → review+
Comment on attachment 8968094 [details]
Bug 1454202: Part 3c - Convert add-on providers to use promise-based methods.

https://reviewboard.mozilla.org/r/236770/#review243220

Nice.  I don't get the direct connection to 3a and 3b though.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:254
(Diff revision 2)
>      addAddon(aAddon) {
>        this._addons.push(aAddon);
>        AddonManagerPrivate.callAddonListeners("onInstalled", new MockAddonWrapper(aAddon));
>      },
>  
> -    getAddonsByTypes(aTypes, aCallback) {
> +    async getAddonsByTypes(aTypes) {

no need for async
Attachment #8968094 - Flags: review?(aswan) → review+
Comment on attachment 8968094 [details]
Bug 1454202: Part 3c - Convert add-on providers to use promise-based methods.

https://reviewboard.mozilla.org/r/236770/#review243220

Yeah, I guess it probably makes more sense as part 4.
Comment on attachment 8968095 [details]
Bug 1454202: Part 4a - Remove callback-based variants of most AddonManager APIs.

https://reviewboard.mozilla.org/r/236772/#review243468

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3414
(Diff revision 2)
>    async getAddonByID(aId) {
>      let aAddon = await XPIDatabase.getVisibleAddonForID(aId);
>      return aAddon ? aAddon.wrapper : null;
>    },
>  
> +  syncGetAddonByID(aId) {

please add a comment explaining that this will always return null if the database isn't loaded and that is is up to the caller to be aware of that (ie, they should not assume that the given addon isn't installed if this returns null unless they are certain that the db is loaded)

::: toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js:43
(Diff revision 2)
>      resetBlocklist();
>    });
>  
>    let pluginTag = getTestPluginTag();
>    pluginTag.enabledState = Ci.nsIPluginTag.STATE_CLICKTOPLAY;
> -  let managerWindow = await new Promise(resolve => open_manager("addons://list/plugin", resolve));
> +  let managerWindow = await open_manager("addons://list/plugin");

should be in part 2b?
Attachment #8968095 - Flags: review?(aswan) → review+
Comment on attachment 8968096 [details]
Bug 1454202: Part 4b - Remove interstitial callback argument from getInstall* APIs.

https://reviewboard.mozilla.org/r/236774/#review243494

hooray
Attachment #8968096 - Flags: review?(aswan) → review+
Comment on attachment 8968097 [details]
Bug 1454202: Part 4c - Update callers for new getInstall* signature.

https://reviewboard.mozilla.org/r/236776/#review243498

Nice, thank you!  This should be rolled into 4b too I think.
Attachment #8968097 - Flags: review?(aswan) → review+
Priority: -- → P2
Comment on attachment 8968094 [details]
Bug 1454202: Part 3c - Convert add-on providers to use promise-based methods.

https://reviewboard.mozilla.org/r/236770/#review243220

> no need for async

Bah. Replied to this earlier but reviewboard barfed.

Without `async`, this will return a raw array, rather than a promise. That will work the same for callers that use `await`, but not for callers that use `.then()`. I could return `Promise.resolve(...)`, but I'd rather be consistent, and use `async` for async APIs where possible.
Comment on attachment 8968024 [details]
Bug 1454202: Part 2c - Manually fix issues in auto-rewrite.

https://reviewboard.mozilla.org/r/236716/#review243086

> I don't understand this, is this comment still correct?

It's as correct as it's ever been. Whether it was ever correct, I'm not sure.
Comment on attachment 8968095 [details]
Bug 1454202: Part 4a - Remove callback-based variants of most AddonManager APIs.

https://reviewboard.mozilla.org/r/236772/#review243468

> should be in part 2b?

No. Part 2b was a scripted rewrite, but this was a manual change for a usage that didn't fit that pattern (i.e., something that already promisified the old API, rather than passing a callback).
https://hg.mozilla.org/integration/mozilla-inbound/rev/f297b23906c2a9e87b34a6cf3aa9aa8dcd73db55
Bug 1454202: Part 1 - Update legacy callers to use Promise-based AddonManager APIs. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5b866d240e67287d5cb1be626287c233dff044
Bug 1454202: Part 2a - Add promise variants for more AOM test "helper" functions. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/66883c7f740a513e414c00565123cc16355e9119
Bug 1454202: Part 2b-c - Update AOM UI tests to use promise variants of helpers. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb25432060f5e7d7c9bc174961a71c2ea207768
Bug 1454202: Part 2d - Modernize other callback-based AOM UI tests. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/0244913345d307219cf92d90addf2e3cd89348ed
Bug 1454202: Part 3a - Promisify more internal add-on manager methods. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/773b6d28b99067395102db127a526b6d08871812
Bug 1454202: Part 3b - Rewrite callers to use promise variants of internal APIs. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/112ff1e321a11fac3895742f1f35fe12d2acf903
Bug 1454202: Part 4 - Convert add-on providers to use promise-based methods. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebacf44a86dc27644717e72a2a302d586d5d81a1
Bug 1454202: Part 5a - Remove callback-based variants of most AddonManager APIs. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/81d9e69a3539603114d1b97f9700c14f45affd49
Bug 1454202: Part 5b-c - Remove interstitial callback argument from getInstall* APIs. r=aswan
Flags: qe-verify-
Could you please add a check, that if still a callback is provided like the old (documented) API

AddonManager.getAllAddons(mycallback);

that I get ANY information on this API change on the console? At the moment it just does nothing without any feedback. Very hard to find. Took me 30min I would like to have back :-)

Otherwise: Nice!
Depends on: 1487103
See Also: → 1555310
You need to log in before you can comment on or make changes to this bug.