Closed Bug 559169 Opened 12 years ago Closed 12 years ago

Implement a collection module that can be used to track Jetpack event handlers

Categories

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

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: adw, Assigned: adw)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
The high-level Jetpack APIs provide access to event handlers and things like event handlers in a unified way, like:

  obj.handlers.add(val);
  obj.handlers.add([val1, val2]);
  obj.handlers.remove(val);
  obj.handlers.remove([val1, val2]);
  obj.handlers = val;
  obj.handlers = [val1, val2];

It would be useful to have a module that defined both the |handlers| class and took care of the setter magic.

This patch adds a jetpack-core/collection modules that exports a Collections class and a method called addCollectionProperty.  Consumers can call that method to add a |handlers|-type property on an object.
Attachment #438836 - Flags: review?(avarma)
Blocks: 548590
Attached patch patch with docs (obsolete) — Splinter Review
Now with docs.  Should this be in some helper package instead of jetpack-core?  Is Collection an OK name?
Attachment #438836 - Attachment is obsolete: true
Attachment #439350 - Flags: review?(avarma)
Attachment #438836 - Flags: review?(avarma)
Attachment #439350 - Flags: review?(avarma) → review?(myk)
Comment on attachment 439350 [details] [diff] [review]
patch with docs

>+The `collection` module provides a simple array-like list class and utilities
>+for using it.

I would describe (and implement) this as a set-like class whose values must be unique, since that is the conventional behavior of web event handler interfaces like the properties for which this class is primarily going to be used (cf. addEventListener <http://www.w3.org/TR/DOM-Level-3-Events/>).

The only problem with describing this class as set-like is that sets are typically unordered, whereas this class's elements are iterated in the order in which they were added (which, incidentally, is consistent with the specification for addEventListener, and thus seems like the right behavior).


>+Adds a collection property to the given object.  Setting the property to a
>+single value adds that value to the collection, and setting it to an array adds
>+all the items in the array.

This should note that setting the property removes existing values in the process.  This might be intuitive, giving that you're setting a property, and doing that conventionally blows away its old value, but language like "adds that value to the collection" seems to imply preservation of existing values, so it would be good to be explicit here.


>+  obj.__defineSetter__(propName, function (itemOrItems) {
>+    array.splice(0, array.length);

Note that it's possible to leave out array.length from this splice call, albeit only on Spidermonkey.  I'm not sure whether it's any faster, though, and in this case speed probably doesn't matter, so we might as well leave it in on the off chance that makes the module reusable in other CommonJS environments (a lower priority than an optimal implementation, but still a nice-to-have).


>+  this.add = function Collection_add(itemOrItems) {
>+    let items = toArray(itemOrItems);
>+    for (let i = 0; i < items.length; i++)
>+      array.push(items[i]);
>+    return this;
>+  };

This should suppress duplicates by checking that |array.indexOf(items[i]) != -1| before pushing the item onto the array.


>Now with docs.  Should this be in some helper package instead of jetpack-core? 

Perhaps, but jetpack-core is fine for now.


>Is Collection an OK name?

Collection is a fine name.  It's a bit generic, but then Set is a bit inaccurate, and I don't have a better idea.  (EventSet? Ugh.)


r=myk with duplicate suppression!
Attachment #439350 - Flags: review?(myk) → review+
Attached patch patch v2Splinter Review
Thanks Myk, good points.

(In reply to comment #2)
> I would describe (and implement) this as a set-like class whose values must be
> unique, since that is the conventional behavior of web event handler interfaces

Went with:

"The collection module provides a simple list-like class and utilities for using it. A collection is ordered, like an array, but its items are unique, like a set."

> This should note that setting the property removes existing values in the
> process.

"Setting the property to a scalar value empties the collection and adds the value. Setting it to an array empties the collection and adds all the items in the array."

I also refactored the test a little, added checks for uniqueness, and clarified some other parts of the docs.
Attachment #439350 - Attachment is obsolete: true
http://hg.mozilla.org/labs/jetpack-sdk/rev/2d04e34a547c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.