David Teller [:Yoric] - still alive but not very active Reporter
	Description • 12 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 1 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 2 • 12 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 3 • 12 years ago

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))

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 4 • 12 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 5 • 12 years ago

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

	Kevin Huynh
	Comment 6 • 12 years ago

Hi David, 

Can I be assigned to this bug?  Thanks

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 7 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 8 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 9 • 12 years ago

Setting the bug as unassigned.

	Abhishek Potnis [:avp] Assignee
	Comment 10 • 12 years ago

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.

	Abhishek Potnis [:avp] Assignee
	Comment 11 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 12 • 12 years ago

(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?

	Abhishek Potnis [:avp] Assignee
	Comment 13 • 12 years ago

yes, thanks !

	Abhishek Potnis [:avp] Assignee
	Comment 14 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 15 • 12 years ago

(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?

	Abhishek Potnis [:avp] Assignee
	Comment 16 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 17 • 12 years ago

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();

	Josh Matthews [:jdm]
	Comment 18 • 12 years ago

(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/.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 20 • 12 years ago

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 .

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 22 • 12 years ago

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

	Abhishek Potnis [:avp] Assignee
	Comment 23 • 12 years ago

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

Thanks.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 24 • 12 years ago

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

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 26 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 28 • 12 years ago

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.

	Abhishek Potnis [:avp] Assignee
	Comment 29 • 12 years ago

review ping?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 30 • 12 years ago

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.

	Abhishek Potnis [:avp] Assignee
	Comment 31 • 12 years ago

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| ?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 32 • 12 years ago

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.

	Abhishek Potnis [:avp] Assignee
	Comment 34 • 12 years ago

Should I mark the patch as check-in required ?

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 12 years ago

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

	Abhishek Potnis [:avp] Assignee
	Comment 36 • 12 years ago

Hi,

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

Thanks

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 37 • 12 years ago

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.

	Ekanan Ketunuti
	Comment 39 • 12 years ago

autoland neverland!

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 12 years ago

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!

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 12 years ago

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

	Abhishek Potnis [:avp] Assignee
	Comment 42 • 12 years ago

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 !

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 44 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 46 • 12 years ago

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?

	Mozilla RelEng Bot
	Comment 48 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 49 • 12 years ago

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?

	Abhishek Potnis [:avp] Assignee
	Comment 50 • 12 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 51 • 12 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 52 • 12 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 53 • 12 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 55 • 12 years ago

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

	Ekanan Ketunuti
	Comment 57 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 58 • 12 years ago

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/

	Ryan VanderMeulen [:RyanVM]
	Comment 59 • 12 years ago

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

	Ekanan Ketunuti
	Comment 60 • 12 years ago

try was green.

	Ryan VanderMeulen [:RyanVM]
	Comment 61 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 62 • 12 years ago

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

	Adam
	Comment 63 • 12 years ago

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.