1229471 - LoopUI shouldn't try to use toolbar buttons in the hidden window (and should probably not use it at all on non-OSX)

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[:Gijs (he/him)]

createLoopButton/<.onBuild() bootstrap.js:711
    CustomizableUIInternal.wrapWidgetEventHandler/aWidget[aEventName]() CustomizableUI.jsm:2396
    CustomizableUIInternal.buildWidget() CustomizableUI.jsm:1331
    WidgetGroupWrapper_forWindow() CustomizableUI.jsm:3755
    WindowListener.setupBrowserUI/LoopUI.toolbarButton() bootstrap.js:46
    WindowListener.setupBrowserUI/LoopUI.updateToolbarState() bootstrap.js:312
    WindowListener.setupBrowserUI/LoopUI.init() bootstrap.js:286
    WindowListener.setupBrowserUI() bootstrap.js:647
    observer() bootstrap.js:777
    gBrowserInit._delayedStartup() browser.js:1407 


At a minimum updateToolbarState() probably shouldn't be called on the hidden window, but there might be more that shouldn't really happen.

Comment 2

[Mark Banner (:standard8)]

Attachment: LoopUI shouldn't try to use toolbar buttons in the hidden window.

Ok, this stops us trying to initialise on the hidden window on non-Mac, and also stops us trying to do things with the toolbar in the hidden window on Mac. I've also spread a few comments around so we're less likely to be caught by this later.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8694710 [details] [diff] [review]
LoopUI shouldn't try to use toolbar buttons in the hidden window.

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

I suspect the OSX checks are enough to avoid having too much of an issue here for the time being. I don't understand the other changes, maybe mostly because I don't know what MozLoopService does and why you need to initialize it on the hidden window. f+ for the appconstants stuff, Mike should probably tell you what to do about the service and the observer calls...

::: browser/extensions/loop/bootstrap.js
@@ +276,3 @@
>          // This is a promise for test purposes, but we don't want to be logging
>          // expected errors to the console, so we catch them here.
>          this.MozLoopService.initialize().catch(ex => {

Why does the service need to be available on the hidden window?

@@ +288,5 @@
> +        if (window == Services.appShell.hiddenDOMWindow) {
> +          return;
> +        }
> +
> +        // Add observer notifications before the service is initialized

This is clearly no longer true now, because the initialize() call is earlier...

@@ +834,5 @@
>      chatbox.content.contentWindow.close();
>    });
>  
> +  if (AppConstants.platform == "macosx") {
> +    // Detach from hidden window (for OS X).

Uber-nit: keep the comment on the same side of the if() block in both the attach and detach case. :-)

Comment 4

[Mark Banner (:standard8)]

(In reply to :Gijs Kruitbosch from comment #3)
> ::: browser/extensions/loop/bootstrap.js
> @@ +276,3 @@
> >          // This is a promise for test purposes, but we don't want to be logging
> >          // expected errors to the console, so we catch them here.
> >          this.MozLoopService.initialize().catch(ex => {
> 
> Why does the service need to be available on the hidden window?

The hidden window on Mac is for when all other windows are closed. I'm not sure if its possible to open Firefox without opening any windows, but I guess it could be using an add-on or something. In which case, I'd rather we initialise the service just in case...

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8694710 [details] [diff] [review]
LoopUI shouldn't try to use toolbar buttons in the hidden window.

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

::: browser/extensions/loop/bootstrap.js
@@ +289,5 @@
> +          return;
> +        }
> +
> +        // Add observer notifications before the service is initialized
> +        Services.obs.addObserver(this, "loop-status-changed", false);

Well, in this case you'll also want to move the 'unload' handler down, because that only removes this observer.

However, this makes me wonder: why are we calling init() at all for the hidden window?

Comment 6

[Mark Banner (:standard8)]

Mike and I discussed this on irc:

15:39	Standard8	mikedeboer: does https://bugzilla.mozilla.org/show_bug.cgi?id=1229471#c4 answer your question? I think we're also going to be injecting the Tools -> Start a Conversation menu item as well
15:39	Standard8	when I remember to file a bug on it
15:39	* Standard8 starts that now
15:42	mikedeboer	Standard8: right. ok, well r=me with the unload handler moved down.
15:42	mikedeboer	(and Gijs' comments adressed)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2ec770d354e5

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ec770d354e5