598978 - change "simple-storage" module to use EventEmitter event registration model

Status: RESOLVED FIXED

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

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: V1

Pull request: http://github.com/Gozala/jetpack-sdk/pull/10
Commit: http://github.com/Gozala/jetpack-sdk/commit/744956d06c81cd4ced107212964b23c0a3d022ff

Comment 2

[Drew Willcoxon :adw]

Comment on attachment 479372 [details] [diff] [review]
V1

Thanks Irakli.

I'm sure you know already, but this patch needs to be updated for the jetpack-core-to-addon-kit transition.

>diff --git a/packages/jetpack-core/docs/simple-storage.md b/packages/jetpack-core/docs/simple-storage.md

>-To listen for quota notifications, register a listener with the module's
>-`onOverQuota` collection:
>+To listen for quota notifications, register a listener for 'overQuota' events:

Let's use `"overQuota"` here (i.e., double quotes inside backticks) so that it's clear it's both a string and code-related.

>-    simpleStorage.onOverQuota.add(myOnOverQuotaListener);
>+    simpleStorage.on('overQuota', myOnOverQuotaListener);

The simple-storage documentation and code uses double quotes for strings, so please continue to do so.

> <api name="quotaUsage">
> @property {number}
>   A number in the range [0, Infinity) that indicates the percentage of quota
>@@ -142,3 +135,4 @@ Reference
>   is within quota.  A value greater than 1 indicates that the storage exceeds
>   quota.
> </api>
>+

Is this blank line at the end of the file really necessary?  Newlines at end of file are good and proper, but two newlines seem unnecessary, so unless there is a good reason please remove this blank line.

>diff --git a/packages/jetpack-core/lib/simple-storage.js b/packages/jetpack-core/lib/simple-storage.js

>+const { EventEmitter } = require('events');

To reiterate, use double quotes for strings.

Also, I think you can remove the require("collection"), right?

>+    // log uncaught exceptions thrown by listeners

If a comment is a full sentence, capitalize and punctuate it.

>+    this.on('error', console.error);

Shouldn't this be console.exception so clients get a full stack trace and the other nice things console.exception provides but console.error does not?

>+    this.writeTimer = timer.setInterval(
>+      this.write.bind(this), this.writePeriod
>+    );

Format like this:

    this.writeTimer = timer.setInterval(this.write.bind(this),
                                        this.writePeriod);

>@@ -140,10 +138,8 @@ JsonStore.prototype = {
>   // If the store is under quota, writes the root to the backing file.
>   // Otherwise quota observers are notified and nothing is written.
>   write: function JsonStore_write() {
>-    if (this.quotaUsage > 1) {
>-      let arr = this._copyAndWrapObservers(this.onOverQuota);
>-      this._notifyObserversArray(arr);
>-    }
>+    if (this.quotaUsage > 1)
>+      this._emit('overQuota', exports);

This works (I presume), but it ties JsonStore to the simple-storage module, and I'd like to keep it isolated so that it can be easily pulled out of the module if need be.  So can you instead have the manager do the emitting for the module?  I'm OK with the JsonStore emitting "overQuota" if that helps your implementation; I just don't want it doing so on behalf of the module.

>     // Clear the collections so they don't keep references to client callbacks.
>-    this.onOverQuota = [];
>-    this.onWrite = [];
>+    this._removeAllListeners('overQuota');

This comment is no longer accurate, if only because it mentions "collections".

>     // Finally, write.
>-    const self = this;
>     let stream = file.open(this.filename, "w");
>     try {
>       stream.writeAsync(JSON.stringify(this.root), function writeAsync(err) {
>         if (err)
>-          console.error("Error writing simple storage file: " + self.filename);
>+          console.error("Error writing simple storage file: " + this.filename);
>         else
>-          self._notifyObserversArray(obsArray);
>-      });
>+          this._emit('write', exports);

See my jsonStore.onWrite test comment below -- since the writing is being done by the JsonStore object specifically, it would be better to pass that object to write listeners, rather than the module, so could you make that change?

>+        // Maybe unload happened before callback was called
>+        if (null == this.writeTimer) {
>+          this._removeAllListeners('write');
>+          this._removeAllListeners('error');
>+        }

I don't understand why these listeners are removed here.  Shouldn't they be removed in unload?  And if they are, and if unload is called before this callback is called, shouldn't _emit just be a no-op since no listeners are registered?

>@@ -267,13 +248,14 @@ let manager = {
>   // Must be called before use.
>   init: function manager_init() {
>     let fname = this.filename;
>-    this.jsonStore = new JsonStore({
>+    let jsonStore = this.jsonStore = new JsonStore({

This isn't really necessary is it?  Can't you just reference this.jsonStore in subsequent lines?

>diff --git a/packages/jetpack-core/tests/test-simple-storage.js b/packages/jetpack-core/tests/test-simple-storage.js

>@@ -58,15 +59,15 @@ exports.testSetGet = function (test) {
>   // Load the module once, set a value.
>   let loader = newLoader(test);
>   let ss = loader.require("simple-storage");
>-  manager(loader).jsonStore.onWrite.add(function () {
>-    test.assertEqual(this, ss, "|this| should be simple storage module");
>+  manager(loader).jsonStore.on('write', function (storage) {
>+    test.assertEqual(storage, ss, "storage should be simple storage module");

To reiterate, use double quotes for strings.

Also, I'm not sure why I wanted to test that `this` is the simple storage module...  jsonStore.onWrite is not part of the API, so it's not important to test that.  So could you please either just remove those assertions, or change them to assert that the arg passed to the listener is the JsonStore object, as I mentioned above?

>-  ss.onOverQuota = function () {
>-    test.pass("onOverQuota callback called as expected");
>+  ss.on('overQuota', function (storage) {
>+    test.pass("overQuota was emitted as expected");

In addition to this test.pass, please assert that storage is the simple storage module.

>@@ -312,3 +313,4 @@ function setGetRootError(test, val, msg) {
>   test.assertRaises(function () ss.storage = val, pred, msg);
>   loader.unload();
> }
>+

Remove the blank line here too.

Comment 3

[Drew Willcoxon :adw]

Also, the doc needs to mention that the module is passed to overQuota listeners.

Comment 4

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

Note: per our conventions for event names passed to the "on" method, the "overQuota" event type should be spelled "OverQuota".

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #2)
> Comment on attachment 479372 [details] [diff] [review]
> V1
> 
> Thanks Irakli.
> 
> I'm sure you know already, but this patch needs to be updated for the
> jetpack-core-to-addon-kit transition.
> 
> >diff --git a/packages/jetpack-core/docs/simple-storage.md b/packages/jetpack-core/docs/simple-storage.md
> 
> >-To listen for quota notifications, register a listener with the module's
> >-`onOverQuota` collection:
> >+To listen for quota notifications, register a listener for 'overQuota' events:
> 
> Let's use `"overQuota"` here (i.e., double quotes inside backticks) so that
> it's clear it's both a string and code-related.
> 
> >-    simpleStorage.onOverQuota.add(myOnOverQuotaListener);
> >+    simpleStorage.on('overQuota', myOnOverQuotaListener);
> 
> The simple-storage documentation and code uses double quotes for strings, so
> please continue to do so.
> 

http://github.com/Gozala/jetpack-sdk/commit/57499a096146637f032b4d6b2f3e9ec9d246563d

> > <api name="quotaUsage">
> > @property {number}
> >   A number in the range [0, Infinity) that indicates the percentage of quota
> >@@ -142,3 +135,4 @@ Reference
> >   is within quota.  A value greater than 1 indicates that the storage exceeds
> >   quota.
> > </api>
> >+
> 
> Is this blank line at the end of the file really necessary?  Newlines at end of
> file are good and proper, but two newlines seem unnecessary, so unless there is
> a good reason please remove this blank line.

Sorry for that I'm still fairly new to vim and it seems that for some reason it doesn't displays last line if it's blank :)

http://github.com/Gozala/jetpack-sdk/commit/b48688ad48fac40856f47874b67cf6fa8e496824 

> 
> >diff --git a/packages/jetpack-core/lib/simple-storage.js b/packages/jetpack-core/lib/simple-storage.js
> 
> >+const { EventEmitter } = require('events');
> 
> To reiterate, use double quotes for strings.
> 
> Also, I think you can remove the require("collection"), right?

http://github.com/Gozala/jetpack-sdk/commit/d0b68bca0085db9da7801c9cc4110ade236d6445

> 
> >+    // log uncaught exceptions thrown by listeners
> 
> If a comment is a full sentence, capitalize and punctuate it.
> 
> >+    this.on('error', console.error);
> 
> Shouldn't this be console.exception so clients get a full stack trace and the
> other nice things console.exception provides but console.error does not?
> 

http://github.com/Gozala/jetpack-sdk/commit/57a0a532ab9f064bbf7ab0294653f08b6a8ca6ef

> >+    this.writeTimer = timer.setInterval(
> >+      this.write.bind(this), this.writePeriod
> >+    );
> 
> Format like this:
> 
>     this.writeTimer = timer.setInterval(this.write.bind(this),
>                                         this.writePeriod);
> 

http://github.com/Gozala/jetpack-sdk/commit/b4c89fe96fffdec32c1ae2aac9f3331163ce30b7

> >@@ -140,10 +138,8 @@ JsonStore.prototype = {
> >   // If the store is under quota, writes the root to the backing file.
> >   // Otherwise quota observers are notified and nothing is written.
> >   write: function JsonStore_write() {
> >-    if (this.quotaUsage > 1) {
> >-      let arr = this._copyAndWrapObservers(this.onOverQuota);
> >-      this._notifyObserversArray(arr);
> >-    }
> >+    if (this.quotaUsage > 1)
> >+      this._emit('overQuota', exports);
> 
> This works (I presume), but it ties JsonStore to the simple-storage module, and
> I'd like to keep it isolated so that it can be easily pulled out of the module
> if need be.  So can you instead have the manager do the emitting for the
> module?  I'm OK with the JsonStore emitting "overQuota" if that helps your
> implementation; I just don't want it doing so on behalf of the module.

http://github.com/Gozala/jetpack-sdk/commit/62a535504cb3c445007a75d1664a3629e4fb8bdc

> 
> >     // Clear the collections so they don't keep references to client callbacks.
> >-    this.onOverQuota = [];
> >-    this.onWrite = [];
> >+    this._removeAllListeners('overQuota');
> 
> This comment is no longer accurate, if only because it mentions "collections".
> 

http://github.com/Gozala/jetpack-sdk/commit/fe2c3b020a88bb2e01b16a84651c786db516a5a1

> >     // Finally, write.
> >-    const self = this;
> >     let stream = file.open(this.filename, "w");
> >     try {
> >       stream.writeAsync(JSON.stringify(this.root), function writeAsync(err) {
> >         if (err)
> >-          console.error("Error writing simple storage file: " + self.filename);
> >+          console.error("Error writing simple storage file: " + this.filename);
> >         else
> >-          self._notifyObserversArray(obsArray);
> >-      });
> >+          this._emit('write', exports);
> 
> See my jsonStore.onWrite test comment below -- since the writing is being done
> by the JsonStore object specifically, it would be better to pass that object to
> write listeners, rather than the module, so could you make that change?
> 
> >+        // Maybe unload happened before callback was called
> >+        if (null == this.writeTimer) {
> >+          this._removeAllListeners('write');
> >+          this._removeAllListeners('error');
> >+        }
> 
> I don't understand why these listeners are removed here.  Shouldn't they be
> removed in unload?  And if they are, and if unload is called before this
> callback is called, shouldn't _emit just be a no-op since no listeners are
> registered?

Well in this case even if unload will happen before `writeAsync` is called, consumers will still be notified if write was successful. I don't really know how useful is this, but kept it the way original implementation was doing:

-    // Before we leave this function, copy observers into an array, because
-    // they're removed on unload.
-    let obsArray = this._copyAndWrapObservers(this.onWrite);

As you can see listeners where copied there for 

-          self._notifyObserversArray(obsArray);

would've called listeners even after unload.

That being said I don't mind changing this it certainly will make code more readable. I'll keep implementation as is before I'll get your feedback on this. 

> 
> >@@ -267,13 +248,14 @@ let manager = {
> >   // Must be called before use.
> >   init: function manager_init() {
> >     let fname = this.filename;
> >-    this.jsonStore = new JsonStore({
> >+    let jsonStore = this.jsonStore = new JsonStore({
> 
> This isn't really necessary is it?  Can't you just reference this.jsonStore in
> subsequent lines?
> 

Yes it is not necessary but access to the local variables is faster then access to object properties. I realize that this wont yield any meaningful performance improvement but if you don't mind it I'll prefer to keep it this way.  

> >diff --git a/packages/jetpack-core/tests/test-simple-storage.js b/packages/jetpack-core/tests/test-simple-storage.js
> 
> >@@ -58,15 +59,15 @@ exports.testSetGet = function (test) {
> >   // Load the module once, set a value.
> >   let loader = newLoader(test);
> >   let ss = loader.require("simple-storage");
> >-  manager(loader).jsonStore.onWrite.add(function () {
> >-    test.assertEqual(this, ss, "|this| should be simple storage module");
> >+  manager(loader).jsonStore.on('write', function (storage) {
> >+    test.assertEqual(storage, ss, "storage should be simple storage module");
> 
> To reiterate, use double quotes for strings.
> 
> Also, I'm not sure why I wanted to test that `this` is the simple storage
> module...  jsonStore.onWrite is not part of the API, so it's not important to
> test that.  So could you please either just remove those assertions, or change
> them to assert that the arg passed to the listener is the JsonStore object, as
> I mentioned above?

I'm not sure I do understand what you want me to do. Above you mentioned that you don't like that `JsonStore` emits events with an argument that is the module, so I reverted it back to the approach that was used in original implementation, where manager was passing a module to the `JsonStore` and it was stored as `observersThisArg` property. Property was later passed to the callbacks. I can't assert arg against instance of JsonStore since it will fail and that's because it's not passed to the callback nor it should be since that would expose JsonStore's private API.

Can you please rephrase what you want me to do on this one if that's still a case.
  
> 
> >-  ss.onOverQuota = function () {
> >-    test.pass("onOverQuota callback called as expected");
> >+  ss.on('overQuota', function (storage) {
> >+    test.pass("overQuota was emitted as expected");
> 
> In addition to this test.pass, please assert that storage is the simple storage
> module.
> 
> >@@ -312,3 +313,4 @@ function setGetRootError(test, val, msg) {
> >   test.assertRaises(function () ss.storage = val, pred, msg);
> >   loader.unload();
> > }
> >+
> 
> Remove the blank line here too.

Comment 6

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #4)
> Note: per our conventions for event names passed to the "on" method, the
> "overQuota" event type should be spelled "OverQuota".

http://github.com/Gozala/jetpack-sdk/commit/687275958d010a90fd104fae6e255c7c442f6f57

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #3)
> Also, the doc needs to mention that the module is passed to overQuota
> listeners.

http://github.com/Gozala/jetpack-sdk/commit/50a650a0447e715ecefaf53d03efa89289a16c14

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v2

Complete pull request: http://github.com/Gozala/jetpack-sdk/pull/12

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v3

Sorry attached wrong patch previously

Comment 10

[Drew Willcoxon :adw]

Comment on attachment 481244 [details] [diff] [review]
v3

> Well in this case even if unload will happen before `writeAsync` is called,
> consumers will still be notified if write was successful. I don't really know
> how useful is this, but kept it the way original implementation was doing:

Oh right, thanks.  The reason for the write notification is so that the test can do its business.  It needs to know when the write completes, but the write doesn't complete until after the unload finishes.  So unless I'm wrong, I think what the implementation is currently doing is necessary.

But, you can't assume that an unload will always be followed by a successful write -- the store may be over quota or empty with the file not yet on disk, in which cases nothing is written at all, or some error might occur while writing -- so I think you need to do what the current implementation does and really remove all listeners on unload yet also notify them when the write completes.

> Yes it is not necessary but access to the local variables is faster then access
> to object properties. I realize that this wont yield any meaningful performance
> improvement but if you don't mind it I'll prefer to keep it this way.

Hmm... :)  I think complicating the code even slightly for no meaningful performance improvement is not a good trade-off, so please don't do it.

> > Also, I'm not sure why I wanted to test that `this` is the simple storage
> > module...  jsonStore.onWrite is not part of the API, so it's not important to
> > test that.  So could you please either just remove those assertions, or change
> > them to assert that the arg passed to the listener is the JsonStore object, as
> > I mentioned above?
> 
> I'm not sure I do understand what you want me to do. Above you mentioned that
> you don't like that `JsonStore` emits events with an argument that is the
> module, so I reverted it back to the approach that was used in original
> implementation, where manager was passing a module to the `JsonStore` and it
> was stored as `observersThisArg` property. Property was later passed to the
> callbacks. I can't assert arg against instance of JsonStore since it will fail
> and that's because it's not passed to the callback nor it should be since that
> would expose JsonStore's private API.

Since the write callback is not part of the simple-storage API (unlike OverQuota), and it's exposed only by the JsonStore object itself (which the test can only access via a loader trick), it's OK if it re-exposes the private JsonStore object.

To be totally correct -- if the JsonStore were its own module with its own public API -- the write listener should be passed the JsonStore object that did the writing, don't you think?  But at the same time, since the write callback isn't part of the simple-storage API, it's not important to test its argument and therefore the assertions should be removed.  Does that make sense?

Also, per Myk's event name conventions, we should call it "Write" instead of "write".

>diff --git a/packages/addon-kit/docs/simple-storage.md b/packages/addon-kit/docs/simple-storage.md

>-To listen for quota notifications, register a listener with the module's
>-`onOverQuota` collection:
>+To listen for quota notifications, register a listener for `"OverQuota"` events
>+that will be called with an argument that is a `"simple-storage"` module.

To listen for quota notifications, register a listener for `"OverQuota"` events.
It will be called when your storage goes over quota.

    function myOnOverQuotaListener(simpleStorage) {
      console.log("Uh oh.");
    }
    simpleStorage.on("OverQuota", myOnOverQuotaListener);

Note that the listener is passed a reference to the simple storage module.

>diff --git a/packages/addon-kit/lib/simple-storage.js b/packages/addon-kit/lib/simple-storage.js

>+const JsonStore = EventEmitter.compose({
>+  constructor: function JsonStore(options) {
>+    this.filename = options.filename;
>+    this.quota = options.quota;
>+    this.writePeriod = options.writePeriod;
>+    this.observersThisArg = options.observersThisArg;
>+    // Log uncaught exceptions thrown by listeners.
>+    this.on("error", console.exception);

observersThisArg should be renamed to overQuotaListenersArg or something.  Also, please add a blank line above the comment for readability.

Actually, observersThisArg is a kludge, especially now that we have nice event emitters.  So really, the JsonStore should pass itself to OverQuota listeners, just like it should for Write listeners.  The manager should register an OverQuota listener on its jsonStore instance and then dispatch events to the module.

Comment 11

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

(In reply to comment #10)
> Also, per Myk's event name conventions, we should call it "Write" instead of
> "write".

Erm, one-word events should actually be lower-case.  It's just those with two or more words that should be CamelCase. :-)  Thus "write" but "OverQuota".

Comment 12

[Drew Willcoxon :adw]

Thanks for the clarification, but that seems inconsistent.  Why capitalize the first letter if the name contains more than one "word" but not capitalize otherwise?

Comment 13

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

(In reply to comment #12)
> Thanks for the clarification, but that seems inconsistent.  Why capitalize the
> first letter if the name contains more than one "word" but not capitalize
> otherwise?

The purpose of the capitalization is to make type names containing multiple words easier to read by distinguishing the words from each other, which doesn't apply to single word names.

Lower-casing single word names is also consistent with DOM events <http://en.wikipedia.org/wiki/DOM_events>, while capitalizing multiple word names is partly consistent (some such DOM events use capitalization, while others don't).

Comment 14

[Drew Willcoxon :adw]

(In reply to comment #13)
> The purpose of the capitalization is to make type names containing multiple
> words easier to read by distinguishing the words from each other, which doesn't
> apply to single word names.

I agree that inter-word caps have that purpose -- and that's all that's required for camel case -- but why cap the first letter?  Or less strictly, why cap it sometimes and sometimes not?

> Lower-casing single word names is also consistent with DOM events
> <http://en.wikipedia.org/wiki/DOM_events>, while capitalizing multiple word
> names is partly consistent (some such DOM events use capitalization, while
> others don't).

DOM-related events are a hodgepodge often proposed, implemented, and de-facto standardized by different browser vendors over periods of many years.  We shouldn't try to emulate that inconsistency.

Comment 15

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

(In reply to comment #14)
> I agree that inter-word caps have that purpose -- and that's all that's
> required for camel case -- but why cap the first letter?  Or less strictly, why
> cap it sometimes and sometimes not?

The primary reason for capping the first letter of multi-word names is that it is consistent with capitalization in DOM events (to the extent that such names are capitalized).  It also has the corollary benefit that it doesn't change between on() argument ("TypeName") and on* property ("onTypeName") forms.

The primary reason for not capping the first letter of uni-word names is also that it is consistent with DOM events.  It is also easier to type (and, I suspect, to read).  On the other hand, it does change between on() argument and on* property forms.


> DOM-related events are a hodgepodge often proposed, implemented, and de-facto
> standardized by different browser vendors over periods of many years.  We
> shouldn't try to emulate that inconsistency.

Agreed, but web developers are familiar with them, so it's worth being similar to them while remaining as consistent as possible.

There is no perfect solution, as all approaches involve tradeoffs, but my sense is that the following guidelines achieve the best balance of simplicity, familiarity, and consistency:

1. single word names
   a. lower case when passed to on(),
      i.e. on("write", function() {})
   b. capitalized when declared via on*,
      i.e. onWrite: function() {}

2. multiple word names
   a. all words capitalized when passed to on(),
      i.e. on("OverQuota", function() {})
   b. all words capitalized when declared via on*,
      i.e. onOverQuota: function() {}

Comment 16

[Drew Willcoxon :adw]

The only DOM events that are camel cased are the DOM* ones, DOMContentLoaded et al.  What about the more widely known mousedown, mouseover, mouseout, mousemove, dblclick, keydown, keyup, keypress, etc.?  I'm not arguing that multiword events should be all lowercase, only that your argument about consistency with DOM event names is not so strong.

And if it's important to match onFoo with on("Foo"), that's an argument for capitalizing first letters regardless of number of words.  The number of words is totally irrelevant for that point.

I'm not getting anywhere, so I'll drop it now.

Comment 17

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: v4

Comment 18

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

link to pull request
http://github.com/mozillalabs/jetpack-sdk/pull/13#files_bucket

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 482241 [details] [diff] [review]
v4

Thanks Irakli, this looks good.

> JsonStore.prototype = {
>+  writeTimer: null,

There's no need to set the prototype's writeTimer to null.  Are you just trying to tell the reader that instances have a writeTimer member?  If so, I think this is misleading, since it's not the prototype that has the member.  And the fact that writeTimer is set directly on the instance in the constructor is enough.

>   // Must be called before use.
>-  init: function manager_init() {
>+  constructor: function manager_constructor() {
>+    // log unhandled errors
>+    this.on('error', console.exception)

Please capitalize and punctuate that comment, use double quotes for the "error" string, and end the last line in a semi-colon.

More importantly, don't you need to

  this.on("error", console.exception.bind(console));

or

  this.on("error", function (err) console.exception(err));

to ensure that exception() is called on console?

Please make the above changes before landing.

Comment 20

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #19)
> Comment on attachment 482241 [details] [diff] [review]
> v4
> 
> Thanks Irakli, this looks good.
> 
> > JsonStore.prototype = {
> >+  writeTimer: null,
> 
> There's no need to set the prototype's writeTimer to null.  Are you just trying
> to tell the reader that instances have a writeTimer member?  If so, I think
> this is misleading, since it's not the prototype that has the member.  And the
> fact that writeTimer is set directly on the instance in the constructor is
> enough.
> 

1. I just find it useful cause it allows you to see all the instance properties by looking at prototype.
2. Not sure how jagermonkey handles that but V8 for example was performing better if properties where declared in the prototype since shadow classes where generated out of them in case where additional properties where added constructor instantiation was slower cause that caused generation of the second shadow class.        

That being said, removed it. It's just a habits (the same with local variables VS object property access) you develop after working with low end devices running js.
   
 

> >   // Must be called before use.
> >-  init: function manager_init() {
> >+  constructor: function manager_constructor() {
> >+    // log unhandled errors
> >+    this.on('error', console.exception)
> 
> Please capitalize and punctuate that comment, use double quotes for the "error"
> string, and end the last line in a semi-colon.
> 
> More importantly, don't you need to
> 
>   this.on("error", console.exception.bind(console));
> 
> or
> 
>   this.on("error", function (err) console.exception(err));
> 
> to ensure that exception() is called on console?
> 
> Please make the above changes before landing.

Comment 21

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to comment #19)
> Comment on attachment 482241 [details] [diff] [review]
> v4
> 
> Thanks Irakli, this looks good.
> 
> > JsonStore.prototype = {
> >+  writeTimer: null,
> 
> There's no need to set the prototype's writeTimer to null.  Are you just trying
> to tell the reader that instances have a writeTimer member?  If so, I think
> this is misleading, since it's not the prototype that has the member.  And the
> fact that writeTimer is set directly on the instance in the constructor is
> enough.
> 
> >   // Must be called before use.
> >-  init: function manager_init() {
> >+  constructor: function manager_constructor() {
> >+    // log unhandled errors
> >+    this.on('error', console.exception)
> 
> Please capitalize and punctuate that comment, use double quotes for the "error"
> string, and end the last line in a semi-colon.
> 
> More importantly, don't you need to
> 
>   this.on("error", console.exception.bind(console));
> 
> or
> 
>   this.on("error", function (err) console.exception(err));
> 
> to ensure that exception() is called on console?
> 

Thanks that was a good catch! For some reason I assumed that console had all it's methods bounded.


> Please make the above changes before landing.

Comment 22

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