Closed Bug 890319 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: jruderman, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P4][Australis:M9])

Attachments

(2 files, 1 obsolete file)

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
P4 since it you get a useless window, but can simply close it (and try customizing again).
Whiteboard: [Australis:P4]
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).
Attachment #819677 - Flags: review?(mdeboer)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attached patch diff -wSplinter Review
Here's a diff -w to ease reviewing.
Attachment #819680 - Attachment description: about:customize doesn't work if we try to enter before the window has loaded, → diff -w
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?
(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 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 ;)
Attachment #819677 - Flags: review?(mdeboer) → review+
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.
Attachment #820279 - Flags: review?(mdeboer)
Attachment #819677 - Attachment is obsolete: true
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.
Attachment #820279 - Flags: review?(mdeboer) → review+
Landed with an initialized variable: https://hg.mozilla.org/projects/ux/rev/68624eb6d2a4
Whiteboard: [Australis:P4] → [Australis:P4][Australis:M9][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/68624eb6d2a4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][Australis:M9][fixed-in-ux] → [Australis:P4][Australis:M9]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.