1190692 - Support Web Extensions in the add-on manager

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We need to support the new manifest format.

Comment 2

[Dave Townsend [:mossop]]

Packaging notes: https://etherpad.mozilla.org/addonpackaging
Example manifest: https://pastebin.mozilla.org/8841912

Comment 3

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1190692: Split out manifest parsing code to support web extension manifests.

Bug 1190692: Split out manifest parsing code to support web extension manifests.

Comment 4

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1190692: Parse manifest.json files into AddonInternal instances.

Bug 1190692: Parse manifest.json files into AddonInternal instances.

Comment 5

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1190692: Load web extensions.

Bug 1190692: Load web extensions.

Also corrects a race condition where if an extension was disabled before it
had finished loading its manifest it would have called GlobalManager.init but
never call GlobalManager.uninit.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8645233 [details]
MozReview Request: Bug 1190692: Split out manifest parsing code to support web extension manifests.

https://reviewboard.mozilla.org/r/15465/#review13857

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1039
(Diff revision 1)
> +    let fis = Cc["@mozilla.org/network/file-input-stream;1"].
> +              createInstance(Ci.nsIFileInputStream);
> +    fis.init(file, -1, -1, false);
> +    let bis = Cc["@mozilla.org/network/buffered-input-stream;1"].
> +              createInstance(Ci.nsIBufferedInputStream);
> +    bis.init(fis, 4096);

Seems like this code could be shared in a function with the copy above.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1028
(Diff revision 1)
> -    addon.hasBinaryComponents = ChromeManifestParser.hasType(chromeManifest,
> +      addon.hasBinaryComponents = ChromeManifestParser.hasType(chromeManifest,

Do we want to set hasBinaryComponents to false for the web manifest case?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1147
(Diff revision 1)
> +      return loadManifestFromWebManifest(bis);

Seems like you need to set hasBinaryComponents here too.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1175
(Diff revision 1)
> +  // Load the storage service before NSS (nsIRandomGenerator),
> +  // to avoid a SQLite initialization error (bug 717904).
> +  let storage = Services.storage;
> +
> +  // Define .syncGUID as a lazy property which is also settable
> +  Object.defineProperty(addon, "syncGUID", {
> +    get: () => {
> +      // Generate random GUID used for Sync.
> +      // This was lifted from util.js:makeGUID() from services-sync.
> +      let rng = Cc["@mozilla.org/security/random-generator;1"].
> +        createInstance(Ci.nsIRandomGenerator);
> +      let bytes = rng.generateRandomBytes(9);
> +      let byte_string = [String.fromCharCode(byte) for each (byte in bytes)]
> +                        .join("");
> +      // Base64 encode
> +      let guid = btoa(byte_string).replace(/\+/g, '-')
> +        .replace(/\//g, '_');
> +
> +      delete addon.syncGUID;
> +      addon.syncGUID = guid;
> +      return guid;
> +    },
> +    set: (val) => {
> +      delete addon.syncGUID;
> +      addon.syncGUID = val;
> +    },
> +    configurable: true,
> +    enumerable: true,
> +  });

Don't see why this big block couldn't be shared.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8645234 [details]
MozReview Request: Bug 1190692: Parse manifest.json files into AddonInternal instances.

https://reviewboard.mozilla.org/r/15467/#review13873

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:747
(Diff revision 1)
> +      return defValue

Missing semicolon.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:756
(Diff revision 1)
> +  let addon = new AddonInternal();

I'm slightly worried that there are a bunch of properties in PROP_METADATA that are defined on a normal add-on but possibily null, while this code just leaves them undefined. It probably doesn't matter, but I'd feel better if we set those properties to null up front.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:766
(Diff revision 1)
> +  addon.defaultLocale = {
> +    name: getProp("name"),
> +    description: getOptionalProp("description"),
> +  }

Similar to above, there are some properties here that I'd like to see null rather than just undefined.

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini:29
(Diff revision 1)
> +

Remove?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8645235 [details]
MozReview Request: Bug 1190692: Load web extensions.

https://reviewboard.mozilla.org/r/15469/#review13879

Ship It!

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8645233 [details]
MozReview Request: Bug 1190692: Split out manifest parsing code to support web extension manifests.

Meant to r+ this one.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bce7e1052bff
https://hg.mozilla.org/integration/fx-team/rev/b958b84f2fa0
https://hg.mozilla.org/integration/fx-team/rev/896476b65693

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bce7e1052bff
https://hg.mozilla.org/mozilla-central/rev/b958b84f2fa0
https://hg.mozilla.org/mozilla-central/rev/896476b65693