Closed Bug 1211366 Opened 6 years ago Closed 6 years ago

Cleaning up contextMenus data structure

Categories

(WebExtensions :: Untriaged, defect, P1)

defect

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Iteration:
45.3 - Dec 14
Tracking Status
firefox46 --- fixed

People

(Reporter: gkrizsanits, Assigned: gkrizsanits, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [contextMenus])

Attachments

(2 files, 1 obsolete file)

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).
Depends on: 1190667
Whiteboard: [contextMenus]
Flags: blocking-webextensions+
Assignee: nobody → gkrizsanits
Iteration: --- → 45.1 - Nov 16
Iteration: 45.1 - Nov 16 → 45.2 - Nov 30
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
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)
Attached patch Cycle check. v1 (obsolete) — Splinter Review
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)
(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+
https://hg.mozilla.org/mozilla-central/rev/a99460bbb350
https://hg.mozilla.org/mozilla-central/rev/efd432a79df6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.