902100 - Use the gNavToolbox directly instead of adding the extra toolbox property to build areas

Status: NEW

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

While building areas, we attach a property to the area for the toolbox, but all of the toolbox references end up pointing to the same node as gNavToolbox. This is unnecessary work and can be cleaned up.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This is the patch that I had initially wrote for this, but I need to switch it to not still set the removable attribute and check for getAttribute("removable") == "true".

Comment 2

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #0)
> While building areas, we attach a property to the area for the toolbox, but
> all of the toolbox references end up pointing to the same node as
> gNavToolbox.

Except when add-ons add another toolbox with a bunch of toolbars? Not sure whether and how that would currently work.

It also seems that hard-coding gNavToolbox here would hinder moving that stuff to toolkit.

Since this blocks bug 889758, did you measure significant performance wins?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Not sure why this was failing tests locally before, as it's passing now. Weird. Anyways, pushed it also to tryserver, https://tbpl.mozilla.org/?tree=Try&rev=d909c40958a9

Comment 4

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

Comment on attachment 786557 [details] [diff] [review]
Patch

Review of attachment 786557 [details] [diff] [review]:
-----------------------------------------------------------------

So I'm not convinced that this gains us anything in the performance department, but I think I'm OK with this change for the following reasons:

1) Seems to simplify the code somewhat
2) Unless I'm greatly mistaken, multiple toolboxes and palettes was never a thing that Australis was required to support.
3) While it's true that this hardcodes in references to browser/ stuff, even without this change, we're not in any shape to uplift to toolkit. Supporting that goal is, IMO, not a need for this v1.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 786557 [details] [diff] [review]
Patch

While it's not needed for v1, I don't see the point in deliberately making it hard to unfork this stuff and move it back to toolkit. If "seems to simplify the code somewhat" is the only upside, then that doesn't seem strong enough to me.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

CustomizableUI.jsm already has five mentions of gNavToolbox, so I'm not sure what this is regressing? We reference gNavToolbox for the "customizationstarting" and "customizationending" event, and also to get to the palette within the XULWidgetGroupWrapper constructor.

Comment 7

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #6)
> CustomizableUI.jsm already has five mentions of gNavToolbox, so I'm not sure
> what this is regressing?

It further goes down a path that seems backwards, unless we've decided that we never want to merge that stuff back to toolkit.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

This patch pulls out the gNavToolbox references to a helper object that can be injected in to the CustomizableUIInternal so when it eventually gets pulled out to toolkit, other consumers can provide their own method for retrieving the toolbox for a node or window.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2'

Couldn't inject the CustomizeHelper object because CustomizableUIInternal is frozen.

0:46.36 INFO TEST-START | Shutdown
 0:46.36 Browser Chrome Test Summary
 0:46.36        Passed: 354
 0:46.36        Failed: 0
 0:46.36        Todo: 1

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 787175 [details] [diff] [review]
Patch v2'

>+let CustomizeHelper = {
>+  getNavToolboxFromWindow: function(aWindow) {
>+    return aWindow.gNavToolbox;
>+  },
>+
>+  getNavToolboxFromNode: function(aNode) {
>+    return this.getNavToolboxFromWindow(aNode.ownerDocument.defaultView);
>+  }
>+}

These names should probably just say "get toolbox" rather than "get nav toolbox", since "nav toolbox" isn't a generic term. As already noted, a window could have multiple toolboxes, e.g. one at the top, one at the bottom or on a side. In toolkit, the toolbox is passed as an argument to the customization code, so customizing is always toolbox-specific. Is there a reason why this can't work the same way? E.g. can the code using CustomizableUI.jsm do 'new CustomizableUI(toolbox)' or something?

As a side note, I'm somewhat confused by CustomizableUI.jsm containing very generic code next to browser.xul-specific code, e.g. all the registerArea stuff in CustomizableUIInternal.initialize. It seems to me that good API design would mean to separate that stuff and make calling registerArea an external responsibility.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

I borrowed the usage of "getNavToolbox" from http://dxr.mozilla.org/mozilla-central/source/toolkit/components/printing/content/printUtils.js#l235 in which PrintPreviewListener defines a getNavToolbox function, http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#l2700, but I can change this to "get toolbox" with no problem.

We could do "new CustomizableUI(toolbox)" but then we wouldn't be able to freeze these objects.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.1

It's not so easy to just pass in a toolbox because the toolbox is associated with a window and that window may close, while the JSM will stick around. Also, I can't use a getter because I need the reference node or window to retrieve the toolbox from. I did remove the 'Nav' portion of the function name, but I don't know any way to change it further.

The other API issues should be filed in a separate bug.

Comment 13

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #12)
> Created attachment 787574 [details] [diff] [review]
> Patch v2.1
> 
> It's not so easy to just pass in a toolbox because the toolbox is associated
> with a window and that window may close, while the JSM will stick around.

But any code requiring access to a toolbox can't run without a toolbox being available. So it still seems to me that the JSM should provide a prototype object where each instance would be associated with a toolbox, and maybe a second singleton object with things that don't need an actual window / toolbox. I must confess, though, this is all pretty shaky in my head, as I've barely grokked the module's current design and structure.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dão Gottwald [:dao] from comment #10)
> As already noted, a
> window could have multiple toolboxes,

The toolkit code allows this, yes. And we don't use it anywhere. In fact, I don't even know of any third party apps that use it either. For something that adds complexity yet isn't used, it's a pretty good candidate for being removed.


> E.g. can the code using
> CustomizableUI.jsm do 'new CustomizableUI(toolbox)' or something?

If we really did need to support multiple toolboxes, this would be my preferred method of doing it.

> e.g. all the registerArea
> stuff in CustomizableUIInternal.initialize. It seems to me that good API
> design would mean to separate that stuff and make calling registerArea an
> external responsibility.

Yes, we'll want to do that eventually. But for now, while it's only used in Firefox, it's complexity we don't need.

Comment 15

[Dão Gottwald [:dao]]

(In reply to Blair McBride [:Unfocused] from comment #14)
> > e.g. all the registerArea
> > stuff in CustomizableUIInternal.initialize. It seems to me that good API
> > design would mean to separate that stuff and make calling registerArea an
> > external responsibility.
> 
> Yes, we'll want to do that eventually. But for now, while it's only used in
> Firefox, it's complexity we don't need.

Good abstraction and separation of different code parts can actually make things easier to comprehend. I really find the current setup pretty messy and at times confusing.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 787574 [details] [diff] [review]
Patch v2.1

Review of attachment 787574 [details] [diff] [review]:
-----------------------------------------------------------------

Should also update the new toolbar binding in toolbar.xml, which implements a complex toolbox property.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +112,5 @@
>  let gModuleName = "[CustomizableUI]";
>  #include logging.js
>  
> +let CustomizeHelper = {
> +  getToolboxFromWindow: function(aWindow) {

Nit: getToolboxForWindow

@@ +117,5 @@
> +    return aWindow.gNavToolbox;
> +  },
> +
> +  getToolboxFromNode: function(aNode) {
> +    return this.getToolboxFromWindow(aNode.ownerDocument.defaultView);

So... if we drop support for multiple toolboxes, this method will always be the same, so should just be a shim on CustomizableUI (or just always use getToolboxForWindow directly). If we decide we need to support multiple toolboxes, getToolboxForWindow would need to not exist.

At the moment, and with this patch, we're in a grey area of half-supporting both - needing APIs for both, but in practice only having one toolbox will work. Based on comment 14, I'd vote to just drop support for multiple toolboxes.

@@ +119,5 @@
> +
> +  getToolboxFromNode: function(aNode) {
> +    return this.getToolboxFromWindow(aNode.ownerDocument.defaultView);
> +  }
> +}

Nit: Missing semicolon.

@@ +2122,5 @@
>      this._list = doc.getElementById(this._toolbar.getAttribute("overflowtarget"));
>  
>      let window = doc.defaultView;
>      window.addEventListener("resize", this);
> +    let navToolbox = CustomizeHelper.getToolboxFromWindow(window);

Nit: s/navToolbox/toolbox/
(And other places)

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.2

Addressed feedback.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 800975 [details] [diff] [review]
Patch v2.2

Review of attachment 800975 [details] [diff] [review]:
-----------------------------------------------------------------

Missed this?

(In reply to Blair McBride [:Unfocused] from comment #16)
> Should also update the new toolbar binding in toolbar.xml, which implements
> a complex toolbox property.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

Oops, yeah.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2.3

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 802606 [details] [diff] [review]
Patch v2.3

Review of attachment 802606 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/customizableui/content/toolbar.xml
@@ +119,5 @@
>        </property>
>  
>        <property name="toolbox" readonly="true">
>          <getter><![CDATA[
> +          return this._toolbox || window.gNavToolbox;

this._toolbox only exists if this method sets it, so it can be killed.

Also, this is part of the new customization system, so should ideally go through CustomizeHelper. Doesn't make sense to export that IMO (same rationale as the rest of the patch - it's app-specific), so lets have a wrapper on the public API.

Also also, I notice a few other bindings in this file implement their own "toolbox" property, which should also be updated to avoid unneeded work.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Unassigning as I haven't been working on this and I don't want to hold people up.