Remove toolbar.toolbox XBL property and update its consumers

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
6 months ago
4 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

6 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.
Blocks: 1397874

Updated

6 months ago
Assignee: nobody → 1991manish.kumar

Comment 1

6 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

6 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

6 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

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

Comment 4

4 months ago
hey, Can i work on this? I am beginner.
(Reporter)

Comment 5

4 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

4 months ago
Blocks: 1356920
(Assignee)

Comment 6

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

Comment 7

4 months ago
Created attachment 9009744 [details]
Bug 1474340 - Remove toolbox XBL property from toolbar. r?Gijs
(Reporter)

Comment 8

4 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

4 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

4 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

4 months ago
Flags: needinfo?(mconley)

Comment 12

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