Closed Bug 887438 Opened 8 years ago Closed 7 years ago

Add currentset setter shim for all toolbars

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:M8])

Attachments

(1 file)

As just discussed, it's probably a good idea to have a backwards compatibility thing for the currentSet setter.
Attached patch Patch with testsSplinter Review
Attachment #771616 - Flags: review?(jaws)
Comment on attachment 771616 [details] [diff] [review]
Patch with tests

>diff --git a/browser/components/customizableui/test/head.js b/browser/components/customizableui/test/head.js
> function getAreaWidgetIds(areaId) {
>-  let widgetAry = CustomizableUI.getWidgetsInArea(areaId);
>-  return widgetAry.map(x => x.id);
>+  return CustomizableUI.getWidgetIdsInArea(areaId);
> }

I guess this is unrelated because I didn't really use it in this test, but now that we have an API for this I figured we should probably not wrap and unwrap these in our tests... it's a little silly.
Status: NEW → ASSIGNED
Blocks: 890245
Comment on attachment 771616 [details] [diff] [review]
Patch with tests

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

::: browser/components/customizableui/content/toolbar.xml
@@ +133,4 @@
>          ]]></getter>
>        </property>
>  
>        <property name="currentSet" readonly="true">

Now that there is a setter, readonly=true seems wrong.

::: browser/components/customizableui/test/browser_887438_currentset_shim.js
@@ +34,5 @@
> +      is(newCurrentSet, navbar.currentSet, "Current set should match expected current set.");
> +      let feedBtn = document.getElementById("feed-button");
> +      let syncBtn = document.getElementById("sync-button");
> +      ok(feedBtn, "Feed button should have been added.");
> +      ok(syncBtn, "Sync button should have been added.");

Please add a check that feedBtn.nextElementSibling == syncBtn as well as syncBtn.nextElementSibling == homeButton.
Attachment #771616 - Flags: review?(jaws) → review+
Pushed with the issues from comment #3 fixed: https://hg.mozilla.org/projects/ux/rev/1b4535123806
Whiteboard: [Australis:M?] → [Australis:M8][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/1b4535123806
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.