Closed
Bug 1211366
Opened 9 years ago
Closed 9 years ago
Cleaning up contextMenus data structure
Categories
(WebExtensions :: Untriaged, defect, P1)
WebExtensions
Untriaged
Tracking
(firefox46 fixed)
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: gkrizsanits, Assigned: gkrizsanits, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [contextMenus])
Attachments
(2 files, 1 obsolete file)
22.19 KB,
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
2.80 KB,
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Blocks: webextensions-chrome-gaps
Updated•9 years ago
|
Whiteboard: [contextMenus]
Blocks: webext
Priority: -- → P1
Updated•9 years ago
|
Flags: blocking-webextensions+
Updated•9 years ago
|
Assignee: nobody → gkrizsanits
Iteration: --- → 45.1 - Nov 16
Updated•9 years ago
|
Iteration: 45.1 - Nov 16 → 45.2 - Nov 30
Updated•9 years ago
|
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
Attachment #8700639 -
Flags: review?(wmccloskey)
Kris, is there any chance you could review this?
Flags: needinfo?(kmaglione+bmo)
Comment 4•9 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #3)
> Kris, is there any chance you could review this?
Sure
Flags: needinfo?(kmaglione+bmo)
Updated•9 years ago
|
Attachment #8700637 -
Flags: review?(wmccloskey) → review?(kmaglione+bmo)
Updated•9 years ago
|
Attachment #8700639 -
Flags: review?(wmccloskey) → review?(kmaglione+bmo)
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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•9 years ago
|
||
(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)
Updated•9 years ago
|
Attachment #8702549 -
Flags: review?(kmaglione+bmo) → review+
Comment 9•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a99460bbb350
https://hg.mozilla.org/mozilla-central/rev/efd432a79df6
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e657460f49969b218ccaca26b158b46e7baf9524
Bug 1211366: Follow-up: Fix ESLint errors. r=trivial
Comment 11•9 years ago
|
||
bugherder |
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•