Closed Bug 892532 Opened 11 years ago Closed 11 years ago

Add an optional fast-path to CustomizableUI.isWidgetRemovable

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Whiteboard: [Australis:M8])

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
In my local timings I'm seeing about 4-7ms during window open coming from us looking for nodes within isWidgetRemovable when the calling function already had a reference to the node.

Try pushes coming, but locally this patch shows a savings of 4-7ms (some window openings are 4ms faster, others are 7ms faster).
Attachment #774013 - Flags: review?(mnoorenberghe+bmo)
Hm, what do you think about the Talos numbers? Should we take it? It is undoubtedly doing less work, but across the average of test runs on WinXP I'm not seeing much change.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(mnoorenberghe+bmo)
Comment on attachment 774013 [details] [diff] [review]
Patch

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

Would this have a bigger impact for people with more extension toolbar buttons?

If so, I think it's still worthwhile.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1685,5 @@
>        parent.setAttribute("flex", Math.max(0, parentFlex - elementFlex));
>      }
>    },
>  
> +  isWidgetRemovable: function(aWidgetId, aOptWidgetNode) {

I would prefer a single argument that can be a Node or a string containing the id. Add a comment explaining the args.
Attachment #774013 - Flags: review?(mnoorenberghe+bmo) → review-
Flags: needinfo?(mnoorenberghe+bmo)
(In reply to Matthew N. [:MattN] from comment #3)
> Comment on attachment 774013 [details] [diff] [review]
> Patch
> 
> Review of attachment 774013 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Would this have a bigger impact for people with more extension toolbar
> buttons?
> 
> If so, I think it's still worthwhile.

Yes, when removing widgets in buildArea we are currently always re-requesting the node even though we have a reference to it. Any add-ons that are PROVIDER_XUL hit this slow path each time isWidgetRemovable gets called.

I'm going to attach two patches here that I'd like to land.

One patch is to add the removable attribute to the default items that don't have the attribute. This is where we can make our defaults fast.

The second patch is to make isWidgetRemovable faster by taking either an id or a node. This will make our API faster-by-default.
(In reply to Jared Wein [:jaws] from comment #4)
> One patch is to add the removable attribute to the default items that don't
> have the attribute. This is where we can make our defaults fast.

Please include a try push of just that patch to see if it's worthwhile. I would actually prefer it in a separate bug since it's independent.
Attached patch Patch v2 (obsolete) — Splinter Review
Updated patch to use only one argument and check the typeof the argument.
Attachment #774013 - Attachment is obsolete: true
Attachment #774385 - Flags: review?(mnoorenberghe+bmo)
Attachment #774385 - Attachment is obsolete: true
Attachment #774385 - Flags: review?(mnoorenberghe+bmo)
Attached patch Patch v2.1Splinter Review
Attachment #774389 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 774389 [details] [diff] [review]
Patch v2.1

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

r+ with a shorter doc. comment that focuses mostly on the argument types.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1688,5 @@
>  
> +  /*
> +   * isWidgetRemovable takes either a widget ID or a widget node. This
> +   * is a performance optimization since some callers already have a
> +   * reference to the widget node and retrieving it again isn't necessary.

A JavaDoc-style comment is more what I had in mind as it's easier to see the allowed argument types at quick glance. 

/**
 * @param {String|Node} aWidget - widget ID or a widget node (preferred for performance).
 * @return {Boolean} whether the widget is removable
 */

@@ +1692,5 @@
> +   * reference to the widget node and retrieving it again isn't necessary.
> +   */
> +  isWidgetRemovable: function(aWidget) {
> +    let widgetId;
> +    let widgetNode;

Nit: You could put these on the same line: let widgetId, widgetNode;
Attachment #774389 - Flags: review?(mnoorenberghe+bmo) → review+
(In reply to Matthew N. [:MattN] from comment #8)
> > +  isWidgetRemovable: function(aWidget) {
> > +    let widgetId;
> > +    let widgetNode;
> 
> Nit: You could put these on the same line: let widgetId, widgetNode;

We don't usually do that, it arguably makes code less readable and I'm not sure why you'd do it anyway; it's not going to improve performance.
I didn't put the declaration of widgetId and widgetNode on the same line because it follows the convention that is used elsewhere.

Landed with the javadoc change,
https://hg.mozilla.org/projects/ux/rev/e1fd258c689b
Whiteboard: [Australis:M8][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/e1fd258c689b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.