902164 - Don't register toolbars on startup if they're collapsed

Status: RESOLVED WONTFIX

(Firefox :: Toolbars and Customization, defect)

Description

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

Not sure if this will move the needle at all, but for the default profile, the PersonalToolbar is collapsed, and doesn't need to register itself until it's first shown. We might be able to do the same thing for the toolbar-menubar.

Comment 1

[:Gijs (out for now; he/him)]

(In reply to Mike Conley (:mconley) from comment #1)
> Not sure if this will move the needle at all, but for the default profile,
> the PersonalToolbar is collapsed, and doesn't need to register itself until
> it's first shown. We might be able to do the same thing for the
> toolbar-menubar.

So unfortunately I don't think this is true. We do need to register otherwise any code trying to add things to the area will error out with "this area doesn't exist".

(alternatively, we could make that stuff work... but that might be complicated? I've been considering that this behaviour makes it hard to have add-ons that depend on other add-ons adding buildAreas, but that's not a super-likely usecase generally speaking... but might bite us if we get add-ons adding back the add-on bar or somesuch)


Anyway, we can register the toolbar but not run buildArea if it's collapsed, and instead wait for it to, err, uncollapse... before we do that?

Comment 2

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

What if we when we find an area doesn't exist, we then initialize the collapsed toolbars and try again?

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] from comment #2)
> What if we when we find an area doesn't exist, we then initialize the
> collapsed toolbars and try again?

On second thought, I think I was wrong. I think CustomizableUI calls registerArea, and that might be enough to init with placements and whatnot, and .addWidgetToArea will then work, etc. etc., and so not calling registerToolbar should be OK - it just won't register that build area until it's needed, which should be fine. So ignore comment #1, I guess?

Comment 4

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch (PTO Aug 8-Aug 18) from comment #3)
> (In reply to Jared Wein [:jaws] from comment #2)
> > What if we when we find an area doesn't exist, we then initialize the
> > collapsed toolbars and try again?
> 
> On second thought, I think I was wrong. I think CustomizableUI calls
> registerArea, and that might be enough to init with placements and whatnot,
> and .addWidgetToArea will then work, etc. etc., and so not calling
> registerToolbar should be OK - it just won't register that build area until
> it's needed, which should be fine. So ignore comment #1, I guess?

I'm trying this and it's hanging after the first of the customizableUI browser mochitests. Not sure why yet (no JS errors in the console). :-\

Comment 5

[:Gijs (out for now; he/him)]

Attachment: delay toolbar registration until the toolbar is actually visible.

Alright, bit more involved than I thought. This is what I came up with. FWIW, I think the modifications outside the toolbar should probably be made anyway, as we should deal with areas not having any build nodes. Try push: https://tbpl.mozilla.org/?tree=Try&rev=267891ff8c6c (you can use https://tbpl.mozilla.org/?tree=Try&rev=6e19ce6097a6 as a baseline).

Comment 6

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

Hrm. I'm not yet convinced this gives us any wins, based on http://compare-talos.mattn.ca/?oldRevs=6e19ce6097a6&newRev=267891ff8c6c&submit=true

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 786878 [details] [diff] [review]
delay toolbar registration until the toolbar is actually visible.

>+            // We can delay registration until we're visible
>+            let obs = new MutationObserver(this._onAttributeChange);
>+            obs.observe(this, {attributes: true});

Does this observer automatically go away in case the binding gets destructed?

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 786878 [details] [diff] [review]
> delay toolbar registration until the toolbar is actually visible.
> 
> >+            // We can delay registration until we're visible
> >+            let obs = new MutationObserver(this._onAttributeChange);
> >+            obs.observe(this, {attributes: true});
> 
> Does this observer automatically go away in case the binding gets destructed?

It goes away when the node is destroyed or when it is disconnected, because there are no closures in the constructor*, so after it has finished running the only reference we have is it observing the node. When the node is destroyed, the observer is, too.


* this leaked when the function was inline rather than a XBL method. :-)

Comment 9

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch (PTO Aug 8-Aug 18) from comment #8)
> > Does this observer automatically go away in case the binding gets destructed?
> 
> It goes away when the node is destroyed or when it is disconnected,

Unless I'm misunderstanding you, this seems insufficient, since the binding can change while the node stays around.

Comment 10

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

Comment on attachment 786878 [details] [diff] [review]
delay toolbar registration until the toolbar is actually visible.

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

I think I'd prefer if we make getCustomizeTargetForArea not error fatally, so as to avoid needing to try/catch everything sometimes.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +492,5 @@
>      // Add drag-and-drop event handlers to all of the customizable areas.
>      return Task.spawn(function() {
>        this.areas = [];
>        for (let area of CustomizableUI.areas) {
> +        let target;

Ooof - this makes me uneasy.

Perhaps we should make getCustomizeTargetForArea not error fatally, but just to return null and log an ERROR message - that way, we can avoid needing to catch-all exceptions.

@@ +520,5 @@
>      let window = this.window;
>      // Add drag-and-drop event handlers to all of the customizable areas.
>      this.areas = [];
>      for (let area of CustomizableUI.areas) {
> +      let target;

Same as above.

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #8)
> > > Does this observer automatically go away in case the binding gets destructed?
> > 
> > It goes away when the node is destroyed or when it is disconnected,
> 
> Unless I'm misunderstanding you, this seems insufficient, since the binding
> can change while the node stays around.

How would this happen in this case, and is this likely to happen to the add-on bar, too? Because XBL destructors are unreliable, unless I am myself misunderstanding you, that would mean we can't use mutation observers from XBL bindings, as even if we manually kept a reference and disconnected + destroyed it, we don't know when to do that.

Comment 12

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > (In reply to :Gijs Kruitbosch from comment #8)
> > > > Does this observer automatically go away in case the binding gets destructed?
> > > 
> > > It goes away when the node is destroyed or when it is disconnected,
> > 
> > Unless I'm misunderstanding you, this seems insufficient, since the binding
> > can change while the node stays around.
> 
> How would this happen in this case,

You'd just switch bindings, e.g. from #toolbar to #toolbar-drag or some other custom binding.

> and is this likely to happen to the add-on bar, too?

If you mean the add-on bar that's never displayed and just exists for migrating items, then probably no... unless there's a reason for add-ons to mess around with it.

Comment 13

[:Gijs (out for now; he/him)]

This doesn't seem to get us much/anything, and the patch seems to have issues, too, so let's just WONTFIX this.

(however, needinfo'ing mconley - do we need to followup and make getCustomizeTargetForArea be less throw-y? IIRC it breaking customize mode is going to be an issue for add-ons etc. that might add areas not-immediately-at-startup or something)

Comment 14

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

(In reply to :Gijs Kruitbosch from comment #13)
> This doesn't seem to get us much/anything, and the patch seems to have
> issues, too, so let's just WONTFIX this.
> 
> (however, needinfo'ing mconley - do we need to followup and make
> getCustomizeTargetForArea be less throw-y? IIRC it breaking customize mode
> is going to be an issue for add-ons etc. that might add areas
> not-immediately-at-startup or something)

getCustomizeTargetForArea is no longer throw-y, it seems. Sorry this took so long to get to (wasn't showing up in my dashboard for some reason...I think it's because the bug was marked RESOLVED).