neil@parkwaycc.co.uk Assignee
	Description • 17 years ago

* The toolbarpalette is invisible, but the toolbox removes it anyway.
  (This means that customised nodes may not exist in the document.)
* The toolbox uses clones of the palette items when it could just move then.
* createWrapper sometimes uses the wrong document.
* wrapPaletteItem should be passed an imported node to avoid adopting it.
* wrapToolbarItem should insert the wrapper as well as removing the node.

	neil@parkwaycc.co.uk Assignee
	Comment 1 • 17 years ago

This seems to work, at least on Windows. Of course I'm somewhat limited because I'm using attachment 292316 [details] [diff] [review] but I've removed all the null-checks to be sure ;-)

Apparently toolbar customisation works differently on the Mac?

	Robert Kaiser
	Comment 2 • 16 years ago

This is the same patch unbitrotted to my best knowledge. Note that with my first tries on testing this, I saw some problems in Minefield, but when I wanted to start debugging this, it suddenly started working as expected.
Only when I "Restore Default Set" from the customization palette, I get empty items in the palette and an exception on closing the palette, leading to menubar and the "customize" entry staying disabled (not the other toolbars though):
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsIDOMXULElement.replaceChild]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://global/content/customizeToolbar.js :: unwrapToolbarItems :: line 183"  data: no]

This might be caused by the restore counting on the old mechanics, though, I'm not sure.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 4 • 16 years ago

Comment on attachment 302343 [details] [diff] [review]
Fix for restore default set

I don't understand:
-    var title = stringBundle.getString(aItem.id + "Title");
+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");

And I think:
     // Attempt to locate an item with a matching id within palette.
-    var paletteItem = this.parentNode.palette.firstChild;
-    while (paletteItem) {
-    var paletteId = paletteItem.id;
-    if (paletteId == aId) {
-      newItem = paletteItem.cloneNode(true);
-      break;
-    }
-    paletteItem = paletteItem.nextSibling;
-    }
+    newItem = document.getElementById(aId);

will break firefox, because we have children of elements in the palette that have the same IDs as elements that used to be children of the palette, and we are specifically relying on the fact that that code only checks children.

I'm also a bit concerned that keeping the palette in the document will have unintended consequences.

r- because of the second issue, but that should be easily fixable. I haven't had a chance to test this yet.

	Robert Kaiser
	Comment 5 • 16 years ago

Gavin, is it feasible to still try fixing this in toolkit, or do you think it's not the right thing to do? If it's worth going this way, is it something that can be considered for 1.9.1?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 6 • 16 years ago

I think it's definitely the right thing to do, we just need to make sure it doesn't cause bustage for Firefox or other consumers. We could certainly do it for 1.9.1 - with my comments in comment 4 addressed I think it might be good to go?

	neil@parkwaycc.co.uk Assignee
	Comment 7 • 16 years ago

(In reply to comment #4)
>(From update of attachment 302343 [details] [diff] [review])
>I don't understand:
>>-    var title = stringBundle.getString(aItem.id + "Title");
>>+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");
Actually I don't understand how the old code worked, unless somehow the cloned spacer/separator/springs always had the unique id rather than the random one.

> And I think:
>>     // Attempt to locate an item with a matching id within palette.
>>-    var paletteItem = this.parentNode.palette.firstChild;
>>-    while (paletteItem) {
>>-    var paletteId = paletteItem.id;
>>-    if (paletteId == aId) {
>>-      newItem = paletteItem.cloneNode(true);
>>-      break;
>>-    }
>>-    paletteItem = paletteItem.nextSibling;
>>-    }
>>+    newItem = document.getElementById(aId);
>will break firefox, because we have children of elements in the palette that
>have the same IDs as elements that used to be children of the palette, and we
>are specifically relying on the fact that that code only checks children.
Ah, but I'm not cloning them any more, so you'll only ever have the one node (per document, that is - the customise dialog still has its own clones).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 8 • 16 years ago

(In reply to comment #7)
> >will break firefox, because we have children of elements in the palette that
> >have the same IDs as elements that used to be children of the palette, and we
> >are specifically relying on the fact that that code only checks children.

> Ah, but I'm not cloning them any more, so you'll only ever have the one node
> (per document, that is - the customise dialog still has its own clones).

I don't see how that's relevant - the problem is not that there are multiple nodes with the same ID, the problem is that one of the toolbar's children has elements whose IDs are the IDs of a previous version's top-level elements. For example, in Firefox 2 we had "back-button" and "forward-button" elements that could be customized, but in Firefox 3 we have only a "unified-back-forward-button", which has child elements with IDs "back-button" and "forward-button".

On upgrade, the code will look for the previous version's elements (since that's what's stored in localStore.rdf), and we're currently relying on the fact that it won't find them and just fail to add them to the toolbar. With this change, as far as I can tell we'll rip out the child element from it's parent and insert it into the toolbar.

That code will also find elements that are outside of the palette, which might be unexpected by current users.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 16 years ago

(In reply to comment #7)
> (In reply to comment #4)
> >(From update of attachment 302343 [details] [diff] [review])
> >I don't understand:
> >>-    var title = stringBundle.getString(aItem.id + "Title");
> >>+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");
> Actually I don't understand how the old code worked, unless somehow the cloned
> spacer/separator/springs always had the unique id rather than the random one.

Looking at this again, I don't even know why this code exists. What is the title used for anyways?

	neil@parkwaycc.co.uk Assignee
	Comment 10 • 16 years ago

(In reply to comment #9)
>(In reply to comment #7)
>>(In reply to comment #4)
>>>(From update of attachment 302343 [details] [diff] [review] [details])
>>>I don't understand:
>>>>-    var title = stringBundle.getString(aItem.id + "Title");
>>>>+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");
>>Actually I don't understand how the old code worked, unless somehow the cloned
>>spacer/separator/springs always had the unique id rather than the random one.
>Looking at this again, I don't even know why this code exists. What is the
>title used for anyways?
I think it's an extra label to be shown in the palette for items that don't normally have one.

	neil@parkwaycc.co.uk Assignee
	Comment 11 • 16 years ago

(In reply to comment #4)
>(From update of attachment 302343 [details] [diff] [review])
>And I think:
>>     // Attempt to locate an item with a matching id within palette.
>>-    var paletteItem = this.parentNode.palette.firstChild;
>>-    while (paletteItem) {
>>-    var paletteId = paletteItem.id;
>>-    if (paletteId == aId) {
>>-      newItem = paletteItem.cloneNode(true);
>>-      break;
>>-    }
>>-    paletteItem = paletteItem.nextSibling;
>>-    }
>>+    newItem = document.getElementById(aId);
>will break firefox, because we have children of elements in the palette that
>have the same IDs as elements that used to be children of the palette, and we
>are specifically relying on the fact that that code only checks children.
Sorry for the misunderstanding last time.

I think I need to check that the element's grandparent is the toolbox - I'd prefer that to checking that the parent is the palette, if possible.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 12 • 16 years ago

(In reply to comment #11)
> I think I need to check that the element's grandparent is the toolbox - I'd
> prefer that to checking that the parent is the palette, if possible.

That wouldn't address the main problem, because in this case the elements' grandparent *is* the toolbox, but we still don't want them found. It would address the (minor) second concern, though (getting elements from elsewhere in the document).

I'm not sure why you want to avoid checking the parent - what's wrong with that?

	neil@parkwaycc.co.uk Assignee
	Comment 13 • 16 years ago

(In reply to comment #12)
>That wouldn't address the main problem, because in this case the elements'
>grandparent *is* the toolbox, but we still don't want them found. It would
>address the (minor) second concern, though (getting elements from elsewhere in
>the document).
No, the back button's parent is the unified button, whose parent is the toolbar, whose parent is the toolbox.

>I'm not sure why you want to avoid checking the parent - what's wrong with that?
I think this code might get called to move items between toolbars. I also thought there was another reason, which I've since forgotten.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 14 • 16 years ago

(In reply to comment #10)
> I think it's an extra label to be shown in the palette for items that don't
> normally have one.

I couldn't get it to appear when hovering over any of the "special" items, on Windows or Mac. Perhaps it should be removed?

(In reply to comment #13)
Ah, I see. Yes, that could work. So with that check this patch is probably OK, but it needs to be unbitrotted.

	neil@parkwaycc.co.uk Assignee
	Comment 15 • 16 years ago

(In reply to comment #14)
> (In reply to comment #10)
> > I think it's an extra label to be shown in the palette for items that don't
> > normally have one.
> I couldn't get it to appear when hovering over any of the "special" items, on
> Windows or Mac.
Not hovering, but when customising, the spacer/spring/separator are labelled.

> (In reply to comment #13)
> Ah, I see. Yes, that could work. So with that check this patch is probably OK,
> but it needs to be unbitrotted.
Yes, by backing out a patch to a bug that this bug incidentally fixes :-)

	neil@parkwaycc.co.uk Assignee
	Comment 16 • 16 years ago

I haven't actually tested this yet, but it's not wildly different than before.

	Robert Kaiser
	Comment 17 • 16 years ago

I just tested this with Firefox, and it works fine - with one exception: When I add a new toolbar, drag some items into it, and then reset to default, those items are gone from the palette and the UI, needing a Firefox restart to get them back in the palette.
Everything else looks good in testing!

	neil@parkwaycc.co.uk Assignee
	Comment 18 • 16 years ago

* Emptied out custom toolbars before removing them from the DOM.

	neil@parkwaycc.co.uk Assignee
	Comment 19 • 16 years ago

My fix for comment 8 broke springs/spaces/separators...

	Robert Kaiser
	Comment 20 • 16 years ago

Thanks a lot, the current patch really looks good in all my Firefox customize testing!

	neil@parkwaycc.co.uk Assignee
	Comment 21 • 16 years ago

Comment on attachment 338158 [details] [diff] [review]
Fix regression from comment 8

>-            // Hold on to the palette but remove it from the document.
>             toolbox.palette = node;       
>-            toolbox.removeChild(node);
If you don't want this change then that's not a problem, we can work around it in our derived binding; just remind me to remove it before check in.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 22 • 16 years ago

(In reply to comment #21)
> (From update of attachment 338158 [details] [diff] [review])
> >-            // Hold on to the palette but remove it from the document.
> >             toolbox.palette = node;       
> >-            toolbox.removeChild(node);
> If you don't want this change then that's not a problem, we can work around it
> in our derived binding; just remind me to remove it before check in.

Right, I don't think we can take this change. Undoing it requires undoing your changes to insertItem in toolbar.xml as well though, doesn't it? getElementById won't find the element in the palette if it's not in the document. Can you post an updated patch that works?

	neil@parkwaycc.co.uk Assignee
	Comment 23 • 16 years ago

Untested... I was too busy losing a chess game (fortunately the team still won)

	Robert Kaiser
	Comment 24 • 16 years ago

Comment on attachment 341027 [details] [diff] [review]
Do things the hard way

>diff -r 432d71ba1387 toolkit/content/widgets/toolbar.xml
>--- a/toolkit/content/widgets/toolbar.xml	Sat Sep 20 00:20:08 2008 +0200
>+++ b/toolkit/content/widgets/toolbar.xml	Tue Sep 30 00:22:47 2008 +0100
>+              if (!newItem || !newItem.parentNode ||
>+                  newItem.parentNode.parentNode != this.parentNode)
>+                return false;

The "newItem.parentNode.parentNode != this.parentNode" part of this check results in no items appearing in the toolbar now - if I remove it, the items appear.

Additionally, I suddenly have back, forward and the history dropdown appearing as separate buttons in the FF palette, and only the history dropdown being in the default set if I restore that, while without the patch those three are treated as one single element. Not sure if that's related with my removal of the check mentioned in comment #1 or if it's something else in your patch.

Unfortunately, we're only 29 hours from the 1.9.1 feature freeze (b1 freeze) right now, I hope you can still get this fixed, reviewed and checked in by then...

	Robert Kaiser
	Comment 25 • 16 years ago

As this is nearing completion and SeaMonkey really wants this in toolkit for the 1.9.1-based 2.0 release, trying to get this on a 1.9.1 radar. We're trying to make today's freeze, but could miss it slightly. I hope we still have a chance of getting this into 1.9.1 despite that as from what I understand, Neil thinks about forking toolbar customization if this doesn't make it, which I'd like to avoid for the sake of maintainability and extension compatibility.

	neil@parkwaycc.co.uk Assignee
	Comment 26 • 16 years ago

Based on KaiRo's comments I realised that my match function failed when the palette wasn't in the document. Hopefully this version works :-)

	Robert Kaiser
	Comment 27 • 16 years ago

Comment on attachment 341171 [details] [diff] [review]
Hopefully fixed patch

This patch works fine for me in FF in the basic tests with moving around stuff, adding toolbars, and restoring defaults.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 28 • 16 years ago

Comment on attachment 341171 [details] [diff] [review]
Hopefully fixed patch

>diff -r 432d71ba1387 toolkit/content/customizeToolbar.js

>-    var title = stringBundle.getString(aItem.id + "Title");
>+    var title = stringBundle.getString(aItem.localName.slice(7) + "Title");

Please add a comment explaining the slice, or use "toolbar".length.

>diff -r 432d71ba1387 toolkit/content/widgets/toolbar.xml

>-            while (this.lastChild) {
>-              if (this.lastChild == this.lastPermanentChild ||
>-                  (this.lastChild.localName == "toolbarpaletteitem" &&
>-                  this.lastChild.firstChild == this.lastPermanentChild))
>-                break;
>-              this.removeChild(this.lastChild);
>-            }
>+            while (this.lastChild != this.lastPermanentChild)
>+              palette.appendChild(this.lastChild);

Hmm, I hope no one else is calling this setter while the items are wrapped... It's a shame that we ever supported that.

>+              // Attempt to locate an item with a matching id within palette.
>+              newItem = document.getElementById(aId);
>+              if (!newItem || !newItem.parentNode ||

Is it really possible for getElementById to return something without a parentNode?

>+                  newItem.parentNode.parentNode != this.parentNode) {
>+                newItem = this.parentNode.palette
>+                              .getElementsByAttribute("id", aId).item(0);
>+                if (!newItem || newItem.parentNode != this.parentNode.palette)
>                   return false;

Might not hurt to explain why you need to fall back to getElementsByAttribute (palette's not be in the document by default).

I'm slightly concerned that some extension is relying on all toolbar items always being in the palette somehow, but I suppose that is pretty unlikely. The win from not keeping two elements in memory for active toolbar items and avoiding clones during startup should more than offset that risk, I think.

It would be really nice to get some tests for this code. There were some toolbar tests in bug 354048, though they mostly focused on the currentSet setter.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 29 • 16 years ago

I'm sorry and rather embarrassed that this review took so long to complete, despite Kairo's prodding. I'll try harder to not be a bottleneck in the future.

	neil@parkwaycc.co.uk Assignee
	Comment 30 • 16 years ago

Requesting approval for this potentially Ts-improving cruft-removal patch.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 31 • 16 years ago

Comment on attachment 350222 [details] [diff] [review]
Final patch for push

a191=beltzner, yay Ts wins

	neil@parkwaycc.co.uk Assignee
	Comment 32 • 16 years ago

Pushed changeset fdd5e4e34241 to mozilla-central.

	Ria Klaassen (not reading all bugmail)
	Comment 33 • 16 years ago

Don't know what exactly happened, but this bug could have been the cause that the Custom Buttons 2 extension stopped function properly. Most of the buttons disappeared from the toolbar and the customize window. They don't exist anymore in the buttonsoverlay.xul and buttonsoverlay.bak files. Not sure if this bug is really the cause but the bug appeared right after this fix for the first time.
It happens after every two or three restarts and it is a kind of little disaster if you didn't backup these files (I think most people don't backup them).

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 34 • 16 years ago

Ria, could you file a followup please, and CC me?

	alta88
	Comment 35 • 16 years ago

there's an additional problem in that if you drag a flexible space eg onto a toolbar, Done, customize and drag it off, Done, then customize again there will be a duplicate in the palette.  only for the special items it seems.

needless to say, this did a number on the totaltoolbar extension..

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 36 • 16 years ago

(In reply to comment #35)
> there's an additional problem in that if you drag a flexible space eg onto a
> toolbar, Done, customize and drag it off, Done, then customize again there will
> be a duplicate in the palette.  only for the special items it seems.

Could you please file a new bug, and CC me? That's almost always better than commenting in a closed bug :)

	Shawn Wilsher :sdwilsh
	Comment 37 • 16 years ago

I backed this out because browser/base/content/test/browser_customize.js has been failing intermittently:
http://hg.mozilla.org/mozilla-central/rev/de15a638ac3c

	Frank Wein [:mcsmurf]
	Comment 38 • 16 years ago

The error was:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/base/content/test/browser_customize.js | Timed out

	Philip Chee
	Comment 39 • 16 years ago

>> there's an additional problem in that if you drag a flexible space eg onto a
>> toolbar, Done, customize and drag it off, Done, then customize again there will
>> be a duplicate in the palette.  only for the special items it seems.

Perhaps the wrapper type gets lost somewhere or the check for wrapper type isn't working in the onDrop method.

> Could you please file a new bug, and CC me? That's almost always better than
> commenting in a closed bug :)

And make that bug block this too.

	Robert Kaiser
	Comment 40 • 16 years ago

Neil, can you get this back into mozilla-central together with the simple fix for bug 467272? I think we want this to go 1.9.1 again after some baking on trunk.

	neil@parkwaycc.co.uk Assignee
	Comment 41 • 16 years ago

Pushed changeset 2f727f142360 to mozilla-central.

This should include the fix to bug 467272.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 43 • 16 years ago

Comment on attachment 351378 [details] [diff] [review]
[checked in trunk and 1.9.1] Patch as pushed including 467272 fix

a=191 on the as-pushed patch for good measure

	Mark Banner (:standard8)
	Comment 44 • 16 years ago

Comment on attachment 351378 [details] [diff] [review]
[checked in trunk and 1.9.1] Patch as pushed including 467272 fix

I've landed this on Neil's behalf: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/48bbf4936793

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 45 • 16 years ago

CustomButtons 2 was fixed in bug 467045, and I think alta88 has the TotalToolbar bustage covered (though would appreciate confirmation - here or in the forum!), but I'm sure there are other addons that are going to be affected by this change, so it's a shame that we didn't get it in for Beta 2.

I did a naive search on the addons MXR for ".palette", and found the following list of addons that looked like they referred to the browser's palette directly:

https://addons.mozilla.org/en-US/firefox/addon/6363
https://addons.mozilla.org/en-US/firefox/addon/28
https://addons.mozilla.org/en-US/firefox/addon/5202
https://addons.mozilla.org/en-US/firefox/addon/5700
https://addons.mozilla.org/en-US/firefox/addon/1865
https://addons.mozilla.org/en-US/firefox/addon/4028
https://addons.mozilla.org/en-US/firefox/addon/6545
https://addons.mozilla.org/en-US/firefox/addon/1027
https://addons.mozilla.org/en-US/firefox/addon/3495
https://addons.mozilla.org/en-US/firefox/addon/1272
https://addons.mozilla.org/en-US/firefox/addon/5941
https://addons.mozilla.org/en-US/firefox/addon/2313
https://addons.mozilla.org/en-US/firefox/addon/5743
https://addons.mozilla.org/en-US/firefox/addon/3615
https://addons.mozilla.org/en-US/firefox/addon/1833

Is there an easy way to reach out to the authors of these addons specifically? This isn't guaranteed to affect all of them, but it would be nice to give them a heads up and offer to help with any issues they might have because of this patch.

	Christopher Blizzard (:blizzard)
	Comment 46 • 16 years ago

Rey, is this something you can do?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 47 • 16 years ago

I added a description of the changes in this bug on MDC:
https://developer.mozilla.org/En/Updating_extensions_for_Firefox_3.1#Customizable_toolbars

I can help out with any problems people might have updating their extensions.

	alta88
	Comment 48 • 16 years ago

the toolbar portion of TT is no problem, but the new design will necessitate a 'total' rethink of how customize statusbar is done (nodes there can't actually be removed so the old design worked).

but since the old design for toolbars was pretty silly i'm not complaining, and it's actually good to see effort in code renovation.

	Philip Chee
	Comment 49 • 16 years ago

https://addons.mozilla.org/en-US/firefox/addon/2313
Lightning. Who are the lightning devs?

https://addons.mozilla.org/en-US/firefox/addon/5743
xSearchbarT2 0.3 is mine. No need to cc me as I am already cc'ed on this and related bugs.

	Tony Mechelynck [:tonymec]
	Comment 50 • 16 years ago

(In reply to comment #49)
> https://addons.mozilla.org/en-US/firefox/addon/2313
> Lightning. Who are the lightning devs?

Lightning is the non-standalone form of the Calendar product. For bugmail, you might try to CC general@calendar.bugs, or for Lightning-but-not-Sunbird, lightning@calendar.bugs -- or else, you might try browsing http://www.mozilla.org/owners.html and scrolling down to Calendar, to find who are the flesh-and-blood people in charge.

	Philipp Kewisch [:Fallen] [:📆][:🧩]
	Comment 51 • 16 years ago

Thanks for the pointer, filed calendar bug 469307.

	Tony Mechelynck [:tonymec]
	Comment 52 • 16 years ago

Well, does anything need to be done for DOMi, ChatZilla, Venkman, or any other "built-in extensions" distributed together with some Mozilla standalone products?

	Philip Chee
	Comment 53 • 16 years ago

Adding Twanno (Duplicate Tab), Wladimir Palant (Adblock Plus), Mossop (Toolbar Thinger), Chris Neale (Status Buttons) to the CC list.

	Philip Chee
	Comment 54 • 16 years ago

> Well, does anything need to be done for DOMi, ChatZilla, Venkman, or any other
> "built-in extensions" distributed together with some Mozilla standalone
> products?

Highly unlikely. Gavin would have grepped MXR and found any dependencies by now.

	Robert Kaiser
	Comment 55 • 16 years ago

(In reply to comment #52)
> Well, does anything need to be done for [...]?

Nothing needs to be done unless the extension 1) uses customizable toolbars (none of the ones you cited do) and 2) are "messing" with the internals of the customizable toolbars mechanism, which most apps and extensions don't need and want to do, unless they are specifically extending/changing the mechanisms of customizations.

	Wladimir Palant
	Comment 56 • 16 years ago

Thanks for letting me know. This doesn't seem to have caused any issues in Adblock Plus. Adblock Plus needs to change icon state and does that both in the document and in the palette, without expecting the icon to exist at any of those locations. So this change only eliminated the case where the icon exists both in document and in palette. Too bad it doesn't leave <toolbarpalette> in the document, that would make the icon always accessible by ID - would make such hacks unnecessary.

	neil@parkwaycc.co.uk Assignee
	Comment 57 • 16 years ago

(In reply to comment #56)
> Too bad it doesn't leave <toolbarpalette> in
> the document, that would make the icon always accessible by ID - would make
> such hacks unnecessary.

That was my one of my ideas, but...

(In reply to comment #4)
> I'm also a bit concerned that keeping the palette in the document will have
> unintended consequences.

	Tony Mechelynck [:tonymec]
	Comment 58 • 16 years ago

(In reply to comment #55)
> (In reply to comment #52)
> > Well, does anything need to be done for [...]?
> 
> Nothing needs to be done unless the extension 1) uses customizable toolbars
> (none of the ones you cited do) and 2) are "messing" with the internals of the
> customizable toolbars mechanism, which most apps and extensions don't need and
> want to do, unless they are specifically extending/changing the mechanisms of
> customizations.

I see. Well, maybe Mel Reyes (MR-Tech Toolkit et al.) will like to be notified then.

	Philip Chee
	Comment 59 • 16 years ago

> I see. Well, maybe Mel Reyes (MR-Tech Toolkit et al.) will like to be notified
> then.

What's stopping you from adding him to the CC list then?

	Mel Reyes
	Comment 60 • 16 years ago

Outside of the standard toolbar icons, icon dropdown menus and customizing some of my toolbar's context menus I don't explicitly handle adding/removing toolbar icons so I think I'm safe, thanks.

	Tony Mechelynck [:tonymec]
	Comment 61 • 16 years ago

(In reply to comment #59)
> > I see. Well, maybe Mel Reyes (MR-Tech Toolkit et al.) will like to be notified
> > then.
> 
> What's stopping you from adding him to the CC list then?

If you check history, you'll see I did, when posting the comment you quoted above.

	Paul Rouget [:paul]
	Comment 62 • 16 years ago

I would like to add a reference to this change to MDC.
I want to be sure to have understood well how this can affect addons developers.
Please correct me if I'm wrong :

Items in browser toolbars are now moved from the toolbarpalette rather than cloned from it. That means the toolbarpalette now only hosts non used items.

	Philip Chee
	Comment 63 • 16 years ago

> Items in browser toolbars are now moved from the toolbarpalette rather than
> cloned from it. That means the toolbarpalette now only hosts non used items.

Correct.

	Stefan [:stefanh]
	Comment 64 • 16 years ago

See https://developer.mozilla.org/En/Updating_extensions_for_Firefox_3.1#Customizable_toolbars