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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P5])

Attachments

(1 file)

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: nobody → jaws
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
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 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+
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
Flags: in-testsuite+
Whiteboard: [Australis:P5] → [Australis:P5][fixed-in-fx-team]
Blair, are you OK with these changes?
Flags: needinfo?(bmcbride)
(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)
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)
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
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)
(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)
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?
Attachment #8407656 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.