892809 - Set removable attribute on primary toolbar buttons.

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

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

Attachment: Patch

We default items without the removable attribute to 'false', but that adds extra work that isn't necessary when we know before compile time that these should have the attribute set on them. The extra work to add the attribute if it's missing should just be reserved for add-ons and other code that is not under our control.

Baseline push to try:
https://tbpl.mozilla.org/?tree=Try&rev=d0e5e0d84f47

Patch pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=f487dfdd7412

Comment 1

[Dão Gottwald [:dao]]

Why does the absence of this attribute imply extra work? What kind of work?

Comment 2

[Matthew N. [:MattN]]

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

Technically the patch is fine but can we wait to land this until we see if we can find tpaint wins first. This did somewhat hinder tpaint by a few milliseconds on XP[1] and makes the markup a bit verbose. If we can find tpaint wins, those will help ts_paint as well and this won't really be necessary.

[1] http://perf.snarkfest.net/compare-talos/index.html?oldRevs=d0e5e0d84f47&newRev=f487dfdd7412&submit=true

(In reply to Dão Gottwald [:dao] from comment #1)
> Why does the absence of this attribute imply extra work? What kind of work?

We end of setting the attribute at runtime anyways. This patch avoid a setAttribute on each one of these during startup. Look for |.hasAttribute("removable")| in https://hg.mozilla.org/projects/ux/file/tip/browser/components/customizableui/src/CustomizableUI.jsm#l297 and you will see the setAttribute calls below.

Comment 3

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #2)
> (In reply to Dão Gottwald [:dao] from comment #1)
> > Why does the absence of this attribute imply extra work? What kind of work?
> 
> We end of setting the attribute at runtime anyways.

It's not clear to me we're doing this. The old code didn't do it. Can the code actually reading that attribute just treat the absence as false?

> This patch avoid a
> setAttribute on each one of these during startup. Look for
> |.hasAttribute("removable")| in
> https://hg.mozilla.org/projects/ux/file/tip/browser/components/
> customizableui/src/CustomizableUI.jsm#l297 and you will see the setAttribute
> calls below.

setAttribute is super-cheap since it has no layout impact. However, you're calling isWidgetRemovable which seems pretty involved at a first glance, probably more than it needs to be for the simple case of initializing toolbars...

Comment 4

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

(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to Matthew N. [:MattN] from comment #2)
> > (In reply to Dão Gottwald [:dao] from comment #1)
> > > Why does the absence of this attribute imply extra work? What kind of work?
> > 
> > We end of setting the attribute at runtime anyways.
> 
> It's not clear to me we're doing this. The old code didn't do it. Can the
> code actually reading that attribute just treat the absence as false?

It eventually does treat the absence of the attribute as false, but as you've said isWidgetRemovable is a bit involved, so setting the attribute in browser.xul skips this code path.

Comment 5

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

However, most of the overhead of isWidgetRemovable is sidestepped by the patch for bug 892532. This patch in this bug just makes it so that isWidgetRemovable doesn't need to be entered.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #3)
> > (In reply to Matthew N. [:MattN] from comment #2)
> > > (In reply to Dão Gottwald [:dao] from comment #1)
> > > > Why does the absence of this attribute imply extra work? What kind of work?
> > > 
> > > We end of setting the attribute at runtime anyways.
> > 
> > It's not clear to me we're doing this. The old code didn't do it. Can the
> > code actually reading that attribute just treat the absence as false?
> 
> It eventually does treat the absence of the attribute as false, but as
> you've said isWidgetRemovable is a bit involved,

Ok, so if the attribute isn't set, isWidgetRemovable will be called later when customizing? That seems like a fine tradeoff. Why should we bother with setting this attribute upon constructing the toolbar?

Comment 7

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

(In reply to Dão Gottwald [:dao] from comment #6)
> (In reply to Jared Wein [:jaws] from comment #4)
> > (In reply to Dão Gottwald [:dao] from comment #3)
> > > (In reply to Matthew N. [:MattN] from comment #2)
> > > > (In reply to Dão Gottwald [:dao] from comment #1)
> > > > > Why does the absence of this attribute imply extra work? What kind of work?
> > > > 
> > > > We end of setting the attribute at runtime anyways.
> > > 
> > > It's not clear to me we're doing this. The old code didn't do it. Can the
> > > code actually reading that attribute just treat the absence as false?
> > 
> > It eventually does treat the absence of the attribute as false, but as
> > you've said isWidgetRemovable is a bit involved,
> 
> Ok, so if the attribute isn't set, isWidgetRemovable will be called later
> when customizing? That seems like a fine tradeoff. Why should we bother with
> setting this attribute upon constructing the toolbar?

The way that it currently works is that during construction of the toolbar we set attributes on all of the widgets to state whether they are removable or not. If the attribute is already specified then we skip that step.

However, it could be just better to move this step to when we actually enter customization. I don't know what issues that might introduce though (for example, if add-ons are looking to see what is removable etc).

Comment 8

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] from comment #7)
> However, it could be just better to move this step to when we actually enter
> customization. I don't know what issues that might introduce though (for
> example, if add-ons are looking to see what is removable etc).

We've never set the attribute on all elements, so this shouldn't surprise add-ons at all.

Comment 9

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

(In reply to Jared Wein [:jaws] (Vacation 8/1 to 8/4) from comment #5)
> However, most of the overhead of isWidgetRemovable is sidestepped by the
> patch for bug 892532. This patch in this bug just makes it so that
> isWidgetRemovable doesn't need to be entered.

Closing this as it's not necessary anymore since bug 892532 has been fixed.