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

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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).

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Not super important, but it should help with bugs similar to bug 892955.

Comment 2

[:Gijs (he/him)]

Attachment: getPlacementOfWidget should return null for non-existent widgets

So, something like this would probably work...

Comment 3

[:Gijs (he/him)]

(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? :-(

Comment 4

[:Gijs (he/him)]

Attachment: getPlacementOfWidget should return null for non-existent widgets

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.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Yea, that sounds like it should work.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 7

[:Gijs (he/him)]

Attachment: getPlacementOfWidget should return null for non-existent widgets

Yeah, the aArea thing was a leftover from my previous iteration. You're right that without it, we can check things immediately.

Comment 8

[:Gijs (he/him)]

Attachment: getPlacementOfWidget should return null for non-existent widgets

Of course, testing it should have happened before putting this up... (missing bracket, fixed in this patch) :-\

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/3816e384573d

Comment 10

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/3816e384573d