Open Bug 902100 Opened 8 years ago Updated 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: jaws, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P-])

Attachments

(1 file, 6 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
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".
(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?
Attached patch Patch (obsolete) — Splinter Review
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
Attachment #786546 - Attachment is obsolete: true
Attachment #786557 - Flags: review?(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.
Attachment #786557 - Flags: review?(mconley) → review+
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.
Attachment #786557 - Flags: review-
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.
(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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #786557 - Attachment is obsolete: true
Attachment #787168 - Flags: review?(dao)
Attachment #787168 - Attachment is obsolete: true
Attachment #787168 - Flags: review?(dao)
Attached patch Patch v2' (obsolete) — Splinter Review
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
Attachment #787175 - Flags: review?(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.
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.
Attached patch Patch v2.1 (obsolete) — Splinter Review
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.
Attachment #787175 - Attachment is obsolete: true
Attachment #787175 - Flags: review?(dao)
Attachment #787574 - Flags: review?(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.
No longer blocks: australis-tpaint
(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.
(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.
Attachment #787574 - Flags: review?(dao) → review?(bmcbride)
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)
Attachment #787574 - Flags: review?(bmcbride) → review-
Attached patch Patch v2.2 (obsolete) — Splinter Review
Addressed feedback.
Attachment #787574 - Attachment is obsolete: true
Attachment #800975 - Flags: review?(bmcbride)
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.
Attachment #800975 - Flags: review?(bmcbride) → review-
Oops, yeah.
Attached patch Patch v2.3Splinter Review
Attachment #800975 - Attachment is obsolete: true
Attachment #802606 - Flags: review?(bmcbride)
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.
Attachment #802606 - Flags: review?(bmcbride) → review-
Unassigning as I haven't been working on this and I don't want to hold people up.
Assignee: jaws → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.