559859 - Move some helpers from the context menu module to a common helpers module (publicConstructor(), validateOptions())

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

The context menu module patch in bug 548590 implements a few helper functions that might be useful to other modules.  It would be good to pull them out into a common helpers or utils module.  The functions are isArray, makePublicConstructor, and validateOptions.

Comment 1

[Atul Varma [:atul]]

Cool. Since Array.isArray() is actually part of ES5, we might want to just monkeypatch this method onto the Array prototype of all new Cu.Sandbox objects, until the actual platform adds native support for it.

Comment 2

[Drew Willcoxon :adw]

Attachment: patch

Adds jetpack-core/api-utils, with publicConstructor() and validateOptions().  At first I made an "options" module with validateOptions, but then I couldn't think of a place to stick publicConstructor.  api-utils seems like an OK module for now, but maybe you have a better idea?  Since adding Array.isArray() to sandboxes is the odd man out, I'd like to move that to a different bug.

Comment 3

[Atul Varma [:atul]]

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

Looks great. A few minor nits:

* You may want to mention that instances created with the public constructor are automatically memory.track()'d, so that the developer knows not to call it in the private constructor, which would effectively result in a double-track.

* I'm not sure, but it might actually be useful to provide an introductory/tutorial section to the markdown docs that shows the examples, instead of putting the examples at the end of the reference docs. The only reason I write this is b/c the reference docs are necessarily a bit complex and hard-to-grok if one doesn't understand the simple example cases.

* It would be cool to have a shortcut to validateOptions that, instead of an object-per-key, provides a string-per-key that simply specifies the type of the option, e.g. "string" vs. "number" vs. "object". We can talk about this or something else like it later though--for now, this looks great and should get into the SDK asap!

Comment 4

[Drew Willcoxon :adw]

Pushed: http://hg.mozilla.org/labs/jetpack-sdk/rev/db9af05cd6b1

(In reply to comment #3)
> * I'm not sure, but it might actually be useful to provide an
> introductory/tutorial section to the markdown docs that shows the examples,
> instead of putting the examples at the end of the reference docs.

Good idea, added an intro.  The examples are actually listed under their respective functions, and I left them there for now, since an example per function seems best way to allow the docs to scale.

> * It would be cool to have a shortcut to validateOptions that, instead of an
> object-per-key, provides a string-per-key that simply specifies the type of the
> option, e.g. "string" vs. "number" vs. "object".

Yeah, I had that thought too, but I'd like to keep the interface really really simple.  I'm not opposed to it though, but it would be good to hear first if that's something people ask for when they use it.

Comment 5

[Drew Willcoxon :adw]

(In reply to comment #4)
> > * It would be cool to have a shortcut to validateOptions that, instead of an
> > object-per-key, provides a string-per-key that simply specifies the type of the
> > option, e.g. "string" vs. "number" vs. "object".
> 
> Yeah, I had that thought too, but I'd like to keep the interface really really
> simple.  I'm not opposed to it though, but it would be good to hear first if
> that's something people ask for when they use it.

This is a good idea, so I filed bug 563555.

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