909366 - Australis: Switching LWT re-constructs XBL bindings, leading to all kinds of sadness

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

STR:

1. Switch to an(other) LWT.

ER:
Nothing in customizable UI really does/changes anything

AR:
The XBL of the navbar (and maybe other toolbars) gets reconstructed. This means it calls registerToolbar again. This means it registers itself another time (creating another instance of OverflowableToolbar) and re-runs buildArea, which creates problems with the downloads-indicator, etc. etc. Sadness all around. Let's not.

Comment 1

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

This sounds bad. When registerToolbar is called, can't we short circuit if it's already been registered?

Comment 2

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

(In reply to Jared Wein [:jaws] from comment #1)
> This sounds bad. When registerToolbar is called, can't we short circuit if
> it's already been registered?

Yes. But really, the whole issue exposes 3 linked issues:

- (I don't understand why) we have separate functional toolbar bindings depending on the lwtheme: https://mxr.mozilla.org/projects-central/source/ux/browser/themes/windows/browser-aero.css#188 (https://bugzilla.mozilla.org/show_bug.cgi?id=803546#c12)
- We could potentially toggle draggability of toolbars in the same binding, using an attribute or property or something, which makes more sense to me than having separate bindings for it (although that's partially because XBL's inheritance model isn't great)
- And yes, we should make sure that when the constructor is repeatedly called, the 2nd to nth calls are no-op. This is probably the simplest solution, but at the very least I'd like to understand the first thing I mentioned - why does this change with LTWs on aero?

Comment 3

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

Attachment: add re-entrancy check to registerToolbar,

Let's at least do this to solve this short-term, it doesn't stop us from investigating the first two issues.

Comment 4

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

https://hg.mozilla.org/projects/ux/rev/4154345daff7

Comment 5

[Florian Bender]

Shouldn't this be [leave open]? Or do you want to file follow ups?

Comment 6

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

(In reply to Florian Bender from comment #5)
> Shouldn't this be [leave open]? Or do you want to file follow ups?

The latter. Deep in some other stuff right now, setting needinfo so I don't forget.

Comment 7

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

Filed bug 925349.

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/4154345daff7