Closed Bug 588732 Opened 14 years ago Closed 14 years ago

EventEmitter base class

Categories

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

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: irakli, Assigned: irakli)

References

Details

Attachments

(1 file, 6 obsolete files)

Addressing review commets
12.22 KB, patch
myk
: review+
Details | Diff | Splinter Review 
The EventEmitter is a is the base class for all classes that emit events.
The EventEmitter class implements well known EventTarget interface from DOM Level 2, implemented by all Nodes in an implementation which supports the DOM Event Model. 

Prior art:
http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-EventTarget
http://nodejs.org/api.html#EventEmitter
http://livedocs.adobe.com/flash/9.0/ActionScriptLangRefV3/flash/events/EventDispatcher.html
No longer depends on: 584707
No longer blocks: 588198
Comment on attachment 467437 [details] [diff] [review]
Implementation + Documentation + Tests

This seems useful, as it is a familiar interface for addon developers with a web background, although I think we should continue to direct them to use the on* properties rather than the EventEmitter interface.

And we should definitely not simply document both interfaces in the module documentation and give developers the choice of which to use, as that introduces unnecessary complexity.  But we can certainly add an addendum to the documentation that describes the availability of this interface on APIs that dispatch events (once we update those APIs to support this interface).

A few issues with the implementation and its documentation are noted in the github change page.
Attachment #467437 - Flags: review?(myk) → review-
Summary: EventEmiter base class → EventEmitter base class
Assignee: nobody → rFobic
Attached patch Addressing review commets (obsolete) — — Splinter Review 
Addressing review comments & adding one convenience function.

Link to the commit with patch:
http://github.com/Gozala/jetpack-sdk/commit/f2a75647e3ff4167ff717dc979dc20230f5e34e7

Branch for this bug:
http://github.com/Gozala/jetpack-sdk/commits/events%40588732
Attachment #467437 - Attachment is obsolete: true
Attachment #470437 - Flags: review?(myk)
This is view of changes from last review:
http://github.com/Gozala/jetpack-sdk/compare/f320d0c9f265f607d6b42414d97fe752f0ed32ed...events@588732

Unfortunately it also contains merge diff with traits.
As I take another look at this, I'm starting to wonder about the motivation for making this change.  If I understand correctly, this is adding three additional ways to register an event listener to the ones we are already using in the SDK.  That sounds like an awful lot of ways to do the same thing.

I understand that these ways are compatible with various other event standards.  But how important is that really relative to having one simple, intuitive way to register an event?

Although I said in comment 2 that I thought this seems useful, I'm having second thoughts, because of the redundancy and complexity this introduces.  Unless the goal is to replace the current model with this one, what's the justification for introducing yet another way to register events?
I would rather say adding a way for adding / removing **multiple** event listeners in familiar fashion (two other shortcuts are nice for compatibility but that's it.). 

I do share your opinion that it's unnecessary complexity if you're dealing with one event listener only, but if your code is more complex then probably you have several independent actors that act on different type of events. It also could be that one actor works only once or only when chain of particular events happen. EventEmitter interface allows you to register listeners when you need them and unregister them when you need it. Of course all this can be done by implementing sort of a router listener that will be able to delegate to one / several actors, but then you will end up with code that depends on all of your actors and you will have to change your router every time you change any of the actors. To put it simply this interface is much more friendly for writing more complex programs containing several individual components, it's also the case where third party libraries can come handy.  

Let me quote few lines from W3C specs which pretty much expresses my point of view here:

> Multiple listeners. Instead of a single message processing function, the code
> here attaches multiple event listeners, each one performing a quick check to 
> see if it is relevant for the message. In this example it doesn't make much 
> difference, but if multiple authors wanted to collaborate using a single port 
> to communicate with a worker, it would allow for independent code instead of 
> changes having to all be made to a single event handling function.

> Registering event listeners in this way also allows you to unregister 
> specific listeners when you are done with them, as is done with the
> configure() method in this example.  

I'm also providing a link to an example:
http://www.w3.org/TR/workers/#shared-workers
Unsolicited opinion:  If the proposed event interface is better than what Jetpack currently uses, let's replace it.  If not, let's not.  But let's also not include more than one interface, which would require us to document, explain, and support each and would require devs stop and think which they'd like to learn and use (and why they have to make such a decision at all).  And let's certainly not include more than one interface if we plan to only ever speak about one of them.
(In reply to comment #7)
> Unsolicited opinion:  If the proposed event interface is better than what
> Jetpack currently uses, let's replace it.  If not, let's not.  But let's also
> not include more than one interface, which would require us to document,
> explain, and support each and would require devs stop and think which they'd
> like to learn and use (and why they have to make such a decision at all).  And
> let's certainly not include more than one interface if we plan to only ever
> speak about one of them.

I don't thing this interface better or worse then the one currently used by jetpack since it includes it, because well known DOM interface `EventTarget` allows setting events in both ways:

object.onevent = function() ....
object.addEventListener('event', function() ....

Firs rarely used but good for simple scenarios & second more common in real apps. I also do think that saying that it the same interface as it was in dom is far better then teaching new way no matter which one. 

Please also keep in mind that compatibility with DOM will allow users to use already well known libraries like jQuery for example which do provide maybe more familiar APIs for some devs for handling events.

Another nice thing about this implementation is that fully decoupled from anything else but it fully handles all event delegation. This allows you to write API that emits 'foo' & 'bar' events much easier:

Trait.compose({
  constructor: function Example() {
    this._listener('foo');
    this._listener('bar');
  }
}, require('events').EventEmitter);
> I would rather say adding a way for adding / removing **multiple** event
> listeners in familiar fashion (two other shortcuts are nice for compatibility
> but that's it.). 
>
> I do share your opinion that it's unnecessary complexity if you're dealing
> with one event listener only, but if your code is more complex then probably
> you have several independent actors that act on different type of events.

Sure, but the existing implementation already handles multiple listeners:

  var thing = Thing({
    onEvent: [function listener1() { ... }, function listener2() { ... }]
  });
  thing.onEvent.add([function listener3() { ... },
                     function listener4() { ... }]);


> I also do think that saying that it the same interface as it was in dom
> is far better then teaching new way no matter which one.

I disagree, because developers only have to learn the new interface once, while they have to use it many times.  If we can make a noticeably better one that isn't prohibitively hard to learn, we will save developers time, effort, and heartache.

That's the basic premise behind libraries like jQuery, and the popularity of such libraries suggests it is right for this project as well.


> Please also keep in mind that compatibility with DOM will allow users to use
> already well known libraries like jQuery for example which do provide maybe
> more familiar APIs for some devs for handling events.

Except that these aren't DOM objects, which is what jQuery is designed to wrap.  It's not clear whether jQuery even supports the manipulation of such objects.  But even if it did, our goal should be to create an interface that is itself as simple and ergonomic as possible, so people don't have to invent jQuery-like libraries to wrap it.


> Another nice thing about this implementation is that fully decoupled from
> anything else but it fully handles all event delegation. This allows you to
> write API that emits 'foo' & 'bar' events much easier:
>
> Trait.compose({
>   constructor: function Example() {
>     this._listener('foo');
>     this._listener('bar');
>   }
> }, require('events').EventEmitter);

Yes, this is great.  But we can accomplish this without adding a new interface.  We would just need to factor out the code for creating event callback properties into a Trait.


Note: I'm not opposed to aligning our interfaces to web standards in general; in fact, I'm in favor of doing so, especially where those interfaces are already optimal (or close enough), which is why I support changing ContentSymbiont to behave more like WebWorker over in bug 588737.

However, in this case, the existing interface is significantly better than the DOM interface.  And it still handles the common use cases we expect developers to have.  So we should stick with it.

Which leaves the question of whether we should add these interfaces to the existing one, to make it easier for developers who are familiar with DOM and other web interfaces to get started with addon development.  And I can imagine that there would be justification for doing so in certain cases.  In this particular case, I can't think of one, but I'm certainly open to hearing more and changing my mind.

In the meantime, though, I think the right thing to do here is to make a Trait for setting up an interface like the existing event callback properties.  Then, if at some point we identify specific common use cases that aren't satisfied by the existing interface, or we accumulate evidence that developers would be better off with a DOM-compatible API (instead of or in addition to the existing one), we can revisit the question.
I want to make clear that interface is not DOM specific since XMLHttpRequest, Worker, WebSocket they all implement it. 

> I disagree, because developers only have to learn the new interface once, while
> they have to use it many times.  If we can make a noticeably better one that
> isn't prohibitively hard to learn, we will save developers time, effort, and
> heartache.

It's not only about learning it's about rewriting things that had been already written
and then maintaining two versions. I do agree though that if we are making noticeably
better then it's not a big tradeoff. Unfortunately I disagree that it's better because:

1. Current implementation is suboptimal in terms of object instantiation speed and memory footprint. (It's not specific to the implementation but specific to the API being implemented)

2. Current implementation allows iteration over listeners which is not good in terms of security since callbacks may be misused by third parties. (It's also specific to the API since in cases with `object.onEvent` `onEvent` is independent object represents collection of listeners and it's complicated to make `object` only privileged construct with an access to items)   
3. `EventEmitter` provides intuitive and familiar interface in comparison to the current API that can be misleading in some scenarios.

Lets compare all the scenarios to see when `EventEmitter` seems more complicated:

- adding listener (one listener scenario):

  1. EventEmitter:

            object.onEvent = callback;
            // or
            object.addListener('event', callback);
		
  2. Jetpack API:

            object.onEvent = callback;
            // or
            object.onEvent.add(callback);

- removing listener (one listener scenario):

  1. EventEmitter:

            object.onEvent = null;
            // or
            object.removeListener('event', callback);
		
  2. Jetpack API:

            object.onEvent.remove(callback);

- Changing listener (one listener scenario):

  1. EventEmitter:

            object.onEvent = callback2;
		
  2. Jetpack API:

            object.onEvent.remove(callback1);
            object.onEvent.add(callback2);


- adding additional listeners:
  
 1. EventEmitter:

            object.addListener('event', callback2);
            object.addListener('event', callback3);

 2. Jetpack API:

            object.onEvent = callback2;
            object.onEvent = callback3;
            // or 
            object.onEvent = [callback2, callback3];

- removing event listeners

  1. Event Emitter:

            object.removeListener('event', callback2);
            object.removeListener('event', callback3);

  2. Jetpack API:
 
            for each (let listener in object.onEvent)
                object.onEvent.remove(listener);

            // or 

            object.onEvent.remove(callback1);
            object.onEvent.remove(callback2);
             
- Scenarios where current APIs is misleading:

  1. Manually calling listener: 
        
            object.onEvent = function(data) {
              object.cache.push(data);
            }

            object.onEvent(data); // oops not a function!

            // ok let my try this then

            object.onEvent[0](data); // oops not defined (can iterate but can't access)
            
  2. Unintentionally adding listener instead of changing it


            object.onEvent = function configure() {
              .........
              if (configured)
                 // will add listener instead of replaceing
                 object.onEvent = handleMessages;
              ..........
            }

  3. Unintentionally adding null as listener instead of removing:

            // will set null as a listener
            object.onEvent = null;


Functionality comparison:

1. `EventEmitter` does not exposes any event listeners except the one that explicitly was set as a property. This allows us / lib devs to use internal event listeners for better component coupling. Good examples of this can be found in the sidebar implementation and it's dependencies.

2. `EventEmitter` intensionally  does not exposes api for emitting events. This api is private which is important in terms of security. Emulating emitting events with current API is simple since all the listeners can be iterated and called.

3. While custom events are not our concerns they are very useful and most likely will be very useful for lid devs.
> I want to make clear that interface is not DOM specific since XMLHttpRequest,
> Worker, WebSocket they all implement it. 

Sure.


> It's not only about learning it's about rewriting things that had been already
> written and then maintaining two versions.

But both APIs have already been implemented elsewhere, so in neither case are we inventing a new API (although doing so would be perfectly legitimate if it were justified by the circumstances).  Nor will we be maintaining two versions if we only implement one, as is currently the case.

In fact, in this particular case it is the pro-EventEmitter position that is suggesting we either rewrite something that has already been written (our existing event model) or maintain two versions.

And although I am expressing skepticism, I'm actually open to both options (i.e. replacing our existing event model or augmenting it with EventEmitter), provided we have good reasons for doing so.


> 1. Current implementation is suboptimal in terms of object instantiation speed
> and memory footprint. (It's not specific to the implementation but specific to
> the API being implemented)

These differences aren't significant, given that the objects are small and are registered in small quantities.  Performance is important, and we should pay close attention to it, but we have to distinguish between important costs and trivial ones, and this falls into the latter camp.


> 2. Current implementation allows iteration over listeners which is not good in
> terms of security since callbacks may be misused by third parties. (It's also
> specific to the API since in cases with `object.onEvent` `onEvent` is
> independent object represents collection of listeners and it's complicated to
> make `object` only privileged construct with an access to items)   

You're right that we should prevent iteration over listeners, particularly on singletons shared by multiple modules.  But it is straightforward to do so using lexical closures.  Is it really that hard to do using Traits?


> 3. `EventEmitter` provides intuitive and familiar interface in comparison to
> the current API that can be misleading in some scenarios.
>
> Lets compare all the scenarios to see when `EventEmitter` seems more
> complicated:
>
> - adding listener (one listener scenario):
>
>   1. EventEmitter:
>
>             object.onEvent = callback;
>             // or
>             object.addListener('event', callback);
>
>   2. Jetpack API:
>
>             object.onEvent = callback;
>             // or
>             object.onEvent.add(callback);

The ability to set object.onEvent to a callback (or array of callbacks) in the current implementation is indeed problematic, primarily because it permits module A to remove a listener added by module B to an object shared with both modules by module C, and secondarily because one cannot then get the callback by evaluating object.onEvent, as one would expect to do with a property one sets to a value.

However, the solution to the problems (the primary one of which is also a problem in EventEmitter) is to disallow setting object.onEvent, which is bug 588250.

And this is a good example of why we cannot simply apply a standard developed for a different domain without carefully considering its ramifications in ours.  In the DOM, all scripts run in the same context and have the untrammeled ability to trample upon each others' states.  Scripts only avoid doing so by convention.

In the SDK, however, scripts run in separate contexts delineated by modules, and we are committed to preventing them from having the ability to alter each others' states in unexpected ways.

The SDK uses web technologies and targets web developers, so we should strive to be as much like the web as possible, but we have to be different where doing so makes sense for our particular problem domain.  Standards are a means to our ends, not the ends in and of themselves.


> - Scenarios where current APIs is misleading:
>
>   1. Manually calling listener: 
>   2. Unintentionally adding listener instead of changing it
>   3. Unintentionally adding null as listener instead of removing:

All of these will be fixed by bug 588250.


> Functionality comparison:
>
> 1. `EventEmitter` does not exposes any event listeners except the one that
> explicitly was set as a property. This allows us / lib devs to use internal
> event listeners for better component coupling. Good examples of this can be
> found in the sidebar implementation and it's dependencies.

This can be fixed in the current model by removing the ability to iterate event collections.


> 2. `EventEmitter` intensionally  does not exposes api for emitting events. This
> api is private which is important in terms of security. Emulating emitting
> events with current API is simple since all the listeners can be iterated and
> called.

This can be fixed in the current model by removing the ability to iterate event collections.


> 3. While custom events are not our concerns they are very useful and most
> likely will be very useful for lid devs.

That may be true, but we haven't yet identified any concrete use cases for them, and the primary locus of extensibility in the SDK lies elsewhere (in modules).

Note that it's possible to add EventEmitter-like functionality on top of the existing API, just as there are both event-specific and generic event registration methods in jQuery.  If at some point it turns out that it would be really useful to have an EventEmitter-like API in addition to the current one, we could certainly add it then.

However, at the moment I don't see the compelling reason to do so, much less to replace the existing model with it.
> > It's not only about learning it's about rewriting things that had been already
> > written and then maintaining two versions.
> 
> But both APIs have already been implemented elsewhere, so in neither case are
> we inventing a new API (although doing so would be perfectly legitimate if it
> were justified by the circumstances).  Nor will we be maintaining two versions
> if we only implement one, as is currently the case.
> 

What I meant is that there are lots of libraries / commonjs packages already, for example only npm node.js specific registry contains 280 different libraries same of them could've be used in jetpack without changes, but it won't be possible now since either will have to implement adapters or someone will have to maintain forks with a changes to the parts that deal with events. I have done it in the past and maintenance is a hell.    

> In fact, in this particular case it is the pro-EventEmitter position that is
> suggesting we either rewrite something that has already been written (our
> existing event model) or maintain two versions.
> 
> And although I am expressing skepticism, I'm actually open to both options
> (i.e. replacing our existing event model or augmenting it with EventEmitter),
> provided we have good reasons for doing so.
> 

Not sure I expressed it, but I am not in favor of having both version myself.

> 
> > 1. Current implementation is suboptimal in terms of object instantiation speed
> > and memory footprint. (It's not specific to the implementation but specific to
> > the API being implemented)
> 
> These differences aren't significant, given that the objects are small and are
> registered in small quantities.  Performance is important, and we should pay
> close attention to it, but we have to distinguish between important costs and
> trivial ones, and this falls into the latter camp.
> 

I think that amount of objects created is underestimated. With current design per event type there are:

1 event container object + 2 function ( add / remove), 1 array with listeners, 1 getter + 1 setter. There is also a runtime overhead since any access to events has to go through getter, but the worth is that nothing here can be shared either by prototype or any other way which multiplies this number per instance created. I'm not saying that this has huge performance impact on the desktop but on mobile it is & I'm thinking about Fennec.

> You're right that we should prevent iteration over listeners, particularly on
> singletons shared by multiple modules.  But it is straightforward to do so
> using lexical closures.  

Well in all the cases so far lexical closures where used for 1 level depth here it goes to the second where you're not only covering instance properties but also properties of a properties like add / remove functions, which will add certain complexity.

> Is it really that hard to do using Traits?

As mentioned above it has certain complexity and is suboptimal but I've been thinking of way to build this on top of current emitter which won't be that hard probably. Of course I'll have to convert all the public API's to private.  

> 
> The ability to set object.onEvent to a callback (or array of callbacks) in the
> current implementation is indeed problematic, primarily because it permits
> module A to remove a listener added by module B to an object shared with both
> modules by module C, and secondarily because one cannot then get the callback
> by evaluating object.onEvent, as one would expect to do with a property one
> sets to a value.
> 
> However, the solution to the problems (the primary one of which is also a
> problem in EventEmitter) is to disallow setting object.onEvent, which is bug
> 588250.
> 

BTW object.onEvent = function() .. is not part of the EventEmitter interface neither it's implemented that way. EventEmitter has internal convenience function that can be used to generate 'onEvent' like properties but it was added only later for better compatibility with current interface. There is no way to iterate over listener of EventEmitter neither it's possible to remove them unless you happened to have reference to a function you want to remove. 


> And this is a good example of why we cannot simply apply a standard developed
> for a different domain without carefully considering its ramifications in ours.
>  In the DOM, all scripts run in the same context and have the untrammeled
> ability to trample upon each others' states.  Scripts only avoid doing so by
> convention.
> 

BTW in dom does not exposes way of listing listeners and use of button.onClick is considered to be a bad practice, actually it's not part of the EventEmitter specification and kept only for backwards compatibility.

> > 3. While custom events are not our concerns they are very useful and most
> > likely will be very useful for lid devs.
> 
> That may be true, but we haven't yet identified any concrete use cases for
> them, and the primary locus of extensibility in the SDK lies elsewhere (in
> modules).
> 
> Note that it's possible to add EventEmitter-like functionality on top of the
> existing API, just as there are both event-specific and generic event
> registration methods in jQuery.  If at some point it turns out that it would be
> really useful to have an EventEmitter-like API in addition to the current one,
> we could certainly add it then.
> 
> However, at the moment I don't see the compelling reason to do so, much less to
> replace the existing model with it.

I don't think it's that easy to implement EventEmitter on top of current design since current design is very tide to the public API. If we will implement EventEmitter on top of current implementation it will mean that emitting custom events will mutate target object by generating onEvent type properties. 

That being said I do think that performance + familiarity overweight syntax sugar of   

object.onEvent.add(function..   VS   object.addListener('event', function..

we actually can stick to original name I had and it will become even less verbose then current API:

object.on('event', function...

Supporting object.onEvent = function() .. is not necessary besides it's not part of the interface & is considered to be a bad practice on the web anyway.  


If are still absolutely sure that current API is the way to go I would suggest modifying EventEmitter by moving all the public API to private and building current jetpack event API on top.
I've been using node.js for some simple experiments over the past few weeks, and I really like node's EventEmitter model for a few reasons:

* It leverages developers' existing understanding of DOM APIs as much as possible, while making the most common use cases significantly easier to use: the changing of the method 'addEventListener()' to 'on()' is an enormous improvement given how often the method is invoked, and it reads nicely.

* Because node's EventEmitter API so similar to DOM APIs, it probably wouldn't be difficult to standardize on in the future.

* Node's EventEmitter API is simple and straightforward enough that it would be really easy to mock/stub out for unit testing or any other purpose.

* node.js has a rapidly growing amount of developer mindshare, especially amongst bleeding-edge innovator types; providing an EventEmitter API would not only keep the barrier for web developers low, but it'd also lower the barrier for node developers to join the community.

While the current collection-based event-handling mechanism has a lot of cool developer-ergonomic-ness to it, there's a few things I find counterintuitive about it at first glance. For instance, for someone who isn't already familiar with them, reading this raises some alarms:

  pb.onStart.add(onStartCallback2);
  pb.onStart = [onStartCallback, onStartCallback2];
  pb.onStart.remove(onStartCallback2);

The first line seems to imply that 'onStart' is some sort of custom object, while the second line seems to be changing the type of 'onStart' to an Array, while the third line seems to be calling Array.remove(), which doesn't actually exist.

To some extent, collections add another kind of "primitive type" to the set of low-level tools a developer needs to keep in their head; it looks and feels a lot like an Array, but behaves just differently enough that it could potentially trip up newcomers--for instance, does a collection support Array methods like forEach()? If not, why not? OTOH, node's EventEmitter feels like a minor evolution of the DOM EventTarget interface--few enough changes that you can remember them easily, while also making the interface easier and more powerful to use.

All that said, I haven't yet looked at what Irakli's version of EventEmitter is like, nor have I thoroughly read all the comments on this bug. So some of these comments may be totally invalid in this context... Sorry about that. :/
This is actually implementation of EventEmitter API from node.js. Only difference is that list of listeners is not exposed into a public API and few alias methods are added for compatibility with dom (I don't mind removing them though). Even more tests are ported from the node.js's EventEmitter tests, so it's really compatible :)
Not all these arguments are convincing (and Atul, yes, there is some backstory that would alleviate your concerns about the existing model, namely bug 588250).

Nevertheless, the weight of the evidence presented suggests that we would be a bit better off employing node.js's model, and although I am concerned about the cost of switching, I'm willing to give it a shot, so I have changed my mind and agree to switch our event model from the current one to the node.js model as implemented in this EventEmitter implementation.

Switching means replacing the existing model with the new one, not keeping the old one around.  And the implementation shouldn't include compatibility methods; there should just be one good way to register an event.

Also note that we need to do this for all high-level modules with events before shipping a beta, since it's a pretty significant interface change.  Given that we are planning to make 0.8 our first beta, that means the work should get done during the 0.8 cycle.  I'll file a separate bug on that.
(In reply to comment #15)
> Also note that we need to do this for all high-level modules with events before
> shipping a beta, since it's a pretty significant interface change.  Given that
> we are planning to make 0.8 our first beta, that means the work should get done
> during the 0.8 cycle.  I'll file a separate bug on that.

Filed as bug 593737.
Great!!

So if get it correctly in order to get this patch accepted I have to:

1. remove alias methods: addEventListener, removeEventListener, addListener
2. remove internal convenience method `_listener` that allows to add `onEvent` setter / getters on instance.
3. have to keep public methods: on, removeListener
4. update docs accordingly
Comment on attachment 470437 [details] [diff] [review]
Addressing review commets

(In reply to comment #17)
> Great!!
> 
> So if get it correctly in order to get this patch accepted I have to:
> 
> 1. remove alias methods: addEventListener, removeEventListener, addListener
> 2. remove internal convenience method `_listener` that allows to add `onEvent`
> setter / getters on instance.
> 3. have to keep public methods: on, removeListener
> 4. update docs accordingly

Yes, that's correct.
Attachment #470437 - Flags: review?(myk)
Attachment #473045 - Attachment is patch: true
Attachment #473045 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 473045 [details] [diff] [review]
updated patch

This is looking better, but there are still a few issues.  Review comments are in the commit page <http://github.com/Gozala/jetpack-sdk/commit/0f62d87df9d68687ae98511395e7de5ea47e35b2>.
Attachment #473045 - Flags: review?(myk) → review-
Comment on attachment 473466 [details] [diff] [review]
Addressing review commets

Only a couple issues left.
Attachment #473466 - Flags: review?(myk) → review-
Comment on attachment 474006 [details] [diff] [review]
Addressing review commets

Just one more issue! (and a couple nits)
Attachment #474006 - Flags: review?(myk) → review-

        Attachment #474006 -
        Attachment is obsolete: true

        Attachment #474577 -
        Flags: review?(myk)

    
    

    
  
Full diff: http://github.com/Gozala/jetpack-sdk/commit/80961afc0d341035d24d7ba34be1d41b00929601

BTW I also experimented with pull request which may be a better option for review comments:
http://github.com/Gozala/jetpack-sdk/pull/2

    
    

    
  
Comment on attachment 474577 [details] [diff] [review]
Addressing review commets

Looks great, r=myk!

        Attachment #474577 -
        Flags: review?(myk) → review+

    
    

    
  
Fixed by changeset https://hg.mozilla.org/labs/jetpack-sdk/rev/6e887c45cc9d.
Status: NEW → RESOLVED
Closed: 14 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: