Closed Bug 948985 Opened 7 years ago Closed 7 years ago

Ensure non-removable API-based widgets with a defaultArea work after having been destroyed

Categories

(Firefox :: Toolbars and Customization, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P4])

Attachments

(1 file, 1 obsolete file)

(In reply to Luís Miguel [:Quicksaver] from bug 947987 comment #3)
> [We should ensure that the non-removable widget ends up in its
> defaultArea if it has no placement.
> 
> For example (I haven't tested this, just throwing it out of my head from
> reading the code) if I create a custom aToolbar, then create a non-removable
> widget defaulting to aToolbar, then unregisterArea with aDestroyPlacements =
> true and then destroy the widget. At this point the widget placement is
> destroyed, but if I re-create aToolbar and the widget (in that order) the
> widget won't be placed in its defaultArea because gSeenWidgets will have the
> widget, and so its defaultArea won't be used for placing it.

Note that in this case we're assuming the new widget isn't listed in the defaultPlacements for the new aToolbar... which sounds somewhat implausible until you realize we persist gSeenWidgets forever-and-ever. So this kind of permanently nukes them. That is kind of sadfaces, also because they're non-removable so they'll always be unhappy widgets. Oops.
Summary: Ensure non-removable API-based widgets with a defaultArea work after having been removed → Ensure non-removable API-based widgets with a defaultArea work after having been destroyed
Test + fix
Attachment #8346196 - Flags: review?(bmcbride)
Comment on attachment 8346196 [details] [diff] [review]
ensure non-removable API-based widgets with a defaultArea work after destruction,

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

Not sure which is more confusing to actually use - having them show, or having them not show. But implementation wise, this is definitely the simplest - so lets go with this patch for now, and re-visit in the future if needed.

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1693,5 @@
>        // seen before, then add it to its default area so it can be used.
> +      // If the widget is not removable, we *have* to add it to its default
> +      // area here.
> +      if (autoAdd && !widget.currentArea &&
> +          (!gSeenWidgets.has(widget.id) || !widget.removable)) {

Non-removable widgets should *not* depend on autoAdd being true - that'll end up being another manifestation of this bug.
Attachment #8346196 - Flags: review?(bmcbride) → review-
Good point. Now with test. :-)
Attachment #8349389 - Flags: review?(bmcbride)
Attachment #8346196 - Attachment is obsolete: true
Attachment #8349389 - Flags: review?(bmcbride) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/f9e3b217d871
Status: NEW → ASSIGNED
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f9e3b217d871
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 29
I've verified this in my add-on with a custom toolbar and a non-removable widget in it. The widget is successfully placed back in the toolbar when I click the reset defaults button, and when I delete its placement directly in the prefs.js file. (Not marking as Verified yet because I'm unsure of how to test the "Non-removable widgets should *not* depend on autoAdd being true" part of the patch)
You need to log in before you can comment on or make changes to this bug.