Closed Bug 1333403 Opened 8 years ago Closed 7 years ago

Introduce browser.menus, an alias for browser.contextMenus

Categories

(WebExtensions :: Frontend, defect)

Product:
WebExtensions
Component:
Frontend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: zombie, Assigned: zombie)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved] triaged)

Attachments

(3 files)

Bug 1333403 - Part 1: Implement $import keyword for schema namespaces
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1333403 - Part 2: Implement browser.menus as alias for contextMenus
59 bytes, text/x-review-board-request
kmag
: review+
Details
Bug 1333403 - Part 3: Fix using browser.menus from multiple contexts
59 bytes, text/x-review-board-request
kmag
: review+
Details 
+++ This bug was initially created as a clone of Bug #1268020 +++

Since we are adding the "main_menu" context in bug 1268020, perhaps it makes more sense to introduce a new browser.menus API to cover all menu-related needs, and deprecate contextMenus (keep it as an alias for now).
Assignee: nobody → tomica
No longer blocks: 1311472
Severity: enhancement → normal
Priority: P3 → --
Whiteboard: [design-decision-approved]triaged
need to decide how we're going to handle the menus.  kris will comment
Flags: needinfo?(kmaglione+bmo)
This wouldn't affect chrome.contextMenus, would it?
Whiteboard: [design-decision-needed] triaged
Status: NEW → ASSIGNED
Flags: needinfo?(kmaglione+bmo)
Summary: Introduce browser.menus, deprecate browser.contextMenus → Introduce browser.menus, an alias for browser.contextMenus
I left most of the tests still using browser.contextMenus, as that's what everyone is currently using.  

We can switch/update the tests incrementally, as we go along.
Attachment #8860675 - Flags: review?(kmaglione+bmo)
Attachment #8860676 - Flags: review?(kmaglione+bmo)
Attachment #8861622 - Flags: review?(kmaglione+bmo)
Whiteboard: [design-decision-needed] triaged → [design-decision-approved] triaged
Comment on attachment 8860675 [details]
Bug 1333403 - Part 1: Implement $import keyword for schema namespaces

https://reviewboard.mozilla.org/r/132626/#review149404

::: toolkit/components/extensions/Schemas.jsm:2250
(Diff revision 1)
>        if (schema[prop]) {
>          this[prop] = schema[prop];
>        }
>      }
> +
> +    if (schema.$import) {

I'd rather not use $names outside of the schema JSON. Also, can we call this something more descriptive, like "superNamespace"?

::: toolkit/components/extensions/Schemas.jsm:2265
(Diff revision 1)
> +    // If this is init()ed before the namespaces that $import it.
> +    for (let ns of this._importedBy) {
> +      if (ns._lazySchemas) {
> +        ns._lazySchemas.unshift(...this._lazySchemas);
> +      }
> +    }
> +    this._importedBy = [];
> +
> +    // If this is init()ed before the namespace it $imports.
> +    if (this.$import && this.$import._lazySchemas) {
> +      this._lazySchemas.unshift(...this.$import._lazySchemas);
> +    }
> +    this.$import = null;

Can we just add a separate `_initialized` property so the schemas that import this can just grab the lazy schemas from the namespace that they extend, and not worry about who was initialized first?
Attachment #8860675 - Flags: review?(kmaglione+bmo)
Comment on attachment 8860676 [details]
Bug 1333403 - Part 2: Implement browser.menus as alias for contextMenus

https://reviewboard.mozilla.org/r/132628/#review149408

::: browser/components/extensions/ext-browser.js:157
(Diff revision 3)
>      scopes: ["addon_parent"],
>      paths: [
>        ["history"],
>      ],
>    },
> +  menusInternal: {

I'd rather we just call this `menus`, in both the parent and the child.
Attachment #8860676 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8861622 [details]
Bug 1333403 - Part 3: Fix using browser.menus from multiple contexts

https://reviewboard.mozilla.org/r/133606/#review149410

::: browser/components/extensions/ext-menus.js:605
(Diff revision 1)
>  this.menusInternal = class extends ExtensionAPI {
> +  constructor(extension) {
> +    super(extension);
> +
> +    gMenuMap.set(extension, new Map());
> +    if (++gExtensionCount == 1) {

Please remove this variable and just check `gMenuMap.size`

::: browser/components/extensions/test/browser/browser_ext_menus.js:215
(Diff revision 1)
> +    browser.menus.create({id: "parent", title: "parent"});
> +    browser.tabs.create({url: "tab.html", active: false});
> +  }
> +
> +  const files = {
> +    "tab.html": "<!doctype html><meta charset=utf-8><script src=tab.js></script>",

Nit: s/doctype/DOCTYPE/

::: toolkit/components/extensions/ExtensionCommon.jsm
(Diff revision 1)
>    }
>  
> -  _initModule(info, cls) {
> -    // FIXME: This both a) does nothing, and b) is not used anymore.
> -    cls.namespaceName = cls.namespaceName;
> -    cls.scopes = new Set(info.scopes);

`cls.scopes` is definitely still used, unless I'm missing something. And `cls.namespaceName` is still checked, despite being assigned the wrong value here.
Attachment #8861622 - Flags: review?(kmaglione+bmo)
Comment on attachment 8860675 [details]
Bug 1333403 - Part 1: Implement $import keyword for schema namespaces

https://reviewboard.mozilla.org/r/132626/#review149416

::: toolkit/components/extensions/Schemas.jsm:2250
(Diff revision 1)
>        if (schema[prop]) {
>          this[prop] = schema[prop];
>        }
>      }
> +
> +    if (schema.$import) {

I suspect this comment was meant for the line below?

::: toolkit/components/extensions/Schemas.jsm:2265
(Diff revision 1)
> +    // If this is init()ed before the namespaces that $import it.
> +    for (let ns of this._importedBy) {
> +      if (ns._lazySchemas) {
> +        ns._lazySchemas.unshift(...this._lazySchemas);
> +      }
> +    }
> +    this._importedBy = [];
> +
> +    // If this is init()ed before the namespace it $imports.
> +    if (this.$import && this.$import._lazySchemas) {
> +      this._lazySchemas.unshift(...this.$import._lazySchemas);
> +    }
> +    this.$import = null;

Once this namespace `A` is initialized, the `_lazySchemas` is nulled out, so the namespace that imports it `B`, and is initialized after `A` doesn't have anything to look at.

The alternative is for the initialization of `B` to block until `A` is initialized, which means we need to guard for infinite recursion, and in the end, code might not be any prittyer.
Comment on attachment 8860676 [details]
Bug 1333403 - Part 2: Implement browser.menus as alias for contextMenus

https://reviewboard.mozilla.org/r/132628/#review149408

> I'd rather we just call this `menus`, in both the parent and the child.

This caused issues when the module is called `menus`, but the extension only uses the `contextMenus` permission.  `menusInternal` is my solution to avoid having to change the permissions/injection logic.
Comment on attachment 8861622 [details]
Bug 1333403 - Part 3: Fix using browser.menus from multiple contexts

https://reviewboard.mozilla.org/r/133606/#review149410

> Nit: s/doctype/DOCTYPE/

nit: lowercase `!doctype` is perfectly cromulent according to the spec: https://dev.w3.org/html5/html-author/#doctype-declaration

> `cls.scopes` is definitely still used, unless I'm missing something. And `cls.namespaceName` is still checked, despite being assigned the wrong value here.

Both `scopes` and `namespaceName` are definitely used, but as properties on the module descriptor, not on the *class* that implements the module.

(We've had this discussion over irc: all our tests pass with this code removed)
Comment on attachment 8860675 [details]
Bug 1333403 - Part 1: Implement $import keyword for schema namespaces

https://reviewboard.mozilla.org/r/132626/#review149404

> Can we just add a separate `_initialized` property so the schemas that import this can just grab the lazy schemas from the namespace that they extend, and not worry about who was initialized first?

Yeah, I presumed we were deleting `_lazySchemas` for memory reasons, but it looks like we keep virtually all parts of of the schema in `this[type]` maps, so this makes sense and is much cleaner.
Comment on attachment 8860676 [details]
Bug 1333403 - Part 2: Implement browser.menus as alias for contextMenus

https://reviewboard.mozilla.org/r/132628/#review149408

> This caused issues when the module is called `menus`, but the extension only uses the `contextMenus` permission.  `menusInternal` is my solution to avoid having to change the permissions/injection logic.

But I added the comment explaining this.
Comment on attachment 8861622 [details]
Bug 1333403 - Part 3: Fix using browser.menus from multiple contexts

https://reviewboard.mozilla.org/r/133606/#review149410

> nit: lowercase `!doctype` is perfectly cromulent according to the spec: https://dev.w3.org/html5/html-author/#doctype-declaration

It's legal, yes, but contrary to our usual style.
Comment on attachment 8860675 [details]
Bug 1333403 - Part 1: Implement $import keyword for schema namespaces

https://reviewboard.mozilla.org/r/132626/#review152078
Attachment #8860675 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8861622 [details]
Bug 1333403 - Part 3: Fix using browser.menus from multiple contexts

https://reviewboard.mozilla.org/r/133606/#review152082
Attachment #8861622 - Flags: review?(kmaglione+bmo) → review+
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/37b0fcb54cba
Part 1: Implement $import keyword for schema namespaces r=kmag
https://hg.mozilla.org/integration/autoland/rev/cfc47df74537
Part 2: Implement browser.menus as alias for contextMenus r=kmag
https://hg.mozilla.org/integration/autoland/rev/d842c744941e
Part 3: Fix using browser.menus from multiple contexts r=kmag
Backed out for eslint failure: browser/components/extensions/ext-c-menus.js:158:24 | 'SingletonEventManager' is not defined:

https://treeherder.mozilla.org/logviewer.html#?job_id=106080449&repo=autoland

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d842c744941efbbcc04136fcce2eb42fd1fb76f2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=106080449&repo=autoland
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/extensions/ext-c-menus.js:158:24 | 'SingletonEventManager' is not defined. (no-undef)
Flags: needinfo?(tomica)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/1bd81296c74c
Backed out changeset d842c744941e 
https://hg.mozilla.org/integration/autoland/rev/30819ae5b63f
Backed out changeset cfc47df74537 
https://hg.mozilla.org/integration/autoland/rev/860e84e33e70
Backed out changeset 37b0fcb54cba for eslint failure: browser/components/extensions/ext-c-menus.js:158:24 | 'SingletonEventManager' is not defined. r=backout
Flags: needinfo?(tomica)
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ad0c7334477b
Part 1: Implement $import keyword for schema namespaces r=kmag
https://hg.mozilla.org/integration/autoland/rev/d9488457b8de
Part 2: Implement browser.menus as alias for contextMenus r=kmag
https://hg.mozilla.org/integration/autoland/rev/5cf9d6d03da7
Part 3: Fix using browser.menus from multiple contexts r=kmag
For the docs, I think we should move the "contextMenus" tree to "menus". We should then include notes on all pages, or at least on the main page, saying that this functionality is (almost) all available via the "contextMenus" namespace, and that using this namespace will enable compatibility with Chrome (and Edge, I assume).
Can you update the compat tables of all menus/contextMenus APIs to show a note about the "menus" vs "contextMenus" namespace? With the current docs, it seems that "menus" is the recommended way to write a WebExtension, but the support is very meager (Firefox 55 onwards, no other browser supports it).
Flags: needinfo?(wbamberg)
Rob, yes, I've made a PR for this but it's not yet merged: https://github.com/mdn/browser-compat-data/pull/367. It doesn't indicate that the renaming is from 55 though, I'll add that note too.
Flags: needinfo?(wbamberg)
OK, I've done all this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus.

The compat tables indicate that the APIs are only available using the "contextMenus" namespace in other browsers, and also in Firefox before version 56. For example: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/create#Browser_compatibility. I think we could probably present this in a more understandable way, but that will have to feed into the work to redesign the compat tables across MDN, which is coming up soon.

Let me know if this looks all right.
Flags: needinfo?(tomica)
Sorry for the delay, but looks good, thanks.
Flags: needinfo?(tomica)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: