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)
Firefox
Toolbars and Customization
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)
2.35 KB,
patch
|
jaws
:
review+
Gijs
:
checkin+
|
Details | Diff | Splinter Review |
1.33 KB,
patch
|
Gijs
:
review+
Gijs
:
checkin+
|
Details | Diff | Splinter Review |
5.27 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
2.94 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
1.82 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 2•11 years ago
|
||
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. :-(
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
These tests won't work except with the updated patch for bug 877603.
Attachment #756504 -
Flags: review?(jaws)
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
Rob, does this look good to you?
Attachment #756812 -
Flags: review?(rcampbell)
Assignee | ||
Comment 8•11 years ago
|
||
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)
Assignee | ||
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
Carrying over r+. This is what it looks like with expectException working.
Attachment #756829 -
Flags: review+
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
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+
Assignee | ||
Comment 14•11 years ago
|
||
(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.
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 756980 [details] [diff] [review] Simpler SimpleTest patch https://hg.mozilla.org/integration/mozilla-inbound/rev/20dbb98fcac2
Attachment #756980 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Whiteboard: [Australis:M6] → [Australis:M6][leave open]
Assignee | ||
Updated•11 years ago
|
Attachment #756502 -
Flags: checkin+
Assignee | ||
Comment 16•11 years ago
|
||
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
Assignee | ||
Comment 17•11 years ago
|
||
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)
Assignee | ||
Comment 18•11 years ago
|
||
(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...
Updated•11 years ago
|
Attachment #757100 -
Flags: review?(jaws) → review+
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 20•11 years ago
|
||
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]
Comment 21•11 years ago
|
||
Originally included in bug 885062 attachment 787438 [details] [diff] [review] but it was unrelated to that bug and was a follow-up to attachment 757100 [details] [diff] [review]. https://hg.mozilla.org/projects/ux/rev/308d7f71097b
Attachment #790056 -
Flags: review+
Assignee | ||
Comment 22•11 years ago
|
||
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.
Description
•