Closed Bug 877447 Opened 8 years ago Closed 7 years ago

Make CustomizableUI.inDefaultState skip IDs for missing items

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M7])

Attachments

(1 file, 1 obsolete file)

CustomizableUI.inDefaultState currently compares the placements of each registered area with their default placement array.

We're not taking into account the possibility that there are IDs within the current placements of an area for items that no longer exist. This can happen if, for example, an add-on adds a widget, and then the add-on is disabled or removed. In that case, the item's ID is still in the placement array, but CustomizableUI just skips over it.

CustomizableUI.inDefaultState, however, will return false, because this missing item's ID isn't in the default state array.

That might be confusing / inconsistent, from a user's perspective.

We can probably be smarter about this by skipping those missing item IDs when comparing the current placements with the default placements.

I'm not convinced this should block Australis at all, but I'll M? it anyways for later triage.
Attached patch Patch (obsolete) — Splinter Review
This actually turned out to be fairly straightforward...
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #759096 - Flags: review?(mconley)
Attached patch PatchSplinter Review
Oops, this time without the unnecessary head.js hunk...
Attachment #759096 - Attachment is obsolete: true
Attachment #759096 - Flags: review?(mconley)
Attachment #759097 - Flags: review?(mconley)
Comment on attachment 759097 [details] [diff] [review]
Patch

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

This looks fine to me - just a few suggestions.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1650,5 @@
>          continue;
>        }
>  
>        let currentPlacements = gPlacements.get(areaId);
> +      // If we have a build area, check that all of these placements exist:

I think we should put in a little more detail documenting what this block is doing.

Specifically - we're excluding all of the placement IDs for items that do not exist, because we don't want to consider them when determining if we're in the default state. This way, if an add-on introduces a widget and is then uninstalled, the leftover placement doesn't cause us to automatically assume that the buttons are not in the default state.

@@ +1655,5 @@
> +      let buildAreaNodes = gBuildAreas.get(areaId);
> +      if (buildAreaNodes && buildAreaNodes.size) {
> +        let container = [...buildAreaNodes][0];
> +        // Clone the array so we don't modify the actual placements...
> +        currentPlacements = currentPlacements.slice(0);

That's a neat trick - didn't know that one. :) Alternatively, you could have used:

currentPlacements = [...currentPlacements];

To keep with the Harmony spread-usage.

::: browser/components/customizableui/test/browser_877447_skip_missing_ids.js
@@ +4,5 @@
> +
> +let gTests = [
> +  {
> +    run: function() {
> +      let kButtonId = "look-at-me-disappear-button";

If this is a konstant, then we should use const

@@ +11,5 @@
> +      let btn = createDummyXULButton(kButtonId, "Gone!");
> +      CustomizableUI.addWidgetToArea(kButtonId, CustomizableUI.AREA_NAVBAR);
> +      ok(!CustomizableUI.inDefaultState, "Should no longer be in the default state.");
> +      is(btn.parentNode.parentNode.id, CustomizableUI.AREA_NAVBAR, "Button should be in navbar");
> +      btn.remove();

Oh, that's right! We can do Element.remove() now instead of Element.parentNode.remove(Element). Happy day!

@@ +19,5 @@
> +    }
> +  }
> +];
> +
> +

Nit - unnecessary newlines on line 23 and 34
Attachment #759097 - Flags: review?(mconley) → review+
This is pretty much done. So M7 it is!
Whiteboard: [Australis:M?] → [Australis:M7]
Pushed: https://hg.mozilla.org/projects/ux/rev/ebbce33ac9ca
Whiteboard: [Australis:M7] → [Australis:M7][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/ebbce33ac9ca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M7][fixed-in-ux] → [Australis:M7]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.