987492 - CustomizableUI.jsm should provide convenience APIs around windows

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

So, working with browser windows is a PITA. Big surprise, right? Enumerating over them is actually filled with all all sorts of gotchas, and most add-ons get the edge-cases wrong for the cases where they want to do something to what they consider a "normal" browser window. Ditto with detecting when a browser window opens/closes.

We can very easily provide a simple API do handle all this in CustomizableUI.jsm

Seems like a nice fit, for a few reasons:
* Extremely simple to do - it's just exposing what's already there
* Noticed this hole when making an add-on - CustomizableUI feels like the new defacto entrypoint for most UI modifications these days, regardless of whether you're using the new API wholesale or not

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Example usage

And here's a simple add-on using the APIs added - made it so much easier to implement the add-on. The alternative would have been quite painful.

Comment 3

[:Gijs (out for now; he/him)]

Comment on attachment 8396134 [details] [diff] [review]
Patch v1

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

I have rather substantial comments, but on the other hand, they should be relatively easy to address, so I'm just going to give you r+ and leave it up to you if you want to ask for another review and/or bikeshed the naming with me. :-)

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +810,5 @@
>  
>        aWindow.addEventListener("unload", this);
>        aWindow.addEventListener("command", this, true);
> +
> +      this.notifyListeners("onWindowOpened", aWindow);

<bikeshed>
In order to avoid confusion with load, DOMContentLoaded and delayedStartup events (we already have rather many definitions of "this window has loaded!), can we make these be called "onWindowRegistered" and "onWindowUnregistered" ?
</bikeshed>

::: browser/components/customizableui/test/browser_987492_window_api.js
@@ +21,5 @@
> +    }
> +  }
> +  CustomizableUI.addListener(openListener);
> +  let win = yield openAndLoadWindow(null, true);
> +  let newWindow = yield openDeferred.promise;

This will hang and timeout if it doesn't work. Can we fix that? Maybe do a setTimeout that does an openDeferred.reject() ?

Also, I'm pretty sure that you're guaranteed to have been called by the time delayedStartup has finished in the new window, because registerToolbar gets called somewhere before "load", which means registerBuildWindow is also called before then, which is well before delayedStartup.

So to make this simpler, we could just have the onWindowOpened callback set a boolean, and ok() that boolean. :-)

@@ +40,5 @@
> +    }
> +  }
> +  CustomizableUI.addListener(closeListener);
> +  yield promiseWindowClosed(newWindow);
> +  let oldWindow = yield closeDeferred.promise;

Same here as for the openDeferred.promise

Comment 4

[:Gijs (out for now; he/him)]

(I think we should try to get this for 29 still, for add-on compat - Blair, do you have time to finish this up?)

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Entirely dependant on how much time/energy I have during the weekend, when I'm taking a break from the weekday stuff :)

Comment 6

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch (no internet 29 Mar - 6 Apr) from comment #3)
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +810,5 @@
> >  
> >        aWindow.addEventListener("unload", this);
> >        aWindow.addEventListener("command", this, true);
> > +
> > +      this.notifyListeners("onWindowOpened", aWindow);
> 
> <bikeshed>
> In order to avoid confusion with load, DOMContentLoaded and delayedStartup
> events (we already have rather many definitions of "this window has
> loaded!), can we make these be called "onWindowRegistered" and
> "onWindowUnregistered" ?
> </bikeshed>

Blair and I discussed this on IRC and agreed to leave the events named as they are in order to keep them easy to understand and "promote" them in that way.

Comment 7

[:Gijs (out for now; he/him)]

Attachment: CustomizableUI.jsm should provide convenience APIs around windows,

Updated the test only. Mike, can you sanity-check me? (Yes, the test passes, but I always feel bad updating someone else's patch that I reviewed and landing in one go).

Comment 8

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

Comment on attachment 8398486 [details] [diff] [review]
CustomizableUI.jsm should provide convenience APIs around windows,

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

I like this a lot! Don't forget to add a dev-doc-needed or whathaveyou so that we can get this into the CustomizableUI.jsm documentation.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +2485,5 @@
> +   * Note that this can *only* be used as an iterator. ie:
> +   *     for (let window of CustomizableUI.windows) { ... }
> +   */
> +  windows: {
> +    "@@iterator": function*() {

Oooooh - fancy new stuff.

@@ +2576,5 @@
>     *     Fired when a widget's DOM node is *not* overflowing its container, a
>     *     toolbar, anymore.
> +   *   - onWindowOpened(aWindow)
> +   *     Fired when a window has been opened that is managed by CustomizableUI,
> +   *     once all the prerequisite setup has been done.

Nit - "all the" -> "all of the". Isn't code review grand! :)

::: browser/components/customizableui/test/browser_987492_window_api.js
@@ +25,5 @@
> +  isnot(newWindow, null, "Should have gotten onWindowOpen event");
> +  is(newWindow, win, "onWindowOpen event should have received expected window");
> +  CustomizableUI.removeListener(openListener);
> +
> +

Nit - extra newlines don't add much. (Here and line 49).

Comment 9

[:Gijs (out for now; he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/5616b5e7bec1


(In reply to Mike Conley (:mconley) from comment #8)
> ::: browser/components/customizableui/test/browser_987492_window_api.js
> @@ +25,5 @@
> > +  isnot(newWindow, null, "Should have gotten onWindowOpen event");
> > +  is(newWindow, win, "onWindowOpen event should have received expected window");
> > +  CustomizableUI.removeListener(openListener);
> > +
> > +
> 
> Nit - extra newlines don't add much. (Here and line 49).

FWIW, this was something Blair did and that I liked. I would be in favour of more liberally whitespacing our code to separate logical bits, but that's a separate discussion. Landed with this addressed per the review. :-)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5616b5e7bec1

Comment 11

[:Gijs (out for now; he/him)]

Comment on attachment 8398486 [details] [diff] [review]
CustomizableUI.jsm should provide convenience APIs around windows,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: missing APIs on 29 as compared to 30/31
Testing completed (on m-c, etc.): on m-c for 2 weeks now
Risk to taking this patch (and alternatives if risky): extremely low, only API addition
String or IDL/UUID changes made by this patch: adds API to JSM, but no IDL/UUID changes.

Comment 12

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

Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/5dc2f1b960ed

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/9c70e4856b3f

Comment 13

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified that using the add-on attached using Firefox 29 beta 7 (20140410150427), latest Aurora 30.0a2 (20140411004002) and latest Nightly 31.0a1 (20140410030200) the Quit Firefox option gives the three functional options (Restart, Safe Mode and Quit). Using the Nightly build from 2014-03-25, the Options are not displayed, the browser is closed.

I used Win 7 64-bit, Ubuntu 32-bit and MAC OSX 10.9.2. 

Is this enough to mark this bug as verified or exploratory testing is still needed? Thank you

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Petruta Rasa [QA] [:petruta] from comment #13)
> Is this enough to mark this bug as verified

Yes, thankyou :)