1474340 - Remove toolbar.toolbox XBL property and update its consumers

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, task, P3)

Description

[:Gijs (he/him)]

With the goal of eventually removing the toolbar.xml toolbar binding from CustomizableUI, we're slowly removing parts of the binding.

In this bug, we should:

- remove https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/customizableui/content/toolbar.xml#92-112 
- remove https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/customizableui/content/toolbar.xml#179-190
- replace the code at https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/customizableui/content/toolbar.xml#22 with:

let toolbox = this.closest("toolbox");

- remove https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#1006


- go through browser/components/customizableui/CustomizableUI.jsm, and wherever we use the `.toolbox` property on an area node, instead reuse a local variable or argument pointing to `window`, or use node.ownerGlobal, to get a reference to `window.gNavToolbox` for the window in which the areaNode is present.

I *think* that should be enough. When fixing this bug, we should run the tests from:

./mach mochitest browser/components/customizableui/

(after rebuilding) to make sure we don't break anything.

Comment 1

[Manish [:manishkk]]

What exactly I should write?

Can you tell about this case?
https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#856

> - go through browser/components/customizableui/CustomizableUI.jsm, and
> wherever we use the `.toolbox` property on an area node, instead reuse a
> local variable or argument pointing to `window`, or use node.ownerGlobal, to
> get a reference to `window.gNavToolbox` for the window in which the areaNode
> is present.

Comment 2

[:Gijs (he/him)]

(In reply to Manish Kumar [:manishkk] from comment #1)
> What exactly I should write?
> 
> Can you tell about this case?
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> customizableui/CustomizableUI.jsm#856
> 
> > - go through browser/components/customizableui/CustomizableUI.jsm, and
> > wherever we use the `.toolbox` property on an area node, instead reuse a
> > local variable or argument pointing to `window`, or use node.ownerGlobal, to
> > get a reference to `window.gNavToolbox` for the window in which the areaNode
> > is present.

So in this particular case, the existing code does:

        let palette = aAreaNode.toolbox ? aAreaNode.toolbox.palette : null;

so you could do:

        let toolbox = aAreaNode.ownerGlobal.gNavToolbox || null;
        let palette = toolbox ? toolbox.palette : null;

In e.g. https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/customizableui/CustomizableUI.jsm#1077 there is already a local variable `window`, so you can just replace:

        areaNode.toolbox.palette.appendChild(widgetNode);

with

        window.gNavToolbox.palette.appendChild(widgetNode);


And so on along the rest of the file. This is because `window` and `areaNode.ownerGlobal` point to the same object, and `window.gNavToolbox` will always point to the same thing that `areaNode.toolbox` points to right now. Does that make sense?

Comment 3

[:Gijs (he/him)]

Hey Manish, do you need more help with this? Let me know on bugzilla if something's not clear. Thanks for working on this!

Comment 4

[akshitsoni0000]

hey, Can i work on this? I am beginner.

Comment 5

[:Gijs (he/him)]

(In reply to akshitsoni0000 from comment #4)
> hey, Can i work on this? I am beginner.

I think as your first bug this is probably a bit tricky. Try looking for bugs that are marked 'good-first-bug' instead. I saw your comment on bug 1423843 - that might be an easier bug to get started on.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

This is blocking my work in bug 1356920, so I'll do this.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1474340 - Remove toolbox XBL property from toolbar. r?Gijs

Comment 8

[:Gijs (he/him)]

Comment on attachment 9009744 [details]
Bug 1474340 - Remove toolbox XBL property from toolbar. r?Gijs

:Gijs (he/him) has approved the revision.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12dcd7c35b36
Remove toolbox XBL property from toolbar. r=Gijs

Comment 10

[Andrei Ciure[:aciure]]

Backed out changeset 12dcd7c35b36 (bug 1474340) for ESlint failure

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=199949216&revision=12dcd7c35b36a48d8a1e101255df15b9112157eb

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=b26a70a0fe8f22ee4a5118c7c563ab98a115e692&selectedJob=199946963&searchStr=linting%2Copt%2Csource-test-mozlint-eslint%2C%28es%29

backout: https://hg.mozilla.org/integration/autoland/rev/941266ad9f8f4093f5de294955ffabe67fcb1e08

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1c0d8cbad37
Remove toolbox XBL property from toolbar. r=Gijs

Comment 12

[Arthur Iakab [arthur_iakab]]

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