Closed Bug 1190692 Opened 4 years ago Closed 4 years ago

Support Web Extensions in the add-on manager

Categories

(WebExtensions :: Untriaged, defect, P1)

34 Branch
defect

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: billm, Assigned: mossop)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

We need to support the new manifest format.
Priority: -- → P1
Duplicate of this bug: 1189935
Assignee: nobody → dtownsend
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Depends on: 1009332
Summary: Support open browser extensions in the add-on manager → Support Web Extensions in the add-on manager
Depends on: 1192432
Depends on: 1192433
Depends on: 1192435
Depends on: 1192437
Depends on: 1192439
Bug 1190692: Split out manifest parsing code to support web extension manifests.
Attachment #8645233 - Flags: review?(wmccloskey)
Bug 1190692: Parse manifest.json files into AddonInternal instances.
Attachment #8645234 - Flags: review?(wmccloskey)
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.
Attachment #8645235 - Flags: review?(wmccloskey)
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.
Attachment #8645233 - Flags: review?(wmccloskey)
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?
Attachment #8645234 - Flags: review?(wmccloskey) → review+
Comment on attachment 8645235 [details]
MozReview Request: Bug 1190692: Load web extensions.

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

Ship It!
Attachment #8645235 - Flags: review?(wmccloskey) → review+
Comment on attachment 8645233 [details]
MozReview Request: Bug 1190692: Split out manifest parsing code to support web extension manifests.

Meant to r+ this one.
Attachment #8645233 - Flags: review+
Depends on: 1196301
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.