Last Comment Bug 727967 - Convert Dict.jsm from/to JSON
: Convert Dict.jsm from/to JSON
Status: RESOLVED FIXED
[mentor=Yoric][lang=js][good first bug]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla19
Assigned To: Abhishek Potnis [:avp]
:
Mentors:
Depends on:
Blocks: 727555
  Show dependency treegraph
 
Reported: 2012-02-16 12:45 PST by David Rajchenbach-Teller [:Yoric] (please use "needinfo")
Modified: 2012-12-19 16:38 PST (History)
11 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
modified the constructor and added the toJSON method (1.44 KB, patch)
2012-06-30 05:53 PDT, Abhishek Potnis [:avp]
no flags Details | Diff | Splinter Review
Made the suggested changes in Dict.jsm (1.68 KB, patch)
2012-06-30 08:40 PDT, Abhishek Potnis [:avp]
no flags Details | Diff | Splinter Review
Made the suggested changes in Dict.jsm (1.64 KB, patch)
2012-06-30 19:48 PDT, Abhishek Potnis [:avp]
gavin.sharp: review+
dteller: feedback+
Details | Diff | Splinter Review
extended test_dict.js to test for added features (1.43 KB, patch)
2012-07-02 04:11 PDT, Abhishek Potnis [:avp]
dteller: feedback+
Details | Diff | Splinter Review
extended test_dict.js to test for added features (1.85 KB, patch)
2012-07-02 06:30 PDT, Abhishek Potnis [:avp]
gavin.sharp: review+
dteller: feedback+
Details | Diff | Splinter Review
Converted Dict.jsm from/to JSON (2.91 KB, patch)
2012-09-01 10:05 PDT, Abhishek Potnis [:avp]
gavin.sharp: review+
Details | Diff | Splinter Review
Converted Dict.jsm from/to JSON (3.81 KB, patch)
2012-09-19 03:00 PDT, Abhishek Potnis [:avp]
no flags Details | Diff | Splinter Review
Rewrote toJSON method (3.89 KB, patch)
2012-10-01 07:49 PDT, Abhishek Potnis [:avp]
no flags Details | Diff | Splinter Review
Converted Dict.jsm from/to JSON (3.97 KB, patch)
2012-10-01 08:45 PDT, Abhishek Potnis [:avp]
dteller: feedback+
Details | Diff | Splinter Review
Converted Dict.jsm from/to JSON (3.98 KB, patch)
2012-10-01 09:48 PDT, Abhishek Potnis [:avp]
gavin.sharp: review+
Details | Diff | Splinter Review
Converted Dict.jsm from/to JSON (3.96 KB, patch)
2012-10-10 07:43 PDT, Abhishek Potnis [:avp]
gavin.sharp: review+
Details | Diff | Splinter Review
Converted Dict.jsm from/to JSON (3.99 KB, patch)
2012-10-10 09:28 PDT, Abhishek Potnis [:avp]
no flags Details | Diff | Splinter Review

Description David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-16 12:45:24 PST
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: gavin@gavinsharp.com] 2012-02-17 16:20:24 PST
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-18 02:03:24 PST
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: gavin@gavinsharp.com] 2012-02-18 17:55:29 PST
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-02-19 03:22:31 PST
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: gavin@gavinsharp.com] 2012-02-19 19:21:38 PST
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 2012-04-02 00:34:27 PDT
Hi David, 

Can I be assigned to this bug?  Thanks
Comment 7 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-04-02 02:00:15 PDT
(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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-21 03:54:51 PDT
Any news about this? Or should I set the bug as unassigned?
Comment 9 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-05-22 08:05:59 PDT
Setting the bug as unassigned.
Comment 10 Abhishek Potnis [:avp] 2012-06-29 23:19:39 PDT
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] 2012-06-29 23:24:24 PDT
extremely sorry Yoric, I misspelled your name in the previous comment.
really very sorry.
Comment 12 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-30 04:11:18 PDT
(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] 2012-06-30 04:27:42 PDT
yes, thanks !
Comment 14 Abhishek Potnis [:avp] 2012-06-30 05:53:17 PDT
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.
Comment 15 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-30 06:00:59 PDT
(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] 2012-06-30 06:06:36 PDT
yes, I did but there is no unit folder in the toolkit/content/tests
Comment 17 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-30 06:07:46 PDT
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] (away until 9/3) 2012-06-30 08:40:07 PDT
(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] 2012-06-30 08:40:23 PDT
Created attachment 638114 [details] [diff] [review]
Made the suggested changes in Dict.jsm
Comment 20 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-06-30 10:35:38 PDT
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] 2012-06-30 19:48:43 PDT
Created attachment 638165 [details] [diff] [review]
Made the suggested changes in Dict.jsm
Comment 22 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-01 08:28:36 PDT
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] 2012-07-01 09:13:53 PDT
Hi Yoric,
Could you please guide me on where and how to start for writing the tests.

Thanks.
Comment 24 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-01 09:34:42 PDT
(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] 2012-07-02 04:11:56 PDT
Created attachment 638301 [details] [diff] [review]
extended test_dict.js to test for added features
Comment 26 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-02 05:25:31 PDT
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] 2012-07-02 06:30:00 PDT
Created attachment 638335 [details] [diff] [review]
extended test_dict.js to test for added features
Comment 28 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-07-02 06:38:06 PDT
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] 2012-08-27 04:17:19 PDT
review ping?
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-30 14:46:28 PDT
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] 2012-09-01 04:11:29 PDT
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: gavin@gavinsharp.com] 2012-09-01 09:50:39 PDT
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] 2012-09-01 10:05:38 PDT
Created attachment 657575 [details] [diff] [review]
Converted Dict.jsm from/to JSON
Comment 34 Abhishek Potnis [:avp] 2012-09-01 22:56:27 PDT
Should I mark the patch as check-in required ?
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-09-05 15:43:43 PDT
The test is failing on Try.
https://tbpl.mozilla.org/?tree=Try&rev=5bc744d42d24
Comment 36 Abhishek Potnis [:avp] 2012-09-18 19:14:46 PDT
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 Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-09-19 02:30:38 PDT
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] 2012-09-19 03:00:05 PDT
Created attachment 662474 [details] [diff] [review]
Converted Dict.jsm from/to JSON
Comment 39 Ekanan Ketunuti 2012-09-19 06:01:28 PDT
autoland neverland!
Comment 40 Ryan VanderMeulen [:RyanVM] 2012-09-19 17:31:12 PDT
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] 2012-09-19 18:28:56 PDT
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] 2012-09-20 00:16:52 PDT
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] 2012-10-01 07:49:01 PDT
Created attachment 666554 [details] [diff] [review]
Rewrote toJSON method
Comment 44 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-01 08:10:56 PDT
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] 2012-10-01 08:45:35 PDT
Created attachment 666570 [details] [diff] [review]
Converted Dict.jsm from/to JSON
Comment 46 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-10-01 08:51:46 PDT
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] 2012-10-01 09:48:23 PDT
Created attachment 666594 [details] [diff] [review]
Converted Dict.jsm from/to JSON
Comment 48 Mozilla RelEng Bot 2012-10-03 07:50:15 PDT
Autoland Patchset:
	Patches: 662474
	Branch: mozilla-central => try
Insufficient permissions to push to try.
Comment 49 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-10 04:50:32 PDT
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] 2012-10-10 07:03:16 PDT
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: gavin@gavinsharp.com] 2012-10-10 07:06:42 PDT
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: gavin@gavinsharp.com] 2012-10-10 07:29:13 PDT
Ah, except that it returns an Generator, not an object, so that won't work.
Comment 53 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-10 07:32:30 PDT
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] 2012-10-10 07:43:36 PDT
Created attachment 669969 [details] [diff] [review]
Converted Dict.jsm from/to JSON
Comment 55 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-10 08:46:50 PDT
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] 2012-10-10 09:28:41 PDT
Created attachment 670014 [details] [diff] [review]
Converted Dict.jsm from/to JSON
Comment 57 Ekanan Ketunuti 2012-10-20 00:05:12 PDT
attachment 670014 [details] [diff] [review] need tryserver run.
Comment 58 Ryan VanderMeulen [:RyanVM] 2012-10-20 05:41:27 PDT
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] 2012-10-20 06:06:48 PDT
https://tbpl.mozilla.org/?tree=Try&rev=105322fd759e
Comment 60 Ekanan Ketunuti 2012-10-20 08:48:46 PDT
try was green.
Comment 61 Ryan VanderMeulen [:RyanVM] 2012-10-20 08:54:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0977a9122d1
Comment 62 Ryan VanderMeulen [:RyanVM] 2012-10-20 15:57:33 PDT
https://hg.mozilla.org/mozilla-central/rev/c0977a9122d1
Comment 63 Adam [:hobophobe] 2012-12-19 16:38:37 PST
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.

Note You need to log in before you can comment on or make changes to this bug.