Fold XPIProvider InstallLocation classes into XPIStateLocation sub-classes

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

Trunk
mozilla62
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

Last year
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.
Priority: -- → P3

Comment 2

Last year
mozreview-review
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...
Assignee

Comment 3

Last year
mozreview-review-reply
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 hidden (mozreview-request)

Comment 5

Last year
mozreview-review-reply
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 hidden (mozreview-request)

Comment 7

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

https://reviewboard.mozilla.org/r/243658/#review251102
Attachment #8975371 - Flags: review?(aswan) → review+
Assignee

Comment 8

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

Comment 9

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f4afd7b0c01
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Updated

Last year
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.