Closed
Bug 1323845
Opened 8 years ago
Closed 7 years ago
Allow bundled experiments, and support for content-side APIs
Categories
(WebExtensions :: Experiments, defect, P1)
WebExtensions
Experiments
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: andy+bugzilla, Assigned: kmag)
References
Details
(Whiteboard: [experiments] triaged)
Attachments
(11 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 |
Kris to enter some more details here.
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(kmaglione+bmo)
Updated•8 years ago
|
Priority: -- → P1
Whiteboard: [experiments] triaged
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → kmaglione+bmo
Reporter | ||
Updated•8 years ago
|
Priority: P1 → P2
Assignee | ||
Updated•7 years ago
|
No longer blocks: webext-oop
Flags: needinfo?(kmaglione+bmo)
Summary: Allow experiments to work with out of process → Allow experiments to include content-side APIs
Assignee | ||
Updated•7 years ago
|
Priority: P2 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8941289 [details] Bug 1323845: Part 0 - Cleanup some code formatting. https://reviewboard.mozilla.org/r/211568/#review217640
Attachment #8941289 -
Flags: review?(aswan) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8941290 [details] Bug 1323845: Part 1 - Support multiple schema root namespaces. https://reviewboard.mozilla.org/r/211570/#review217688 This looks good, but some documentation would be nice (just an overview of how a SchemaRoot can be overlayed on an existing SchemaRoot and the intended use is one SchemaRoot for all the built-in APIs with additional roots for extensions that use different experiments). If that documentation is actually coming in a subsequent patch that I haven't seen yet, ignore this remark. ::: toolkit/components/extensions/Schemas.jsm:1137 (Diff revision 1) > - static parseSchema(schema, path, extraProperties = []) { > + static parseSchema(root, schema, path, extraProperties = []) { > this.checkSchemaProperties(schema, path, extraProperties); > > return new this(schema); > } is this function ever actually called? it looks like we only call it on derived types ::: toolkit/components/extensions/Schemas.jsm:2685 (Diff revision 1) > return super.has(key); > } > } > > -this.Schemas = { > - initialized: false, > +class SchemaRoot extends Namespace { > + constructor(base, schemaJSON) { can we have a documentation comment here? (one for the whole class would be good too) ::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_roots.js:104 (Diff revision 1) > + > +let talliedErrors = []; > + > +let permissions = new Set(); > + > +class TallyingAPIImplementation extends SchemaAPIInterface { all this stuff is duplicated with other tests, can it move to a head.js file? ::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_roots.js:200 (Diff revision 1) > + baseSchema.inject(base, wrapper); > + schema.inject(root, wrapper); > + > + equal(typeof base.base, "object", "base.base exists"); > + equal(typeof root.base, "object", "root.base exists"); > + equal(typeof base.experiments, "undefined", "base.experiments exists not"); o.O ::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_roots.js:237 (Diff revision 1) > + Assert.throws(() => root.experiments.foo.foo("Meh."), > + /Incorrect argument types/, > + "root.experiments.foo.foo()"); > + > + Assert.throws(() => root.experiments.bar.bar("Meh."), > + /Incorrect argument types/, shouldn't this be "wrong number of arguments" or something?
Attachment #8941290 -
Flags: review?(aswan) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8941291 [details] Bug 1323845: Part 2a - Support separate API managers for each extension. https://reviewboard.mozilla.org/r/211572/#review217936 ::: toolkit/components/extensions/ExtensionCommon.jsm:963 (Diff revision 1) > this.apis = new DefaultWeakMap(() => new Map()); > > this._scriptScopes = []; > } > > + onStartup(extension) { I don't see this getting called outside of the main process. I'm assuming this is laying groundwork for something in a future patch, leaving this note here to myself while I read the other patches.
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8941292 [details] Bug 1323845: Part 2b - Support separate schema roots for each API manager. https://reviewboard.mozilla.org/r/211574/#review217942 ::: toolkit/components/extensions/ExtensionContent.jsm:76 (Diff revision 1) > const CATEGORY_EXTENSION_SCRIPTS_CONTENT = "webextension-scripts-content"; > const CONTENT_SCRIPT_INJECTION_HISTOGRAM = "WEBEXT_CONTENT_SCRIPT_INJECTION_MS"; > > var apiManager = new class extends SchemaAPIManager { > constructor() { > - super("content"); > + super("content", Schemas); I expected this (and other occurences below) to be Schemas.rootSchema, but I guess that would defeat lazy initilization?
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8941293 [details] Bug 1323845: Part 2c - Lazy init API global only when APIs are first loaded. https://reviewboard.mozilla.org/r/211576/#review217944 ::: toolkit/components/extensions/ExtensionCommon.jsm (Diff revision 1) > * @param {SchemaRoot} schema > */ > constructor(processType, schema) { > super(); > this.processType = processType; > - this.global = this._createExtGlobal(); we should leave the initializer, just set to null
Attachment #8941293 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8941292 [details] Bug 1323845: Part 2b - Support separate schema roots for each API manager. https://reviewboard.mozilla.org/r/211574/#review217942 > I expected this (and other occurences below) to be Schemas.rootSchema, but I guess that would defeat lazy initilization? Yeah, this winds up being called before the root schema is fully initialized at the moment. My plan for the near future is to get rid all of the dynamic schema adding and removal that we need for old-style experiments, and then just use the root schema directly.
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8941291 [details] Bug 1323845: Part 2a - Support separate API managers for each extension. https://reviewboard.mozilla.org/r/211572/#review217936 > I don't see this getting called outside of the main process. I'm assuming this is laying groundwork for something in a future patch, leaving this note here to myself while I read the other patches. It's not used in the content process. I mainly added this to make it easier to broadcast the startup event to all of the API managers for an extension, rather than just the main one.
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8941295 [details] Bug 1323845: Part 4 - Add Extension.isPrivileged property. https://reviewboard.mozilla.org/r/211580/#review217958 ::: toolkit/components/extensions/Extension.jsm:572 (Diff revision 1) > if (this.manifest.devtools_page) { > permissions.add("devtools"); > } > > for (let perm of manifest.permissions) { > - if (perm === "geckoProfiler") { > + if (perm === "geckoProfiler" && !this.isPrivileged) { why do this here? we should change gecko profiler to be signed as privileged, but at that point we should just add "mozillaAddons" as a permission in the schema and remove this whole fragment. ::: toolkit/components/extensions/Extension.jsm:1221 (Diff revision 1) > get manifestCacheKey() { > return [this.id, this.version, Services.locale.getAppLocaleAsLangTag()]; > } > > + get isPrivileged() { > + return (this.addonData.signedState === AddonManager.SIGNEDSTATE_PRIVILEGED || I think we'll probably want this to be true for system addons as well, but you're preserving the existing behavior here so that can happen in a separate bug. ::: toolkit/components/extensions/Extension.jsm:1234 (Diff revision 1) > + } > + > async _parseManifest() { > let manifest = await super.parseManifest(); > if (manifest && manifest.permissions.has("mozillaAddons") && > - this.addonData.signedState !== AddonManager.SIGNEDSTATE_PRIVILEGED) { > + !this.isPrivileged) { There's a behavior change here that this grants access to mozillaAddons APIs to anything installed temporarily. I don't really have an opinion on whether we should or shouldn't do that, but was it deliberate?
Attachment #8941295 -
Flags: review?(aswan) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8941294 [details] Bug 1323845: Part 3 - Auto-create intermediate API namespace objects when necessary. https://reviewboard.mozilla.org/r/211578/#review217960
Attachment #8941294 -
Flags: review?(aswan) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8941291 [details] Bug 1323845: Part 2a - Support separate API managers for each extension. https://reviewboard.mozilla.org/r/211572/#review217964
Attachment #8941291 -
Flags: review?(aswan) → review+
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8941292 [details] Bug 1323845: Part 2b - Support separate schema roots for each API manager. https://reviewboard.mozilla.org/r/211574/#review217968 ::: toolkit/components/extensions/ProxyScriptContext.jsm:391 (Diff revision 1) > let can = new CanOfAPIs(this, proxyScriptAPIManager, localAPIs); > proxyScriptAPIManager.lazyInit(); > > let browserObj = Cu.createObjectIn(this.sandbox); > let injectionContext = new ProxyScriptInjectionContext(this, can); > - Schemas.inject(browserObj, injectionContext); > + proxyScriptAPIManager.schema.inject(browserObj, injectionContext); I guess this should really be in the previous patch but don't we need a separate ProxyScriptAPIManager per extension?
Attachment #8941292 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8941292 [details] Bug 1323845: Part 2b - Support separate schema roots for each API manager. https://reviewboard.mozilla.org/r/211574/#review217968 > I guess this should really be in the previous patch but don't we need a separate ProxyScriptAPIManager per extension? If we want to support experiments in proxy script contexts, yes. But I'd really rather just get rid of proxyScriptAPIManager.
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8941296 [details] Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules. https://reviewboard.mozilla.org/r/211582/#review217994 I feel sorry for anybody who ends up trying to make sense out this patch in isolation in the future since in this patch, MultiAPIManager is a big old no-op wrapped around LazyAPIManager. I realize it is laying the foundation for letting other extensions use an experimental API, but as we talked about on IRC I would really prefer just not supporting that at all which would cut down on the amount and complexity of the code here. ::: toolkit/components/extensions/Extension.jsm:646 (Diff revision 1) > + if (!schemaPromises.has(schema)) { > + schemaPromises.set(schema, this.readJSON(data.schema).then(json => Schemas.processSchema(json))); > + } can you just use a DefaultMap here? ::: toolkit/components/extensions/ExtensionCommon.jsm:969 (Diff revision 1) > + if (schema) { > - this.schema = schema; > + this.schema = schema; > + } This is mildly gross, taking this out of this class and adding it to a thin subclass would be nice. Up to you... ::: toolkit/components/extensions/ExtensionCommon.jsm:1403 (Diff revision 1) > + get schema() { > + if (!this._schema) { > + this._schema = new SchemaRoot(Schemas.rootSchema, this.schemaURLs); > + this._schema.parseSchemas(); > + } > + return this._schema; any reason not to just use defineLazyGetter() ::: toolkit/components/extensions/ExtensionCommon.jsm:1428 (Diff revision 1) > + if (child.lazyInit) { > + let res = child.lazyInit(); > + if (res && typeof res.then === "function") { > + await res; > + } > + } what's this for? from my reading, children is only ever SchemaAPIManaer or LazyAPIManager, neither of which has a lazyInit method ::: toolkit/components/extensions/ExtensionCommon.jsm:1459 (Diff revision 1) > + let promises = []; > + for (let child of this.children) { > + promises.push(child.onStartup(extension)); > + } > + return Promise.all(promises); why not just `return Promise.all(this.children.map(child => child.onStartup(extension)));` ::: toolkit/components/extensions/schemas/experiments.json:118 (Diff revision 1) > + "type": "string", > + "enum": [ > + "addon_parent", > + "content_parent", > + "devtools_parent", > + "proxy_script" I think we don't actually support these?
Attachment #8941296 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 26•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8941296 [details] Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules. https://reviewboard.mozilla.org/r/211582/#review217994 `MultiAPIManager` isn't a no-op in this patch. It allows us to use both the bundled experiment API manager and the base API manager in the same extension. > can you just use a DefaultMap here? I guess, but I think it would be harder to follow, since we'd just wind up doing no-op `.get(schema)`s and throwing away the result. > any reason not to just use defineLazyGetter() Probably not. > what's this for? from my reading, children is only ever SchemaAPIManaer or LazyAPIManager, neither of which has a lazyInit method For the base API manager, it's a SchemaAPIManager sub-class that has a lazyInit method that needs to be called.
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8941297 [details] Bug 1323845: Part 5b - Add tests for bundled experiments APIs. https://reviewboard.mozilla.org/r/211584/#review218358 Testing an experiment that defines multiple APIs would be good, but the test could get arbitrarily complicated, I'm not sure where to draw the line.
Attachment #8941297 -
Flags: review?(aswan) → review+
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8941296 [details] Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules. https://reviewboard.mozilla.org/r/211582/#review218360 ::: toolkit/components/extensions/schemas/experiments.json:104 (Diff revision 1) > + { > + "id": "APIEvent", > + "type": "string", > + "enum": [ > + "startup" > + ] > + }, Is there some reason we can't support manifest entries, updates, uninstalls? If not can you at least file a bug to add them later?
Comment 29•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #26) > `MultiAPIManager` isn't a no-op in this patch. It allows us to use both the > bundled experiment API manager and the base API manager in the same > extension. Oops, let me revise that, the fact that for the common case of a single xpi with an experiment and the extension that uses it, the base and experiment schemas are merged from LazyAPIManager while the actual API implementations are merged from MultiAPIManager is making this harder to follow than it would be if this didn't need to be written to handle the more general case.
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8941298 [details] Bug 1323845: Part 6a - Support WebExtension-style experiment API provider extensions. https://reviewboard.mozilla.org/r/211586/#review218362 ::: toolkit/components/extensions/Schemas.jsm:2709 (Diff revision 1) > + } > + } > +} > + > +class SchemaRoots extends Namespaces { > + constructor(root, roots) { These names could be clearer. `targetRoot` and `sourceRoots` maybe? or src/dst or something? ::: toolkit/components/extensions/Schemas.jsm:2756 (Diff revision 1) > + yield* root.getNamespaces(name); > + } > + } > +} > + > +let alreadyInjected = new DefaultWeakMap(() => new Set()); why can't this just be a property of the context?
Attachment #8941298 -
Flags: review?(aswan) → review+
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8941298 [details] Bug 1323845: Part 6a - Support WebExtension-style experiment API provider extensions. https://reviewboard.mozilla.org/r/211586/#review218364 Arg hit publish too early. Please add comments explaining the `Namespaces` and `SchemaRoots` classes. Explaining when and how they are used is more useful than documenting individual methods...
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8941299 [details] Bug 1323845: Part 6b - Test WebExtension-style experiments API providers. https://reviewboard.mozilla.org/r/211588/#review218366
Attachment #8941299 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 33•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8941296 [details] Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules. https://reviewboard.mozilla.org/r/211582/#review218360 > Is there some reason we can't support manifest entries, updates, uninstalls? If not can you at least file a bug to add them later? We need to parse and normalize the manifest without relying on experiment schemas, and I'm hesitant to try to hack in support for experiment manifest entries until I have a good use case. I'd like to support updates and uninstalls, but those are currently implemented in a weird way that have access to an extension instance, and without an extension instance, we have no API managers. I was planning to fix that in a follow-up, though.
Assignee | ||
Comment 34•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8941290 [details] Bug 1323845: Part 1 - Support multiple schema root namespaces. https://reviewboard.mozilla.org/r/211570/#review217688 > all this stuff is duplicated with other tests, can it move to a head.js file? Ideally, yes. But the mocking is all pretty kludgy, at this point, and I'd like to do something more sensible before I try to generalize it enough to be shared between tests. > shouldn't this be "wrong number of arguments" or something? Ideally, yes. But the the overloading rules mean that the number of expected arguments depends on their types, and the argument checking logic isn't smart enough to figure that out.
Assignee | ||
Comment 35•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0764b417e993c0fee98396d1965fdae55ad52a Bug 1323845: Part 0 - Cleanup some code formatting. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/a72e4624c800733ee6b6f557352a884fd6e32d3a Bug 1323845: Part 1 - Support multiple schema root namespaces. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/099c60efb4f1da18636efd8e2abb23492fc57f5b Bug 1323845: Part 2a - Support separate API managers for each extension. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/346bd1b4769ae35b065a7c3ce27d8fb102413c02 Bug 1323845: Part 2b - Support separate schema roots for each API manager. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/43f9533906cfada71309e73d977326b69ef9c7de Bug 1323845: Part 2c - Lazy init API global only when APIs are first loaded. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/77442f18cc027a5cd4bef9a3ac5cdc3d615e3805 Bug 1323845: Part 3 - Auto-create intermediate API namespace objects when necessary. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b83e2001740feec71bf29ec88a00825f3c4c79 Bug 1323845: Part 4 - Add Extension.isPrivileged property. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/179d025d51cdf2c5d0e7eea3357d8f5fd1dc9b6c Bug 1323845: Part 5a - Allow extensions to bundle experiment API modules. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/bbf3129f4a56db20b1806247bc8d4bab0e8a430f Bug 1323845: Part 5b - Add tests for bundled experiments APIs. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/17ee1f919ec1d4a98c9a6ecaab695cf2501992c7 Bug 1323845: Part 6a - Support WebExtension-style experiment API provider extensions. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/e8cb79d5fb06904ad04000042fb23360f145c4b6 Bug 1323845: Part 6b - Test WebExtension-style experiments API providers. r=aswan
Assignee | ||
Comment 36•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/08f4bfbeee04702bf4fb42a3fa4218eb4e2e6ef2 Bug 1323845: Follow-up: Fix Android bustage. r=bustage
Assignee | ||
Comment 37•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0f42e531ca08301185da410b2498446395e568 Bug 1323845: Follow-up: Fix debug bustage. r=bustage
Assignee | ||
Comment 38•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/964afaaa2851efee193c2005488ea26d0d5aca17 Bug 1323845: Follow-up: Fix more bustage. r=bustage
Assignee | ||
Comment 39•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2768e6d9ab52a3d41a7fd116db0c9c4d2d32d8 Bug 1323845: Follow-up: Really fix debug bustage. r=bustage
Assignee | ||
Comment 40•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c513b6a6c7d7681bf213563e79e3c0520c8ee8fe Bug 1323845: Follow-up: Fix more debug bustage in browserSettings normalization. r=bustage
Comment 41•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4a0764b417e9 https://hg.mozilla.org/mozilla-central/rev/a72e4624c800 https://hg.mozilla.org/mozilla-central/rev/099c60efb4f1 https://hg.mozilla.org/mozilla-central/rev/346bd1b4769a https://hg.mozilla.org/mozilla-central/rev/43f9533906cf https://hg.mozilla.org/mozilla-central/rev/77442f18cc02 https://hg.mozilla.org/mozilla-central/rev/e8b83e200174 https://hg.mozilla.org/mozilla-central/rev/179d025d51cd https://hg.mozilla.org/mozilla-central/rev/bbf3129f4a56 https://hg.mozilla.org/mozilla-central/rev/17ee1f919ec1 https://hg.mozilla.org/mozilla-central/rev/e8cb79d5fb06 https://hg.mozilla.org/mozilla-central/rev/08f4bfbeee04 https://hg.mozilla.org/mozilla-central/rev/4f0f42e531ca https://hg.mozilla.org/mozilla-central/rev/964afaaa2851 https://hg.mozilla.org/mozilla-central/rev/8b2768e6d9ab https://hg.mozilla.org/mozilla-central/rev/c513b6a6c7d7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 42•7 years ago
|
||
Pushed by maglione.k@gmail.com https://hg.mozilla.org/integration/mozilla-inbound/rev/c513b6a6c7d7681bf213563e79e3c0520c8ee8fe Follow-up: Fix more debug bustage in browserSettings normalization. r=bustage
Flags: needinfo?(kmaglione+bmo)
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Comment 43•7 years ago
|
||
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(kmaglione+bmo)
Comment 44•7 years ago
|
||
I'm really wondering why the devs here have not provided an example of an combined experiment API. According my findings this bug is not yet a replacement for the legacy experiments. I have migrated the toolbar API from mixedpuppy https://github.com/mixedpuppy/web-ext-toolbar to a combined experiment. As first there was some adjustments needed to get the API working as legacy version for Nightly 59, but this was no problem. Then i migrated this to combined experiment API with help of the tests in this bug. So far so good. But there was always the addon disabled with only remove button in addon manager. I checked the extensions.jsm in profile and found still "dependencies":["toolbar@experiments.addons.mozilla.org"]. I removed the dependency and set it as enabled in the jsm. After next start the addon was able disable and enable again. But from time time the dependency reappears again and it is needed to remove this again and again. I deleted the entire profile to get sure that there is a profile from scratch without old addon installations. But no success, the dependencies comes back. Then i changed the permission to toolbar2 in manifest: "permissions": ["experiments.toolbar2"], and changed the schema.json also to toolbar2 "namespace": "manifest", "types": [ { "$extend": "Permission", "choices": [{ "type": "string", "enum": [ "toolbar2" ] }] }, .... "namespace": "toolbarAction", "description": "Use toolbar actions to add a toolbar to Firefox.", "permissions": ["experiments.toolbar2"], "functions": [ Result in extensions.jsm: ,"dependencies":["toolbar2@experiments.addons.mozilla.org"],"hasEmbeddedWebExtension":false,"userPermissions":{"origins":["moz-extension://a05a4b60-704c-4688-869e-e645a29efa27/*"],"apis":["toolbar2"],"permissions":[]} At the moment i have following results: - a constantly disabled not enable-able addon with a permission property - permission property seems to have a relation to dependencies is extensions.jsm - the registered API browser.toolbarAction seems accessable - at browser start a WARN Loading extension 'null': Reading manifest: Error processing toolbar_action: An unexpected property was found in the WebExtension manifest -> later toolbar_action is available -> Bug 1394140 is very annoying and got only P5. No comments. The new toolbar is also not yet visible if i remove the permissions. No failure message on enable. Need more checks for initialization now.
Comment 45•7 years ago
|
||
latest results: - it looks like the complete script api.js environment will not be executed at startup - if i change the name of the script in manifest to a not exist file, there appears no "file not found" message - if i try to call the API functions in bg i get the expected "file not found" message - if i change back file name and call the API function i get fun is not a function I will stop here now.
Comment 46•7 years ago
|
||
Found still some points in toolbar API experiment. - script file was not executed because there was some commas and semicolons to much which was no problem with the legacy experiments API - fun is not a function was caused by manifest property "paths": [["experiments", "toolbarAction", "setPanel", "getPanel"], was split to 2 arrays now - for execution of the script file i need still to call a API function from bg - on call of API functions i get still module is not a constructor ExtensionCommon.jsm:1203 Toolbar is visible now.
Comment 47•7 years ago
|
||
On mailing list i found a important comment from Kris. The ExtensionAPI constructor must now match the name of the API in the "experiment_apis" object. This fixes the module is not a constructor error. The API script will called from startup now. So for me remains only two problems. The permission problem in comment 44 and I need to disable/enable the addon to get the toolbar visible, which was not the case with the legacy API.
Updated•7 years ago
|
Summary: Allow experiments to include content-side APIs → Allow bundled experiments, and support for content-side APIs
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo) → qe-verify-
Updated•7 years ago
|
Depends on: CVE-2018-12369
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•