898040 - Delay adding the event listeners for PanelUI until the panel menu is toggled

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

We shouldn't need to run PanelUI.init until the panel is activated.

Comment 1

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

We still need the lazy getters, but the event listeners aren't needed and it looks like the retrieval of the panel element is causing the panel constructor to be run.

Comment 2

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

Attachment: 898040.patch

I confirmed locally that the PanelUI.xml binding constructor isn't being run with this patch and that the panel opens and functions correctly.

Comment 3

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

Comment on attachment 781091 [details] [diff] [review]
898040.patch

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

It's a little ugly that we have to run _ensureEventListenersAdded at the start of each of those functions, but I'll take performant code over pretty code at this point.

::: browser/components/customizableui/content/panelUI.js
@@ +25,5 @@
>        panel: "PanelUI-popup"
>      };
>    },
>  
> +  _ensureInitialized: function() {

I guess we should add a 

_initialized: false,

 somewhere here as well.

@@ +65,5 @@
>    uninit: function() {
> +    if (!this._initialized) {
> +      return;
> +    }
> +

And I guess we don't need to remove these if !this._eventListenersAdded.

Comment 4

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

Oh, I forgot to remove the _initialized business. It's not needed anymore.

Comment 5

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

https://hg.mozilla.org/projects/ux/rev/89dfb5b64ed0

Comment 6

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

Attachment: followup patch

I forgot to add a check in uninit to only remove the event listeners if they were added. Without this check, the PanelUI.xml cstor will run when a window is closed.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 781242 [details] [diff] [review]
followup patch

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

r=me

Comment 8

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

Landed the follow-up on UX, https://hg.mozilla.org/projects/ux/rev/91c9b0675e2b

Comment 9

[:Gijs (he/him)]

Is adding event listeners for begin/end batch updates & showHelpView really needed? For the former, I don't think we need the listeners to be there. For the latter, that can't really be called without the panel having been opened before... right?

Comment 10

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

I was added them as defensive programming in case they become needed in the future, but we can remove them if you feel strongly about it.

Comment 11

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/89dfb5b64ed0
https://hg.mozilla.org/mozilla-central/rev/91c9b0675e2b