563447 - Update context-menu module to use new api-utils module

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Drew Willcoxon :adw]

Bug 559859 moved publicConstructor() and validateOptions() to a new api-utils module.  We should remove those implementations from context-menu and replace their uses with api-utils.

Comment 1

[Drew Willcoxon :adw]

Attachment: patch

Replaces proto-api-utils implementations with api-utils use.  Noticed an anon function that needs a name, took the opportunity to name it.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 444228 [details] [diff] [review]
patch

>diff --git a/packages/jetpack-core/lib/context-menu.js 

>-  this.__defineGetter__("onClick", function () options.onClick);
>-  this.__defineGetter__("data", function () options.data);
>+  this.__defineGetter__("onClick", function () options.onClick || undefined);

Perhaps I'm not seeing something, but if onClick has been validated to be either a function or undefined, then isn't it redundant to return onClick || undefined, as returning just onClick will return undefined if the property hasn't been set to a function?


>+  this.__defineGetter__("data", function () options.data || undefined);

I might be missing something here too, but if data has been set to the empty string, which evaluates to false in a conditional, then it seems like this will return undefined when it should be returning the empty string.

r- per the issue with data, will switch to r+ once you point out what I'm missing. ;-)

Comment 3

[Drew Willcoxon :adw]

Attachment: patch v2

(In reply to comment #2)
> Perhaps I'm not seeing something, but if onClick has been validated to be
> either a function or undefined, then isn't it redundant to return onClick ||
> undefined, as returning just onClick will return undefined if the property
> hasn't been set to a function?

The problem is, if an option "foo" is optional (undefined is a valid value) and !("foo" in options), then !("foo" in validateOptions(options, requirements)).  So the || undefined is only there to avoid strict warnings.

Originally it was the case that if ("foo" in requirements), then ("foo" in validateOptions()).  But Felipe had a case where the value of "foo" was a number, zero being a valid value, so he couldn't use the idiom |if (validateOptions().foo)|, and he also couldn't use ("foo" in validatedOptions()) since it was always true, so he had to resort to some awkward or redundant checks.  If you have any thoughts about this, I'd love to hear them.

> I might be missing something here too, but if data has been set to the empty
> string, which evaluates to false in a conditional, then it seems like this will
> return undefined when it should be returning the empty string.

D'oh, totally right.

I also noticed I was doing an unnecessary options.items || [] in Menu.  options.items is validated to be an array, so it can't be undefined.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 444409 [details] [diff] [review]
patch v2

(In reply to comment #3)
> If you have any thoughts about this, I'd love to hear them.

Hmm, that makes perfect sense; I don't have any better ideas.

Looks great, r=myk!

Comment 5

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack-sdk/rev/1cd09bb9b5f5

Comment 6

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".