727967 - Convert Dict.jsm from/to JSON

Status: RESOLVED FIXED

(Toolkit :: General, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

Dict.jsm is quite useful to represent key/value pairs such as preferences. However, there is no good way of saving it to disk/restoring it from disk.

Implementing Dict.prototype.toJSON and Dict.ofJSON would essentially solve the issue.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

It's current toString is JSON-compatible, isn't it? There may be some edge cases it doesn't handle. It's also somewhat inefficient, but I guess there's no easy way around that given the key prefix.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Indeed, |toString| should be JSON-compatible, but |JSON.stringify| will not use it. Rather, |JSON.stringify| returns a string that cannot be properly parsed back to a |Dict| object.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

So what is this bug about, then? I don't think it's feasible to change JSON.parse/stringify to properly round-trip Dict objects. Dict.jsm users must use 

json = JSON.stringify(JSON.parse(dict.toString()))
and
dict = new Dict(JSON.parse(json))

Comment 4

[David Teller [:Yoric] - still alive but not very active]

I hope you agree that it would be nicer and less error-prone to simply write

json = JSON.stringify(dict)
dict = Dict.ofJSON(json)

To fix the behavior of |JSON.stringify|, we simply need to implement |json.prototype.toJSON|, with the exact same behavior as the current |json.prototype.toString|. |JSON.stringify| will use it.

To implement |Dict.ofJSON|, we simply need a one-liner returning |new Dict(JSON.parse(json))|.

Note: I did not mark this "good first bug" by accident. This can be done in 5 minutes + testsuite.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Ah, I didn't know that the toJSON hook already existed. Nevermind, then!

(I think it would be nicer to change the constructor to assume a string argument is JSON, rather than adding a static ofJSON method.)

Comment 6

[Kevin Huynh]

Hi David, 

Can I be assigned to this bug?  Thanks

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Kevin Huynh from comment #6)
> Hi David, 
> 
> Can I be assigned to this bug?  Thanks

With pleasure. Feel free to get in touch with me on IRC (irc.mozilla.org, channel #introduction ) if you have any question.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Any news about this? Or should I set the bug as unassigned?

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Setting the bug as unassigned.

Comment 10

[Abhishek Potnis [:avp]]

Hi Yorin,
I have started working on the Dict.jsm file. I have edited the constructor and am pasting the constructor here.

function Dict(aInitial) {
  if (aInitial === undefined)
    aInitial = {};

    //my code begins here
    if(typeof aInitial =="string")//passed object is a string
    {
    	//code to convert into JSON 
    	aInitial = JSON.parse(aInitial);
    	}
    //my code ends here

  var items = {}, count = 0;
  // That we don't look up the prototype chain is guaranteed by Iterator.
  for (var [key, val] in Iterator(aInitial)) {
    items[convert(key)] = val;
    count++;
  }
  this._state = {count: count, items: items};
  return Object.freeze(this);
} 

is this part correct ? I tested it using xpcshell by pasting the constructor in the console.
is that the right way to test the code or is there any other way through which I can test my code ?

Also I am confused about the toString and toJSON part that you explained to me on IRC.Could you please explain it to me again.

Thanks.

Comment 11

[Abhishek Potnis [:avp]]

extremely sorry Yoric, I misspelled your name in the previous comment.
really very sorry.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(In reply to Abhishek Potnis from comment #10)
> Hi Yorin,
> I have started working on the Dict.jsm file. I have edited the constructor
> and am pasting the constructor here.
> 
> function Dict(aInitial) {
>   if (aInitial === undefined)
>     aInitial = {};
> 
>     //my code begins here
>     if(typeof aInitial =="string")//passed object is a string
>     {
>     	//code to convert into JSON 
>     	aInitial = JSON.parse(aInitial);
>     	}
>     //my code ends here
> 
>   var items = {}, count = 0;
>   // That we don't look up the prototype chain is guaranteed by Iterator.
>   for (var [key, val] in Iterator(aInitial)) {
>     items[convert(key)] = val;
>     count++;
>   }
>   this._state = {count: count, items: items};
>   return Object.freeze(this);
> } 
> 
> is this part correct ?

That looks good.

By the way, generally, for this kind of feedback, we prefer the following process:
- extract the changes as a patch using mercurial;
- attach the patch to the bug;
- mark it as "feedback?" for the person who should provide feedback (in my case, you can use ":Yoric", for instance, or my full name).

This will send an e-mail to the person who should give you feedback on the code. Later, when the patch is complete, use the same process but mark instead it as "review?".

You can find more information here: https://developer.mozilla.org/en/Code_Review_FAQ

> I tested it using xpcshell by pasting the constructor
> in the console.
> is that the right way to test the code or is there any other way through
> which I can test my code ?

It is a good start. You should also run test_dict.js to ensure that you have not broken it, as follows:

 cd your_object_directory
 make -C toolkit/content
 make -C browser
 make -C toolkit/content/tests/unit xpcshell-tests


This will rebuild the JavaScript parts of your code and launch the xpcshell tests for toolkit/content.

You can find more information about xpcshell tests here: https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests#Running_unit_tests

> Also I am confused about the toString and toJSON part that you explained to
> me on IRC. Could you please explain it to me again.

It is actually quite simple.
We want a new method |toJSON|. This method should return a string in JSON format representing the dictionary. As it turns out, the current version of method |toString| does exactly that. So we should basically:
- rename the current |toString| into |toJSON|;
- add a new method |toString| that just calls this new method |toJSON|.

This will not change the behavior of |toString|, but this will let us free to change it at a later date if we want, without breaking |toJSON|.

Does this answer your questions?

Comment 13

[Abhishek Potnis [:avp]]

yes, thanks !

Comment 14

[Abhishek Potnis [:avp]]

Attachment: modified the constructor and added the toJSON method

I have added the toJSON method and also made the toString call toJSON.
I tested by pasting the entire code on xpcshell console.
I could not execute the last command :  make -C toolkit/content/tests/unit xpcshell-tests
got an error :make: *** toolkit/content/tests/unit: No such file or directory.  Stop.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(In reply to Abhishek Potnis from comment #14)
> Created attachment 638103 [details] [diff] [review]
> modified the constructor and added the toJSON method
> 
> I have added the toJSON method and also made the toString call toJSON.
> I tested by pasting the entire code on xpcshell console.
> I could not execute the last command :  make -C toolkit/content/tests/unit
> xpcshell-tests
> got an error :make: *** toolkit/content/tests/unit: No such file or
> directory.  Stop.

Did you run this from the objdir?

Comment 16

[Abhishek Potnis [:avp]]

yes, I did but there is no unit folder in the toolkit/content/tests

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 638103 [details] [diff] [review]
modified the constructor and added the toJSON method

Review of attachment 638103 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/content/Dict.jsm
@@ +39,5 @@
> +    {
> +    	//code to convert into JSON 
> +    	aInitial = JSON.parse(aInitial);
> +    	}
> +

Code looks good, but you will have to fix the layout.
Something along the lines of

if (typeof aInitial == "string") {
  aInitial = JSON.parse(aInitial);
}

Also, you will need to add your changes to the documentation (see the line |@param aInitial ... |).

@@ +235,5 @@
>  
>    /**
>     * Returns a string representation of this dictionary.
>     */
> +  toJSON: function Dict_toJSON() {

That's good.

@@ +242,5 @@
>        "}";
>    },
> +  toString: function Dict_toString() {
> + Dict_toJSON();
> +  },

Nope, this should be

  return this.toJSON();

Comment 18

[Josh Matthews [:jdm]]

(In reply to Abhishek Potnis from comment #14)
> Created attachment 638103 [details] [diff] [review]
> modified the constructor and added the toJSON method
> 
> I have added the toJSON method and also made the toString call toJSON.
> I tested by pasting the entire code on xpcshell console.
> I could not execute the last command :  make -C toolkit/content/tests/unit
> xpcshell-tests
> got an error :make: *** toolkit/content/tests/unit: No such file or
> directory.  Stop.

Remove the unit/ from the command line. xpcshell-tests is always invoked from tests/.

Comment 19

[Abhishek Potnis [:avp]]

Attachment: Made the suggested changes in Dict.jsm

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 638114 [details] [diff] [review]
Made the suggested changes in Dict.jsm

Review of attachment 638114 [details] [diff] [review]:
-----------------------------------------------------------------

I'm good with this patch if you make the final few changes.

::: toolkit/content/Dict.jsm
@@ +34,1 @@
>   */

Let me suggest the following: "if |aInitial| is a string, it is assumed to be JSON and parsed into an object".

@@ +39,5 @@
> + 
> +    if(typeof aInitial =="string") {
> +    	aInitial = JSON.parse(aInitial);
> +  }
> +

We are pretty strict about layout, not (just) because we are psychorigid, but also because if everybody uses the same layout, this simplifies a lot reading for the next hundred persons who are going to try and understand the code.

So, I am going to insist about the following points:
 - missing space after "if";
 - missing space after "==";
 - the closing "}" is not aligned.

Could you please fix these?

For more details, see https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide .

Comment 21

[Abhishek Potnis [:avp]]

Attachment: Made the suggested changes in Dict.jsm

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 638165 [details] [diff] [review]
Made the suggested changes in Dict.jsm

Review of attachment 638165 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Now, you need to write tests for the new features you have added. Once this is done, we will ask gavin to make the final review(s).

Comment 23

[Abhishek Potnis [:avp]]

Hi Yoric,
Could you please guide me on where and how to start for writing the tests.

Thanks.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

(In reply to Abhishek Potnis from comment #23)
> Could you please guide me on where and how to start for writing the tests.

Sure thing.
We use a xpcshell test to test the features of Dict.jsm. This test is in toolkit/content/tests/unit/test_dict.js .

Now, you have added two features:
1/ constructing a dictionary from a JSON string;
2/ serializing a dictionary to a JSON string.

So you should extend test_dict.js to test these two features.

1/ Testing that you can indeed construct a dictionary from a JSON string:
- Construct a dictionary |d1| from an object (e.g. |{a:1, b:2, c:"foobar"}|);
- Construct a second dictionary |d2| from a JSON string representing the same object:
   |JSON.stringify(({a:1, b:2, c:"foobar"})|;
- Ensure that each key of |d1| appears in |d2| with the same value and that each key of |d2| appears in |d1| with the same value.

See the manual of Dict.jsm https://developer.mozilla.org/en/JavaScript_code_modules/Dict.jsm for how to get the keys and the values of a dictionary.

2/ Testing that serialization works:
- Construct a dictionary |d1| from an object (as above);
- Serialize this dictionary to a JSON string by using |d1.toJSON()|;
- Construct a dictionary |d2| from the JSON string;
- As above, check that they have the exact same of keys and values.

Comment 25

[Abhishek Potnis [:avp]]

Attachment: extended test_dict.js to test for added features

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 638301 [details] [diff] [review]
extended test_dict.js to test for added features

Review of attachment 638301 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. Could you please ensure that the layout of your added tests matches the layout of the existing tests?

If the tests pass, you should now request a review from :gavin.

Comment 27

[Abhishek Potnis [:avp]]

Attachment: extended test_dict.js to test for added features

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 638335 [details] [diff] [review]
extended test_dict.js to test for added features

Review of attachment 638335 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. You should now request reviews from :gavin.

Comment 29

[Abhishek Potnis [:avp]]

review ping?

Comment 30

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 638165 [details] [diff] [review]
Made the suggested changes in Dict.jsm

>diff -r a12ce6b09f13 toolkit/content/Dict.jsm

> function Dict(aInitial) {
>   if (aInitial === undefined)
>     aInitial = {};
>+    if(typeof aInitial =="string") {
>+    	aInitial = JSON.parse(aInitial);
>+    }

Indentation is wrong here, which makes the code look wrong.

>   /**
>    * Returns a string representation of this dictionary.
>    */

Fix the comment? s/string/JSON/

>+  toString: function Dict_toString() {
>+     return this.toJSON();

Indentation is wrong here too (one extra space).

r=me with those fixed.

Comment 31

[Abhishek Potnis [:avp]]

I have fixed the indentation, but I did not get how to fix that comment. Should it be like "Returns a string/JSON representation of this dictionary" ? or should I add separate comments for the two functions |toJSON| and |toString| ?

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Just replace "string" with "JSON" in the comment (that's what "s/string/JSON/" means). I don't think it's necessary to add any more comments, this is pretty straightforward.

Comment 33

[Abhishek Potnis [:avp]]

Attachment: Converted Dict.jsm from/to JSON

Comment 34

[Abhishek Potnis [:avp]]

Should I mark the patch as check-in required ?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

The test is failing on Try.
https://tbpl.mozilla.org/?tree=Try&rev=5bc744d42d24

Comment 36

[Abhishek Potnis [:avp]]

Hi,

What changes are required in the patch, so that the test passes ? How should I analyze this
scenario ? Please help.

Thanks

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Abhishek, the failure indicated by Ryan leads to the error at: https://tbpl.mozilla.org/php/getParsedLog.php?id=14996672&tree=Try#error0

You should fix that error, launch the test locally to ensure that it now works, then mark the patch as checkin-needed.

Comment 38

[Abhishek Potnis [:avp]]

Attachment: Converted Dict.jsm from/to JSON

Comment 39

[Ekanan Ketunuti]

autoland neverland!

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Pushed to Try.
https://tbpl.mozilla.org/?tree=Try&rev=e6d648fea6f0

Also Abhishek, can you please edit your hgrc so that there's a space between your name and your email address? Thanks!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

This is still failing tests. Please do not request checkin without a link to a green Try run.

TEST-UNEXPECTED-FAIL | /Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/content/tests/unit/test_dict.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | resource://gre/modules/Dict.jsm:38 | SyntaxError: JSON.parse: expected property name or '}' - See following stack:
JS frame :: Dict@resource://gre/modules/Dict.jsm:38
JS frame :: test_serialize_dict_to_json_string@/Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/content/tests/unit/test_dict.js:279
JS frame :: run_test@/Users/cltbld/talos-slave/test/build/xpcshell/tests/toolkit/content/tests/unit/test_dict.js:303
JS frame :: _execute_test@/Users/cltbld/talos-slave/test/build/xpcshell/head.js:315
JS frame :: @-e:1

Comment 42

[Abhishek Potnis [:avp]]

extremely sorry for the inconvenience caused ! will look into it and fix the error. Sorry for marking it as checkin without testing it locally. Will mark as checkin needed only after testing locally ! very sorry !

Comment 43

[Abhishek Potnis [:avp]]

Attachment: Rewrote toJSON method

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 666554 [details] [diff] [review]
Rewrote toJSON method

Review of attachment 666554 [details] [diff] [review]:
-----------------------------------------------------------------

(canceling review, you got my feedback on IRC)

::: toolkit/content/Dict.jsm
@@ +28,5 @@
>   *
>   * @param aInitial An object containing the initial keys and values of this
>   *                 dictionary. Only the "own" enumerable properties of the
>   *                 object are considered.
> + *                 If |aInitial| is a string, it is assumed to be JSON and parsed into an object.	

Nitpick: please remove the white spaces at the end of the line.

Comment 45

[Abhishek Potnis [:avp]]

Attachment: Converted Dict.jsm from/to JSON

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 666570 [details] [diff] [review]
Converted Dict.jsm from/to JSON

Review of attachment 666570 [details] [diff] [review]:
-----------------------------------------------------------------

This should be good, except for a few typography issues. You can find more details about our coding guidelines at https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

You have my feedback+, provided tests pass, but could you please fix these issues?

::: toolkit/content/Dict.jsm
@@ +240,5 @@
> +   * Returns a JSON representation of this dictionary.
> +   */
> +  toJSON: function Dict_toJSON() {
> +    var items = this._state.items;
> +    let JSONobj={};

Nitpick: whitespaces around "=".

@@ +242,5 @@
> +  toJSON: function Dict_toJSON() {
> +    var items = this._state.items;
> +    let JSONobj={};
> +    for(let k in items)
> +    JSONobj[unconvert(k)] = items[k];

We generally:
- add a whitespace after |for|;
- add a "{" at the end of that line;
- move the following block by two whitespaces.

This simplifies reading the code.

::: toolkit/content/tests/unit/test_dict.js
@@ +281,5 @@
> +  do_check_eq(d1.get("a"), d2.get("a"));
> +  do_check_eq(d1.get("b"), d2.get("b"));
> +  do_check_eq(d1.get("c"), d2.get("c"));
> +}
> +	

Nitpick: I see 4 lines ending with whitespaces. Could you please remove them?

Comment 47

[Abhishek Potnis [:avp]]

Attachment: Converted Dict.jsm from/to JSON

Comment 48

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 662474
	Branch: mozilla-central => try
Insufficient permissions to push to try.

Comment 49

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 666594 [details] [diff] [review]
Converted Dict.jsm from/to JSON

>diff --git a/toolkit/content/Dict.jsm b/toolkit/content/Dict.jsm

>+  toJSON: function Dict_toJSON() {
>+    var items = this._state.items;
>+    let JSONobj = {};
>+    for (let k in items) {
>+      JSONobj[unconvert(k)] = items[k];
>+    }
>+    return JSON.stringify(JSONobj);
>+  },

This seems equivalent to JSON.stringify(this.items), can't you just use that?

Comment 50

[Abhishek Potnis [:avp]]

Using JSON.stringify(this._state.items) is not letting me separate the ":" that is prefixed to the keys that is used to prevent collision with any built-ins. It was being possible to do so in my previous patch as I had used unconvert method. So can you please suggest an alternate way to do it ?


Thanks.

Comment 51

[:Gavin Sharp [email: [e-mail]]]

Note that I suggested |this.items|, not |this._state.items|. The former is a getter that already does essentially what you were doing in your toJSON, AFAICT.

Comment 52

[:Gavin Sharp [email: [e-mail]]]

Ah, except that it returns an Generator, not an object, so that won't work.

Comment 53

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 666594 [details] [diff] [review]
Converted Dict.jsm from/to JSON

You should be able to use:
let obj = {};
for (let [key, item] of Iterator(this._state.items)) {
  obj[unconvert(key)] = item;
return JSON.stringify(obj);

r=me with that.

Comment 54

[Abhishek Potnis [:avp]]

Attachment: Converted Dict.jsm from/to JSON

Comment 55

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 669969 [details] [diff] [review]
Converted Dict.jsm from/to JSON

The indentation is wrong in this patch (the line in the for loop needs to be indented). r=me with that fixed (you don't need to re-request review again, just attach an updated patch).

Comment 56

[Abhishek Potnis [:avp]]

Attachment: Converted Dict.jsm from/to JSON

Comment 57

[Ekanan Ketunuti]

attachment 670014 [details] [diff] [review] need tryserver run.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Abhishek, you should really file a bug for Level 1 commit access so you can push your patches to Try. Myself or Yoric can vouch for you.
http://www.mozilla.org/hacking/commit-access-policy/

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=105322fd759e

Comment 60

[Ekanan Ketunuti]

try was green.

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0977a9122d1

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0977a9122d1

Comment 63

[Adam]

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Dict.jsm now has documentation of the constructor taking a JSON String and the |toJSON| method.

https://developer.mozilla.org/en-US/docs/Firefox_19_for_developers#Changes_for_add-on_and_Mozilla_developers has this added as well.