Cleaning up contextMenus data structure

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: krizsa, Assigned: krizsa, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla46
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [contextMenus])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
The current data structure is not doing a good job at handling various edge cases of updating items. It should be cleaned up. The preferred way to do this should be too store the item data in a tree structure instead of building the tree structure only when building the actual menu items. We should also detect cycles, or convert an element back from menu to regular element when needed (last child removed).
(Assignee)

Updated

2 years ago
Depends on: 1190667
(Assignee)

Updated

2 years ago
Blocks: 1161828

Updated

2 years ago
Whiteboard: [contextMenus]
Blocks: 1214433
Priority: -- → P1

Updated

2 years ago
Flags: blocking-webextensions+

Updated

2 years ago
Assignee: nobody → gkrizsanits
Iteration: --- → 45.1 - Nov 16

Updated

2 years ago
Iteration: 45.1 - Nov 16 → 45.2 - Nov 30

Updated

2 years ago
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
(Assignee)

Comment 1

a year ago
Created attachment 8700637 [details] [diff] [review]
contextMenus refactoring. v1

Sorry for the lag on thins one, but there were numerous things I wanted to clean up, and I was too busy with other stuff... I think this looks better now.
Attachment #8700637 - Flags: review?(wmccloskey)
(Assignee)

Comment 2

a year ago
Created attachment 8700639 [details] [diff] [review]
Cycle check. v1
Attachment #8700639 - Flags: review?(wmccloskey)
Kris, is there any chance you could review this?
Flags: needinfo?(kmaglione+bmo)
(In reply to Bill McCloskey (:billm) from comment #3)
> Kris, is there any chance you could review this?

Sure
Flags: needinfo?(kmaglione+bmo)
Attachment #8700637 - Flags: review?(wmccloskey) → review?(kmaglione+bmo)
Attachment #8700639 - Flags: review?(wmccloskey) → review?(kmaglione+bmo)
Comment on attachment 8700637 [details] [diff] [review]
contextMenus refactoring. v1

Review of attachment 8700637 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-contextMenus.js
@@ +72,5 @@
> +    // root menu element for the extension.
> +    if (element.firstChild &&
> +        element.firstChild.childNodes.length == 1) {
> +      let menuPopup = element.firstChild;
> +      let onlyChild = menuPopup.removeChild(menuPopup.firstChild);

This is a bit hard to read. Can you make it something like:

let menuPopup = element.firstChild;
if (menuPopup && menuPopup.childNodes.length == 1) {
  let onlyChild = menuPopup.firstChild;
  onlyChild.remove();
  return onlyChild;
}

@@ +102,5 @@
> +    element.appendChild(menupopup);
> +    return element;
> +  },
> +
> +  customiseElement(element, item, contextData) {

|customizeElement|

@@ +110,5 @@
> +    if (!item.enabled) {
> +      element.setAttribute("disabled", true);
> +    }
> +
> +    if (item.onclick) {

This needs to be updated for bug 1190688.

@@ +127,4 @@
>    handleEvent: function(event) {
> +    if (this.xulMenu != event.target) {
> +      return;
> +    }

Please check the |event.type| here, if only so people reading this know what event we're handling.

@@ +135,1 @@
>        target.removeChild(item);

|item.remove()|

@@ +199,5 @@
> +    if (createProperties.wrappedJSObject) {
> +      // TODO: argument validation should already check if it's safe to
> +      // waive xray wrappers and pass in the waived version already, but
> +      // for now we have to do this. (we need to waive because of onclick)
> +      createProperties = createProperties.wrappedJSObject;

This shouldn't be necessary since bug 1208257 (mostly) landed. If it is, please use |Cu.waiveXrays| instead of |.wrappedJSObject|, and |unwaiveXrays| the values.

@@ +204,3 @@
>      }
>  
> +    for (propName in createProperties) {

|let propName|

@@ +254,5 @@
> +      // any item with the given id, we should throw.
> +      throw new Error("Could not find any MenuItem with id: " + parentId);
> +    }
> +
> +    this._parentId = parentId;

I don't like that we have separate |parent| and |_parentId| properties. Can we make the |parentId| getter just retrieve the ID from |parent|?

@@ +261,5 @@
> +  get parentId() {
> +    return this._parentId;
> +  },
> +
> +  addChild(child) {

It would be good to check that |child| is parentless here, and either throw or detach it if it's not.

@@ +267,5 @@
> +    child.parent = this;
> +  },
> +
> +  detachChild(child) {
> +    let idx = this.children.indexOf(child);

Please check that child is actually a member of |children|.

@@ +297,5 @@
> +
> +    let menuMap = contextMenuMap.get(this.extension);
> +    menuMap.delete(this.id);
> +    if (this.root == this) {
> +      rootItems.delete(this);

|.delete(this.extension)|

@@ +344,4 @@
>      let contexts = getContexts(contextData);
> +    let intersect = this.contexts.filter(n => contexts.has(n));
> +      if (!intersect.length) {
> +        return false;

There's extra indentation here. Also, this could be |if (!this.contexts.some(n => contexts.has(n)))|.
Attachment #8700637 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8700639 [details] [diff] [review]
Cycle check. v1

Review of attachment 8700639 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-contextMenus.js
@@ +278,5 @@
> +
> +    // Check for cycle.
> +    let visitedItems = new Set();
> +    visitedItems.add(this);
> +    let cycleFound = this.traverseChildren(visitedItems);

This doesn't seem necessary. It should be enough to check that this node isn't an ancestor (or self) of |newParent| before making any changes.

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js
@@ +47,5 @@
>          { title: "child2", parentId: parentToDel, onclick: genericOnClick });
>        browser.contextMenus.remove(parentToDel);
>  
> +      try() {
> +        browser.contextMenus.update(parent, { parentId: child2 });

We need to fail (rather than just time out) if this doesn't throw.
Attachment #8700639 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 7

a year ago
Created attachment 8702549 [details] [diff] [review]
parentId validation. v1

(In reply to Kris Maglione [:kmag] from comment #6)
> This doesn't seem necessary. It should be enough to check that this node
> isn't an ancestor (or self) of |newParent| before making any changes.

Thanks, you're totally right. I over-complicated this. Also if parentId is undefined the node should be added to the root as a child. I hope this looks better now.
Attachment #8700639 - Attachment is obsolete: true
Attachment #8702549 - Flags: review?(kmaglione+bmo)
Attachment #8702549 - Flags: review?(kmaglione+bmo) → review+

Comment 8

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a99460bbb350
https://hg.mozilla.org/integration/mozilla-inbound/rev/efd432a79df6

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a99460bbb350
https://hg.mozilla.org/mozilla-central/rev/efd432a79df6
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
https://hg.mozilla.org/integration/fx-team/rev/e657460f49969b218ccaca26b158b46e7baf9524
Bug 1211366: Follow-up: Fix ESLint errors. r=trivial

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e657460f4996
You need to log in before you can comment on or make changes to this bug.