1461217 - Fold XPIProvider InstallLocation classes into XPIStateLocation sub-classes

Status: RESOLVED FIXED

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

Description

[Kris Maglione [:kmag]]

Right now, we have a bunch of different "location" concepts. Sometimes we have a location name, sometimes an InstallLocation, sometimes an InstallLocation that we actually install things to, sometimes an XPIStateLocation. We spend a lot of effort mapping between these.

The InstallLocation concept in XPIProvider and XPIDatabase should just be XPIStateInstallLocations, all the time. The InstallLocation classes in XPIInstall.jsm should have a reference to these, and defer to them when appropriate, but their install logic should be confined to their own, separate classes.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1461217: Fold InstallLocation classes into XPIStateLocation sub-classes.

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

Comment 2

[Andrew Swan [:aswan]]

Comment on attachment 8975371 [details]
Bug 1461217: Fold InstallLocation classes into XPIStateLocation sub-classes.

https://reviewboard.mozilla.org/r/243658/#review250904

::: toolkit/mozapps/extensions/internal/XPIDatabase.jsm:299
(Diff revision 1)
> -    if (this._installLocation) {
> -      this.location = this._installLocation.name;
> +    if (this._location) {
> +      this.location = this._location.name;
>      } else if (this.location) {
> -      this._installLocation = XPIProvider.installLocationsByName[this.location];
> +      this._location = XPIStates.getLocation(this.location);
>      }

This is still obscure.  Both the two different externally settable properties and the fact that it happens as a side effect inside addedToDatabase().  Location should be fixed for the lifetime of an individual AddonInternal instance, so ideally we would just figure out a way to pass it into the constructor and get rid of this bit of code.

::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:2779
(Diff revision 1)
>      XPIInstall.installs.delete(this);
>      return Promise.resolve(null);
>    }
>  }
>  
> -// These are partial classes which contain the install logic for the
> +class DirectoryInstallLocation {

can we call these FooInstaller for slightly less confusion with other similarly named classes

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1026
(Diff revision 1)
> -    return data;
> -  },
> +    this._linkedAddons = {};
> +    this._IDToFileMap = {};

i think these are now obsolete?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1048
(Diff revision 1)
> +   *        The file containing the directory path
> +   * @returns {nsIFile?}
> +   *        An nsIFile object representing the linked directory, or null
> +   *        on error.
>     */
> -  loadExtensionState() {
> +  _readDirectoryFromFile(aFile) {

nit: this name is terribly confusing.  `_readLinkedFile` perhaps?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1387
(Diff revision 1)
>  
> -      Object.assign(params, aExtraParams);
> +/**
> + * Keeps track of the state of XPI add-ons on the file system.
> + */
> +var XPIStates = {
> +  // Map(location name -> Map(add-on ID -> XPIState))

aren't the values here XPIStateLocation instances?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1495
(Diff revision 1)
> -   *        bootstrap method.
> +   *        side-loads.
> +   *
> +   * @returns {boolean}
> +   *        True if anything has changed.
>     */
> -  loadBootstrapScope(aReason) {
> +  getInstallState(ignoreSideloads = true) {

I know you're not changing this but I've always found this method name very non-descriptive.  Naming is hard.  How about `scanForChanges()` or something

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1820
(Diff revision 1)
> +              await XPIDatabase.updateAddonDisabledState(addon);
> +          }
> -      }
> +        }
> -    }
> +      }
>  
> -    let enabledScopes = Services.prefs.getIntPref(PREF_EM_ENABLED_SCOPES,
> +      let installLocation = addon._location || addon.location;

this does not seem right...  location (without the leading undescore) is just a string...

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1890
(Diff revision 1)
> -  },
>  
>    /**
> -   * Starts the XPI provider initializes the install locations and prefs.
> +   * Loads a bootstrapped add-on's bootstrap.js into a sandbox and the reason
> +   * values as constants in the scope. This will also add information about the
> +   * add-on to the bootstrappedAddons dictionary and notify the crash reporter

I could swear this comment was rewritten in a recent patch...
The body of this function also looks old, does this need a rebase?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2127
(Diff revision 1)
> -        let scope = BootstrapScope.get(addon);
> -        let existing = XPIStates.findAddon(id, loc => loc != tempLocation);
> +  // An array of known install locations
> +  installLocations: null,
> +  // A dictionary of known install locations by name
> +  installLocationsByName: null,

These can go now right?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2396
(Diff revision 1)
> -      if (updateReasons.length > 0) {
> -        AddonManagerPrivate.recordSimpleMeasure("XPIDB_startup_load_reasons", updateReasons);
> +          // The startup update check above may have already started some
> +          // extensions, make sure not to try to start them twice.

there isn't a startup update check above any more, but that's for another day...

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8975371 [details]
Bug 1461217: Fold InstallLocation classes into XPIStateLocation sub-classes.

https://reviewboard.mozilla.org/r/243658/#review250904

> This is still obscure.  Both the two different externally settable properties and the fact that it happens as a side effect inside addedToDatabase().  Location should be fixed for the lifetime of an individual AddonInternal instance, so ideally we would just figure out a way to pass it into the constructor and get rid of this bit of code.

We can, but given the multiple ways we create AddonInternal instances, that is easier said than done. It seemed better done in another bug.

> i think these are now obsolete?

Yes. I'm pretty sure I updated the patch to get rid of tiem.

> aren't the values here XPIStateLocation instances?

Yes. But given that XPIStateLocation is a Map subclass, they are also Map instances.

> this does not seem right...  location (without the leading undescore) is just a string...

It depends on whether `addon` is an AddonInternal or an XPIState. Yes, this is confusing, but it's an existing problem.

> I could swear this comment was rewritten in a recent patch...
> The body of this function also looks old, does this need a rebase?

Yes. I rebased the corrections in earler patches when I updated them, but did not push an updated version of this patch.

> These can go now right?

Yes. I removed it in the updated patch.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8975371 [details]
Bug 1461217: Fold InstallLocation classes into XPIStateLocation sub-classes.

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

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8975371 [details]
Bug 1461217: Fold InstallLocation classes into XPIStateLocation sub-classes.

https://reviewboard.mozilla.org/r/243658/#review250904

> We can, but given the multiple ways we create AddonInternal instances, that is easier said than done. It seemed better done in another bug.

I think there are just two different places where is is intantiated and the location is readily available in both...

> Yes. But given that XPIStateLocation is a Map subclass, they are also Map instances.

I'm not sure if that means you don't think this should change.  Map may be correct, but XPIStateLocation would be clearer.

> It depends on whether `addon` is an AddonInternal or an XPIState. Yes, this is confusing, but it's an existing problem.

oh, ugh

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8975371 [details]
Bug 1461217: Fold InstallLocation classes into XPIStateLocation sub-classes.

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

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8975371 [details]
Bug 1461217: Fold InstallLocation classes into XPIStateLocation sub-classes.

https://reviewboard.mozilla.org/r/243658/#review251102

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f4afd7b0c01bff41478c859ecb124d735338562
Bug 1461217: Fold InstallLocation classes into XPIStateLocation sub-classes. r=aswan

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/4f4afd7b0c01