Closed Bug 893609 Opened 8 years ago Closed 7 years ago

CustomizableUI.getPlacementOfWidget () should return null for non-existent widgets

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Unfocused, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M9][Australis:P3])

Attachments

(1 file, 3 obsolete files)

From bug 892955 comment 4:

> (In reply to :Gijs Kruitbosch from comment #0)
> > Note further that after step 2, getPlacementOfWidget still returns a
> > placement, even though the widget itself doesn't exist. I'm not convinced we
> > should expose that
> 
> Agreed - seems like it should just return null. The saved positions of
> non-existent widgets should be invisible from the API (at least by default -
> dunno if we'll want a separate API for that in the future).
Not super important, but it should help with bugs similar to bug 892955.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P3]
So, something like this would probably work...
Assignee: nobody → gijskruitbosch+bugs
(In reply to :Gijs Kruitbosch from comment #2)
> Created attachment 795395 [details] [diff] [review]
> getPlacementOfWidget should return null for non-existent widgets
> 
> So, something like this would probably work...

... except, of course, it'll return null for lazily loaded areas like the panel if they haven't been constructed yet.

I'm wondering if this is worth the trouble. Blair, what do you think? :-(
Flags: needinfo?(bmcbride)
On second thought, why don't we make it behave that way just for API-based widgets? That's more foolproof and that way we don't break anyone's expectations, and still avoid the original problems.
Attachment #795411 - Flags: review?(bmcbride)
Attachment #795395 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Yea, that sounds like it should work.
Flags: needinfo?(bmcbride)
Comment on attachment 795411 [details] [diff] [review]
getPlacementOfWidget should return null for non-existent widgets

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1103,5 @@
>  
>      return [...widgets];
>    },
>  
> +  getPlacementOfWidget: function(aWidgetId, aNeedsToExist) {

Nit: Got confused by 'aNeedsToExist'. How about something like 'aOnlyRegistered' or 'aExcludeDead' or something ?

@@ +1108,3 @@
>      for (let [area, placements] of gPlacements) {
>        let index = placements.indexOf(aWidgetId);
> +      if (index != -1 && (!aNeedsToExist || this.widgetExists(aWidgetId, area))) {

widgetExists() doesn't seem to use |area|, so that call doesn't need to be inside the loop. Or did you mean it to use |area|?

@@ +1124,5 @@
> +    if (gSeenWidgets.has(aWidgetId)) {
> +      return false;
> +    }
> +
> +    return true;

Should add a comment here stating the assumption that since it's a XUL widget and we're querying it, we're assuming it exists.
Attachment #795411 - Flags: review?(bmcbride) → review-
Yeah, the aArea thing was a leftover from my previous iteration. You're right that without it, we can check things immediately.
Attachment #795933 - Flags: review?(bmcbride)
Attachment #795411 - Attachment is obsolete: true
Of course, testing it should have happened before putting this up... (missing bracket, fixed in this patch) :-\
Attachment #795959 - Flags: review?(bmcbride)
Attachment #795933 - Attachment is obsolete: true
Attachment #795933 - Flags: review?(bmcbride)
Attachment #795959 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/projects/ux/rev/3816e384573d
Whiteboard: [Australis:M?][Australis:P3] → [Australis:M9][Australis:P3][fixed-in-ux]
Blocks: 910190
https://hg.mozilla.org/mozilla-central/rev/3816e384573d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P3][fixed-in-ux] → [Australis:M9][Australis:P3]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.