Closed Bug 652842 Opened 9 years ago Closed 8 years ago

_getToolbarItem(aId) returns null if the toolbaritem is on the Add-on Bar

Categories

(Toolkit :: Toolbars and Toolbar Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: tabutils+bugzilla, Assigned: tabutils+bugzilla)

References

Details

Attachments

(1 file, 2 obsolete files)

1.03 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110425 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110425 Firefox/6.0a1

See the following steps.


Reproducible: Always

Steps to Reproduce:
1. Customize and move the New Window button to the Add-on Bar
2. Execute in JS shell:
document.getElementById("nav-bar")._getToolbarItem("new-window-button")


Actual Results:  
The call returns null

Expected Results:  
The call returns the New Window button


http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/toolbar.xml#282

308                 // look for an item with the same id, as the item may be
309                 // in a different toolbar.
310                 var item = document.getElementById(aId);
311                 if (item && item.parentNode && item.parentNode.parentNode == toolbox) {
312                   newItem = item;
313                   break;
314                 }

The problem lies in line 311. "item.parentNode.parentNode" should be "item.parentNode.toolbox".
Blocks: 595937
Component: Toolbars → Toolbars and Toolbar Customization
Product: Firefox → Toolkit
QA Contact: toolbars → toolbars
This is new code which has been landed with Neil's patch on bug 354048.
Blocks: 354048
(In reply to comment #1)
> This is new code which has been landed with Neil's patch on bug 354048.

I don't see it newer than Firefox 4.0.
Besides, it seems this method should belong to the toolbox, not the toolbar.
Some comments from https://bugzilla.mozilla.org/show_bug.cgi?id=599029#c5.
> The problem lies in line 311. "item.parentNode.parentNode" should be "item.parentNode.toolbox".

Can you make a patch which fixes this?
Attached patch patch (obsolete) — Splinter Review
Attachment #536829 - Flags: review?(enndeakin)
Comment on attachment 536829 [details] [diff] [review]
patch

This looks ok. You may want to do an extra conditional check of either of:
  1. 'toolbox' in item.parentNode
  2. item.parentNode.localName == "toolbar"

to avoid an error in the case where someone incorrectly used a non-toolbar id.
Attachment #536829 - Flags: review?(enndeakin) → review+
We have checked !toolbox and item.parentNode.toolbox == toolbox, which implies 'toolbox' in item.parentNode. I think it's enough, isn't it?
Yeah, it's not a big issue, but item.parentNode.toolbox == toolbox will cause a warning (in js strict mode) if item.parentNode isn't a toolbar as it won't have a toolbox property.
(In reply to comment #9)
> Yeah, it's not a big issue, but item.parentNode.toolbox == toolbox will
> cause a warning (in js strict mode) if item.parentNode isn't a toolbar as it
> won't have a toolbox property.

I see. Then it makes some sense.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #536829 - Attachment is obsolete: true
Attachment #537097 - Flags: review?(enndeakin)
Attachment #537097 - Flags: review?(enndeakin) → review+
Keywords: checkin-needed
Assignee: nobody → ithinc
malformed patch a/toolkit/content/widgets/toolbar.xml @@ -303,17 +303,17 @@
Keywords: checkin-needed
Attached patch patch v3Splinter Review
(In reply to comment #12)
> malformed patch a/toolkit/content/widgets/toolbar.xml @@ -303,17 +303,17 @@

Thanks, corrected.
Attachment #537097 - Attachment is obsolete: true
Attachment #538671 - Flags: review?(enndeakin)
Attachment #538671 - Flags: review?(enndeakin) → review-
no reason to stay unconfirmed if there is a patch, right?
Status: UNCONFIRMED → NEW
Ever confirmed: true
> no reason to stay unconfirmed if there is a patch, right?
Right, setting status to ASSIGNED.
Status: NEW → ASSIGNED
Attachment #538671 - Flags: review- → review?(enndeakin)
Comment on attachment 538671 [details] [diff] [review]
patch v3

I already reviewed this, no? Did you mean to attach a different version?

Or is the patch ready to be checked in?
Attachment #538671 - Flags: review?(enndeakin) → review+
I have no idea why you gave a review- for patch v3. Maybe it was a mis-operation. Patch v2 was malformed, but v3 should be ok. It is ready to be checked in.
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/cffa5e5bb0f6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.