890319 - [Australis] Dragging about:customizing to a new window breaks it

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jesse Ruderman]

UX 25.0a1 (2013-07-04)

1. Menu > Customize
2. Right-click the tab and select "Move to New Window", or drag the tab out.

Result: The detached tab shows about:customizing in the address bar and has an empty content area

Expected: empty address bar, working customize mode

Comment 1

[Justin Dolske [:Dolske]]

P4 since it you get a useless window, but can simply close it (and try customizing again).

Comment 2

[:Gijs (he/him)]

Attachment: about:customize doesn't work if we try to enter before the window has loaded,

This fixes things for me. The problem is that we try to access various objects (like PanelUI) that are instantiated in delayedStartup, which also adds handlers to the toolbox to deal with customize mode. We basically shouldn't try to enter customize mode before that listener has finished firing. AFAICT there isn't really a reliable way of telling whether or not it has, so I'm just relying on our own APIs. Alternatively, we could make delayedStartup set a flag when it's finished (in addition to firing an observer notification).

Comment 3

[:Gijs (he/him)]

Attachment: diff -w

Here's a diff -w to ease reviewing.

Comment 4

[Dão Gottwald [::dao]]

Comment on attachment 819677 [details] [diff] [review]
about:customize doesn't work if we try to enter before the window has loaded,

>+    Task.spawn(function() {
>+      // If the window's delayed startup methods haven't run, we can't access this yet,
>+      // so wait until they have:
>+      if (!this.window.PanelUI.panel) {
>+        let delayedStartupDeferred = Promise.defer();
>+        let delayedStartupObserver = function(aSubject) {
>+          if (aSubject == this.window) {
>+            Services.obs.removeObserver(delayedStartupObserver, "browser-delayed-startup-finished");
>+            delayedStartupDeferred.resolve();
>+          }
>+        }.bind(this);
>+        Services.obs.addObserver(delayedStartupObserver, "browser-delayed-startup-finished", false);
>+        yield delayedStartupDeferred.promise;
>+      }

Can you make PanelUI dispatch an event when it's initialized?

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #4)
> Comment on attachment 819677 [details] [diff] [review]
> about:customize doesn't work if we try to enter before the window has loaded,
> 
> >+    Task.spawn(function() {
> >+      // If the window's delayed startup methods haven't run, we can't access this yet,
> >+      // so wait until they have:
> >+      if (!this.window.PanelUI.panel) {
> >+        let delayedStartupDeferred = Promise.defer();
> >+        let delayedStartupObserver = function(aSubject) {
> >+          if (aSubject == this.window) {
> >+            Services.obs.removeObserver(delayedStartupObserver, "browser-delayed-startup-finished");
> >+            delayedStartupDeferred.resolve();
> >+          }
> >+        }.bind(this);
> >+        Services.obs.addObserver(delayedStartupObserver, "browser-delayed-startup-finished", false);
> >+        yield delayedStartupDeferred.promise;
> >+      }
> 
> Can you make PanelUI dispatch an event when it's initialized?

That is possible, but I suspect we also need to wait for the rest of browser-delayed-startup to finish. It does other stuff, like initializing the lightweight theme listener, making the toolbox track customize events (!) etc.

As noted, using PanelUI is an imperfect way of checking "has browser-delayed-startup finished" (and should I add an observer) but if anything I think we should change that, not restrict ourselves to waiting only for the panel to be initialized.

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 819677 [details] [diff] [review]
about:customize doesn't work if we try to enter before the window has loaded,

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

LGTM, but absolutely thanks to the diff -w patch... brownie points!!

Small thing, can you use 'mikedeboer' in the commit message? Thanks ;)

Comment 7

[:Gijs (he/him)]

Attachment: about:customize doesn't work if we try to enter before the window has loaded,

So when I looked at this again, I felt we should really be setting a flag once delayed startup finishes. So this patch does that instead. Mike, sorry for the second r? request on a very similar patch. I'm not attaching another diff -w - this is the same patch as before, but with the browser.js hunk added and the check in CustomizeMode.enter changed to check for the flag we set there.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 820279 [details] [diff] [review]
about:customize doesn't work if we try to enter before the window has loaded,

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

::: browser/base/content/browser.js
@@ +1184,5 @@
>        TabView.init();
>  
>        setTimeout(function () { BrowserChromeTest.markAsReady(); }, 0);
>      });
> +    this.delayedStartupFinished = true;

r=me with a properly initialized property on gBrowserInit.

Comment 9

[:Gijs (he/him)]

Landed with an initialized variable: https://hg.mozilla.org/projects/ux/rev/68624eb6d2a4

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/68624eb6d2a4