bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 45

Status

Hello (Loop)
Client
P1
normal
Rank:
9
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: standard8)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [go faster])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Summary: LoopUI shouldn't try to add itself to the hidden window → LoopUI shouldn't try to use toolbar buttons in the hidden window (and should probably not use it at all on non-OSX)
Assignee: nobody → standard8
Blocks: 1223573
Rank: 9
Priority: -- → P1
Whiteboard: [go faster]
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1229487
Created attachment 8694710 [details] [diff] [review]
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.
Attachment #8694710 - Flags: review?(mdeboer)
Attachment #8694710 - Flags: feedback?(gijskruitbosch+bugs)
(Reporter)

Comment 3

3 years ago
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. :-)
Attachment #8694710 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
(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 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?
Attachment #8694710 - Flags: review?(mdeboer)
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 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ec770d354e5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.