Introduce browser.menus, an alias for browser.contextMenus

RESOLVED FIXED in Firefox 55

Status

defect
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: zombie, Assigned: zombie)

Tracking

({dev-doc-complete})

unspecified
mozilla55
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [design-decision-approved] triaged)

Attachments

(3 attachments)

Assignee

Description

2 years ago
+++ 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

Updated

2 years ago
Assignee: nobody → tomica
No longer blocks: 1311472
Severity: enhancement → normal
Priority: P3 → --
Whiteboard: [design-decision-approved]triaged

Comment 1

2 years ago
need to decide how we're going to handle the menus.  kris will comment
Flags: needinfo?(kmaglione+bmo)

Comment 2

2 years ago
This wouldn't affect chrome.contextMenus, would it?

Updated

2 years ago
Whiteboard: [design-decision-needed] triaged
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Flags: needinfo?(kmaglione+bmo)
Summary: Introduce browser.menus, deprecate browser.contextMenus → Introduce browser.menus, an alias for browser.contextMenus
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 8

2 years ago
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.
Assignee

Updated

2 years ago
Attachment #8860675 - Flags: review?(kmaglione+bmo)
Attachment #8860676 - Flags: review?(kmaglione+bmo)
Attachment #8861622 - Flags: review?(kmaglione+bmo)

Updated

2 years ago
Whiteboard: [design-decision-needed] triaged → [design-decision-approved] triaged

Comment 9

2 years ago
mozreview-review
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 10

2 years ago
mozreview-review
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 11

2 years ago
mozreview-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)
Assignee

Comment 12

2 years ago
mozreview-review
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.
Assignee

Comment 13

2 years ago
mozreview-review-reply
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.
Assignee

Comment 14

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 21

2 years ago
mozreview-review-reply
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.
Assignee

Comment 22

2 years ago
mozreview-review-reply
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 23

2 years ago
mozreview-review-reply
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 24

2 years ago
mozreview-review
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 25

2 years ago
mozreview-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+

Comment 26

2 years ago
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)

Comment 28

2 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Flags: needinfo?(tomica)

Comment 32

2 years ago
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).

Comment 35

2 years ago
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)
Assignee

Comment 38

2 years ago
Sorry for the delay, but looks good, thanks.
Flags: needinfo?(tomica)

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.