Closed
Bug 996899
Opened 10 years ago
Closed 10 years ago
Calling CustomizableUI.registerArea a second time with different properties should throw
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 31
People
(Reporter: jaws, Assigned: jaws)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [Australis:P5])
Attachments
(1 file)
8.19 KB,
patch
|
mconley
:
review+
bkerensa
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Follow-up from bug 996364, "I'm curious to know - what happens if "overflowable" or "legacy", or any of the other registerArea object properties change from one registration to the other?"
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
I also fixed an inconsistency with one of the const variables within CustomizableUI.jsm so that it also starts with a 'k'. That was the only one I found that was inconsistent.
Attachment #8407656 -
Flags: review?(mconley)
Comment 2•10 years ago
|
||
Comment on attachment 8407656 [details] [diff] [review] Patch Review of attachment 8407656 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Might want to double-check with Unfocused to make sure this jives with his original vision though. ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +296,5 @@ > } > > let areaIsKnown = gAreas.has(aName); > let props = areaIsKnown ? gAreas.get(aName) : new Map(); > + const kImmutableProperties = ["type", "legacy", "overflowable"]; For simple presence like this, I tend to prefer Sets, and using .has instead of Arrays and indexOf, but I won't harp on it. :) ::: browser/components/customizableui/test/browser_996364_registerArea_different_properties.js @@ +19,5 @@ > > add_task(function() { > let exceptionThrown = false; > try { > + CustomizableUI.registerArea("area-996364-2", {type: CustomizableUI.TYPE_TOOLBAR, "defaultCollapsed": "false"}); Why no quotes around "type", but quotes around "defaultCollapsed"? For consistencies sake, we should probably pick one and stick with it. IMO, you can probably keep the quotes on type, but either is fine with me. @@ +94,5 @@ > + CustomizableUI.registerArea("area-996899-3", { legacy: false }); > + } catch (ex) { > + exceptionThrown = ex; > + } > + ok(exceptionThrown, "Changing 'legacy' should throw an exception: " + (exceptionThrown ? exceptionThrown : "[no exception]")); I guess we could have used SimpleTest.doesThrow here instead, but then we don't get the message of the Error. Oh well, I guess.
Attachment #8407656 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 3•10 years ago
|
||
I tried to switch to SimpleTest.doesThrow but the test output showed "unknown test url" for the test cases that used SimpleTest.doesThrow which is very odd. I made the other requested changes. https://hg.mozilla.org/integration/fx-team/rev/6f49fe82d009
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
Flags: in-testsuite+
Whiteboard: [Australis:P5] → [Australis:P5][fixed-in-fx-team]
Comment 5•10 years ago
|
||
(In reply to Mike Conley (:mconley) from comment #2) > Looks good. Might want to double-check with Unfocused to make sure this > jives with his original vision though. Hah, my original vision was to never allow calling this twice - we already decided it was best to throw that out the door :) So yea, +1 to this patch.
Flags: needinfo?(bmcbride)
Comment 6•10 years ago
|
||
Oh, so, after reading the conversation on IRC about this, I realised the question was more *why* we now allow registerArea to be called twice. And bug 940013 doesn't make that obvious. And my head feels awful and I can't actually remember myself. Gijs?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6f49fe82d009
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P5][fixed-in-fx-team] → [Australis:P5]
Target Milestone: --- → Firefox 31
Assignee | ||
Updated•10 years ago
|
Comment 8•10 years ago
|
||
I think the gist is in https://bugzilla.mozilla.org/show_bug.cgi?id=940013#c1 . We're automatically calling registerArea when we first get a registerToolbarNode for an area that isn't registered, but if the properties we pass (based on defaultset, plus legacy: true, IIRC) aren't correct, I figured we should allow a second call in case consumers want other properties. I *think* we could (and possibly should) allow switching overflowable/non-overflowable (basically just a question of creating+enabling and/or disabling+destroying the toolbar.overflowable property for the area nodes), in part because getting a registerArea call done before the first registerToolbarNode call for XUL overlay add-ons is non-trivial (IIRC you need to be there at/before DOMContentLoaded). That said, I don't care deeply, and I forgot to ask a more important question in bug 996364: Dave, why did you call registerArea a second time? And what did you expect to happen (as opposed to an exception) ?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dtownsend+bugmail)
Comment 9•10 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #8) > I think the gist is in > https://bugzilla.mozilla.org/show_bug.cgi?id=940013#c1 . We're automatically > calling registerArea when we first get a registerToolbarNode for an area > that isn't registered, but if the properties we pass (based on defaultset, > plus legacy: true, IIRC) aren't correct, I figured we should allow a second > call in case consumers want other properties. > > I *think* we could (and possibly should) allow switching > overflowable/non-overflowable (basically just a question of > creating+enabling and/or disabling+destroying the toolbar.overflowable > property for the area nodes), in part because getting a registerArea call > done before the first registerToolbarNode call for XUL overlay add-ons is > non-trivial (IIRC you need to be there at/before DOMContentLoaded). That > said, I don't care deeply, and I forgot to ask a more important question in > bug 996364: > > Dave, why did you call registerArea a second time? And what did you expect > to happen (as opposed to an exception) ? I wasn't really (a mistake in a patch I was working on caused the add-on SDK code to call into it twice), I just noticed that the function looked like it supported being called twice yet was throwing.
Flags: needinfo?(dtownsend+bugmail)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8407656 [details] [diff] [review] Patch [Approval Request Comment] Bug caused by (feature/regressing bug #): followup from bug 996364 User impact if declined: this just strengthens the add-ons api, so if declined add-ons may call this function with different properties, causing unexpected/unknown behavior in the CustomizableUI feature Testing completed (on m-c, etc.): locally, on m-c, and automated testing Risk to taking this patch (and alternatives if risky): none expected String or IDL/UUID changes made by this patch: none
Attachment #8407656 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Attachment #8407656 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/64a06ad9fe0d
You need to log in
before you can comment on or make changes to this bug.
Description
•