877856 - Remove obsolete defaultset attributes once Australis lands

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

Per bug 877524 comment #2, we should probably get rid of defaultset attributes which will no longer be used.

Comment 1

[:Gijs (he/him)]

Ironically, we now use this for compatibility for addon toolbars. I guess we can remove them from the default toolbars but we can't remove the support for add-ons for some time, I'd expect.

Comment 2

[:Gijs (he/him)]

This will need to:
- remove all the defaultset attributes from the builtin toolbars (#nav-bar, #TabsToolbar, #toolbar-menubar, #addon-bar, #PersonalToolbar)
- add a one-time migrateUI migration that removes all of those attributes from the persist store in case they're in there.
- update CustomizableUI's code to never read the attribute for the builtin toolbars

Comment 3

[Akshat Kedia [:aks]]

Hi Gijs,

I am taking this up. I will first tackle the removal of all the defaultset attributes. Where do I need to look at to get started here ?

Comment 4

[:Gijs (he/him)]

(In reply to Akshat Kedia [:aks] from comment #3)
> Hi Gijs,
> 
> I am taking this up. I will first tackle the removal of all the defaultset
> attributes. Where do I need to look at to get started here ?

In browser.xul, there's a number of <toolbar> elements that have a defaultset attributes. All of those should be removed (and in one case, both 2 versions separated by ifdefs, including the ifdefs themselves, should be removed).

(In reply to :Gijs Kruitbosch from comment #2)
> - add a one-time migrateUI migration that removes all of those attributes
> from the persist store in case they're in there.

More precisely, in this method: http://searchfox.org/mozilla-central/rev/3f096f780166bc4772199d79a79c8e3541ff99be/browser/components/nsBrowserGlue.js#1898

- increment the const (at the time of writing it's 38, so it'd become 39), let's say the new value is N
- add an if block for (currentUIVersion < N) below the last similar if block there, and inside the if block write code like in this if block: http://searchfox.org/mozilla-central/rev/3f096f780166bc4772199d79a79c8e3541ff99be/browser/components/nsBrowserGlue.js#2067-2074 that removes the 'defaultset' value for all the toolbars (so don't include navigator-toolbox), but include everything else

> - update CustomizableUI's code to never read the attribute for the builtin
> toolbars

I actually think this might not need to happen, and it should work as-is. To make sure this is the case, verify (probably easiest to use logging through Components.utils.reportError) that the code in this if block: http://searchfox.org/mozilla-central/rev/3f096f780166bc4772199d79a79c8e3541ff99be/browser/components/customizableui/CustomizableUI.jsm#620-638 doesn't get executed for the builtin toolbars (ie the ones where you're removing the defaultset attribute browser.xul).

Comment 5

[Akshat Kedia [:aks]]

Attachment: Bug 877856 - Remove obsolete defaultset attributes once Australis lands. Kruitbosch

Review commit: https://reviewboard.mozilla.org/r/59080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59080/

Comment 6

[:Gijs (he/him)]

Comment on attachment 8762280 [details]
Bug 877856 - Remove obsolete defaultset attributes once Australis lands.  Kruitbosch

https://reviewboard.mozilla.org/r/59080/#review56130

This looks good, thanks! I assume you verified manually that the builtin toolbars hit the right codepath in the code in comment #4 ?

Comment 7

[Akshat Kedia [:aks]]

I did Components.utils.reportError(defaultsetAttribute); in here http://searchfox.org/mozilla-central/rev/3f096f780166bc4772199d79a79c8e3541ff99be/browser/components/customizableui/CustomizableUI.jsm#620-638 but the jsconsole did not output anything. I assume that this verifies as expected.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/ea5833a015a1
Remove obsolete defaultset attributes from default toolbars, r=gijs

Comment 9

[:Gijs (he/him)]

(In reply to Akshat Kedia [:aks] from comment #7)
> I did Components.utils.reportError(defaultsetAttribute); in here
> http://searchfox.org/mozilla-central/rev/
> 3f096f780166bc4772199d79a79c8e3541ff99be/browser/components/customizableui/
> CustomizableUI.jsm#620-638 but the jsconsole did not output anything. I
> assume that this verifies as expected.

Yup, I doublechecked as well, this looks excellent. Landed on fx-team. Thanks!

Comment 10

[Akshat Kedia [:aks]]

Great! :)

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea5833a015a1