Closed Bug 877178 Opened 11 years ago Closed 11 years ago

Allow add-ons/code to unregister an area

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M6])

Attachments

(5 files, 5 obsolete files)

This got its own bug after a quick discussion on IRC realized this'd be a hairy endeavour. What happens with the widgets in an area that are unregistered? Do we keep placements in the area in the pref? If you register, unregister, and the widgets end up in the palette, and the user sticks the widget into some other area, and then you re-register the old area, what happens with the widget (move/stay?).

Would like to sort this out because of tests; I'll take and try to get it done within M6.
Attached patch Untested Patch (obsolete) — Splinter Review
I'd like to test this once the tests for bug 873501 have landed and I can abuse the head.js from there etc.

Note also that this patch was made on top of the one for bug 877603; I guess it may not apply completely cleanly. :-(
Attachment #755875 - Flags: review?(jaws)
Depends on: 877851
Blocks: 877851
No longer depends on: 877851
So I'm writing tests and noticed that this patch misses the public accessor for unregisterArea, for a start.

Furthermore, I noticed we're not in the default state still - that is, there is still a mismatch between the default state we list in CustomizableUI.jsm and the state of the browser once it's started up. :-(
Attached patch Patch v1.1Splinter Review
This functions with the tests, which are coming in a bit...
Attachment #755875 - Attachment is obsolete: true
Attachment #755875 - Flags: review?(jaws)
Attachment #756502 - Flags: review?(jaws)
Attached patch Tests (obsolete) — Splinter Review
These tests won't work except with the updated patch for bug 877603.
Attachment #756504 - Flags: review?(jaws)
Comment on attachment 756504 [details] [diff] [review]
Tests

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

::: browser/components/customizableui/test/browser_877178_unregisterArea.js
@@ +3,5 @@
> +    desc: "Sanity checks",
> +    run: function() {
> +      let gotException = false;
> +      try {
> +        CustomizableUI.registerArea("@foo");

Factor out a function like this, and possibly add it to /testing/mochitest/tests/SimpleTest/SimpleTest.js

expectException: function(func, name) {
  let gotException = false;
  try {
    func();
  } catch (ex) { gotException = true; }
  ok(gotException, name);
}

Then the rest of the code can just do the following:

expectException(function() CustomizableUI.registerArea("@foo"), "Registering areas with an invalid ID should throw.");
expectException(function() CustomizableUI.registerArea([]), "Registering areas with an invalid ID should throw.");
...
Attachment #756504 - Flags: review?(jaws) → review+
Comment on attachment 756502 [details] [diff] [review]
Patch v1.1

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +244,5 @@
> +    // Delete all remaining traces.
> +    gAreas.delete(aName);
> +    gPlacements.delete(aName);
> +    gFuturePlacements.delete(aName);
> +    this.endBatchUpdate();

this.endBatchUpdate(true);

Need to pass true to endBatchUpdate because gDirty will be false when it is called. Each call to removeWidgetFromArea sets gDirty to try, but then it subsequently calls saveState which resets gDirty to false at the end of it.

@@ +1592,5 @@
>    registerMenuPanel: function(aPanel) {
>      CustomizableUIInternal.registerMenuPanel(aPanel);
>    },
> +  unregisterArea: function(aName, aProperties) {
> +    CustomizableUIInternal.unregisterArea(aName, aProperties);

aProperties is never used, we can just remove it :)
Attachment #756502 - Flags: review?(jaws) → review+
Rob, does this look good to you?
Attachment #756812 - Flags: review?(rcampbell)
Huh, except of course it needs to be made available to mochitest-browser separately...
Attachment #756812 - Attachment is obsolete: true
Attachment #756812 - Flags: review?(rcampbell)
Attachment #756823 - Flags: review?(rcampbell)
Pushed the code ( https://hg.mozilla.org/projects/ux/rev/4814c5774def ), waiting with test stuff to see if we can get SimpleTest/mochitest-browser improved.
Attached patch Tests, simplified (obsolete) — Splinter Review
Carrying over r+. This is what it looks like with expectException working.
Attachment #756829 - Flags: review+
Comment on attachment 756823 [details] [diff] [review]
Add expectException to SimpleTest, v2

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

ok, as per irc, I'm mostly ok with this, if somewhat creeped out at making changes to it.

please rename expectException to doesThrow() and we have an r+.

Wouldn't hurt to do a full try run to make sure we don't break anywhere.

::: testing/mochitest/browser-test.js
@@ +558,5 @@
>    this.info = function test_info(name) {
>      aTest.addResult(new testMessage(name));
>    };
> +  this.expectException = function test_expectException(fn, name) {
> +    self.SimpleTest.expectException(fn, name);

if this is going to be one of these types of tests, you should use the aTest.addResult(new testMessage(... )) business to nicely format your message.

In fact, I don't think you have to add this here at all, since your test is included in expectException as an ok().

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +268,5 @@
> +    try {
> +      fn();
> +    } catch (ex) { gotException = true; }
> +    ok(gotException, name);
> +};

ok.

name is probably more of a message than a name.

I might even rename this to doesThrow() to separate it from expectUncaughtException which has different semantics.

@@ +1119,5 @@
>  var todo_is = SimpleTest.todo_is;
>  var todo_isnot = SimpleTest.todo_isnot;
>  var isDeeply = SimpleTest.isDeeply;
>  var info = SimpleTest.info;
> +var expectException = SimpleTest.expectException;

Do you expect (heh) this to be used heavily? It's the first of the expect functions to get a global declaration.

It's also the only one of these that expects (darn) a function as an argument.

We've kept this list pretty short for a long time. Intentionally.
Attachment #756823 - Flags: review?(rcampbell) → review+
Simpler patch for the simpletest addition, carrying over r+.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=f3f512389f0f
Attachment #756823 - Attachment is obsolete: true
Attachment #756980 - Flags: review+
Tests using this version of SimpleTest (also in try push, from UX branch)
Attachment #756504 - Attachment is obsolete: true
Attachment #756829 - Attachment is obsolete: true
Attachment #756981 - Flags: review+
(In reply to :Gijs Kruitbosch from comment #13)
> Created attachment 756981 [details] [diff] [review]
> Tests using SimpleTest
> 
> Tests using this version of SimpleTest (also in try push, from UX branch)

So, funny thing, the default placement stuff is broken on the try push. It passes locally. I don't understand why.

The SimpleTest bits seem to be fine, and those tests pass, so if inbound is open in a few minutes, I'll land the SimpleTest change there, and it'll get merged back into UX sometime this weekend / on monday. Hopefully by that time I've figured out where the window-controls come from and why they're in the tabstoolbar on try, but not when I run the test locally.
Whiteboard: [Australis:M6] → [Australis:M6][leave open]
Attachment #756502 - Flags: checkin+
So it looks like this breaks on tinderboxen because the window-controls, which are normally in the navbar, outside the customization area, get pushed into the tab bar by the fullscreen code[1], which I guess is exercised by some of the other mochitests.

I can see four ways of fixing this:
 - make the placements code ignore the window controls in some way (probably by adding and respecting some attribute on the window-controls item)
 - make the fullscreen tests leave the window as they found it somehow. I suspect that the window not being what we expect it to be is a bug, but I'm not 100% sure.
 - make the tabstoolbar have its own customization target hbox, like the nav-bar. I'm scared of doing this for tab-styling reasons (as the tabs would ideally be inside the customization target).
 - make our tests call reset() before doing anything, and ensure that that moves the window-controls back.

I'm inclined to go with *both* the first and second option, and possibly the fourth: our code should be ignoring these (they ought not to be customizable), and the fullscreen tests shouldn't mess with our tests. Note that the latter is still only a suspicion so far; maybe they don't and something else is confusing us.

[1] http://hg.mozilla.org/projects/ux/file/918e40dc02a2/browser/base/content/browser-fullScreen.js#l572
So it turns out there's already an attribute that is used to deal with buttons which are in a toolbar but shouldn't end up in currentset because we move/replace them programmatically. I suspect we should probably respect that attribute - and add it to the window-controls. This passess all the browser/base and browser/components tests on my machine, including the new ones in the remaining patch. I've set off a try run as well: https://tbpl.mozilla.org/?tree=Try&rev=b6b7d0cb0e89 .
Attachment #757100 - Flags: review?(jaws)
(In reply to :Gijs Kruitbosch from comment #16)
> I can see four ways of fixing this:
>  - make the placements code ignore the window controls in some way (probably
> by adding and respecting some attribute on the window-controls item)
>  - make the fullscreen tests leave the window as they found it somehow. I
> suspect that the window not being what we expect it to be is a bug, but I'm
> not 100% sure.

So it turns out that the code there never puts the controls back if TabsOnTop is true, which I think it will soon be everywhere (?)... so we should probably not mess with it?

>  - make our tests call reset() before doing anything, and ensure that that
> moves the window-controls back.

This doesn't help without the first point, as the items don't have a removable="true" attribute and therefore don't get removed and then get included in the placements...
Attachment #757100 - Flags: review?(jaws) → review+
Status: NEW → ASSIGNED
Pushed:

https://hg.mozilla.org/projects/ux/rev/98f19b9442c0
https://hg.mozilla.org/projects/ux/rev/4b3950dd3859
Whiteboard: [Australis:M6][leave open] → [Australis:M6][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/4814c5774def
https://hg.mozilla.org/mozilla-central/rev/98f19b9442c0
https://hg.mozilla.org/mozilla-central/rev/4b3950dd3859
https://hg.mozilla.org/mozilla-central/rev/308d7f71097b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: