[de-xbl] get rid of the toolbox binding (remove support for adding custom custom toolbars)
Categories
(Thunderbird :: Toolbars and Tabs, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: khushil324)
References
Details
Attachments
(1 file, 5 obsolete files)
|
(deleted),
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
Bug 1429861 moved this from toolkit to common.
The binding is apparently only used for adding new toolbars. Going forward I don't think we should support adding toolbars like this, so this is code that can be removed. It's most likely a rather unused feature.
|
Reporter | |
Comment 2•5 years ago
|
||
Comment on attachment 9068055 [details] [diff] [review] Bug-1546315_remove_toolbox-binding.patch Review of attachment 9068055 [details] [diff] [review]: ----------------------------------------------------------------- Did you check what happens if someone did have an additional toolbar, and that want's to be restored (on restart?). Maybe we need something like https://searchfox.org/comm-central/rev/2ddfe0d5bd910e7e7bdbdebb5b826b4863dfd880/mozilla/browser/components/BrowserGlue.jsm#2473 Also, you need to make sure all the places this is used are taken care of. This finds a bunch of them which would probably be broken. toolbox has no features at all in mozilla-central (since bug 1429464) grep -r --exclude-dir=.hg --exclude-dir=suite '\.palette' grep -r --exclude-dir=.hg --exclude-dir=suite '\.toolbarset' ::: mail/base/content/mailCore.js @@ +318,5 @@ > toolbox.querySelectorAll("[toolbarname]") > ); > + > + // Add the folder pane toolbar to the list of toolbars that can be shown and hidden. > + if(toolbox.getAttribute("id") === "mail-toolbox") { nit: if ( did you check the linter?
|
Assignee | |
Comment 3•5 years ago
|
||
|
|
Assignee | |
Updated•5 years ago
|
|
|
Reporter | |
Comment 4•5 years ago
|
||
Comment on attachment 9070214 [details] [diff] [review] Bug-1546315_remove_toolbox-binding.patch Review of attachment 9070214 [details] [diff] [review]: ----------------------------------------------------------------- I think you didn't get them all grep -r --exclude-dir=.hg --exclude-dir=suite '\.palette' Most if not all of that should probably be gone, no?
|
|
Assignee | |
Comment 5•5 years ago
|
||
toolbarpalette is used to add/remove the toolbarbuttons/items from customize toolbar dialogue. We have removed support to add new toolbar so we will not be able to add toolbarbuttons to the newly created toolbar so I don't see any need to work around palette. There was binding of toolbox extending m-c toolbox. The main reason was to have a feature of adding new toolbars, keeping the default toolbars there. And with this patch, we are removing the c-c toolbox and feature of adding new toolbar. We are not doing anything with palette feature in it.
|
|
Reporter | |
Updated•5 years ago
|
|
|
Reporter | |
Comment 6•5 years ago
|
||
Comment on attachment 9070214 [details] [diff] [review] Bug-1546315_remove_toolbox-binding.patch Review of attachment 9070214 [details] [diff] [review]: ----------------------------------------------------------------- Looks alright. This feature apparently didn't work, at least going back as far as 60. I'll attach an unbitrotted patch.
|
|
Reporter | |
Comment 7•5 years ago
|
||
|
|
Assignee | |
Comment 8•5 years ago
|
||
There are some changes in customizeToolbar.js for customization on tabtoolbar.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f6b2aa2b6ee665493de0c15dde9edbd9a063cfcc
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 9•5 years ago
|
||
|
|
Reporter | |
Comment 10•5 years ago
|
||
I think you meant to link https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0067183e6463d741f4a2878cbc9e6e587372ac44
|
|
Reporter | |
Comment 11•5 years ago
|
||
Comment on attachment 9078177 [details] [diff] [review] Bug-1546315_remove-toolbox-binding.patch Review of attachment 9078177 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, r=mkmelin ::: common/src/customizeToolbar.js @@ +187,5 @@ > function getRootElements() { > + if (window.frameElement && "externalToolbars" in window.frameElement) { > + return [gToolbox].concat(window.frameElement.externalToolbars); > + } else if ("arguments" in window && window.arguments[1].length > 0) { > + return [gToolbox].concat(window.arguments[1]); no else after return please
|
|
Assignee | |
Comment 12•5 years ago
|
||
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 13•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4498629b0c98
[de-xbl] remove the toolbox binding. r=mkmelin
|
||
Updated•5 years ago
|
Description
•