Remove toolbar.toolbox XBL property and update its consumers

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
11 months ago
8 months ago

People

(Reporter: Gijs, Assigned: mconley)

Tracking

(Blocks 1 bug)

Trunk
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 wontfix, firefox64 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment)

Reporter

Description

11 months ago
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.

Updated

11 months ago
Assignee: nobody → 1991manish.kumar

Comment 1

11 months ago
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.
Flags: needinfo?(gijskruitbosch+bugs)
Reporter

Comment 2

11 months ago
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(1991manish.kumar)
Reporter

Comment 3

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

Updated

9 months ago
Assignee: 1991manish.kumar → nobody
Flags: needinfo?(1991manish.kumar)

Comment 4

9 months ago
hey, Can i work on this? I am beginner.
Reporter

Comment 5

9 months ago
(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.
Assignee

Updated

8 months ago
Blocks: 1356920
Assignee

Comment 6

8 months ago
This is blocking my work in bug 1356920, so I'll do this.
Assignee: nobody → mconley
Mentor: gijskruitbosch+bugs
Reporter

Comment 8

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

:Gijs (he/him) has approved the revision.
Attachment #9009744 - Flags: review+

Comment 9

8 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12dcd7c35b36
Remove toolbox XBL property from toolbar. r=Gijs

Comment 11

8 months ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e1c0d8cbad37
Remove toolbox XBL property from toolbar. r=Gijs
Assignee

Updated

8 months ago
Flags: needinfo?(mconley)

Comment 12

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e1c0d8cbad37
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.