Closed Bug 1123309 Opened 9 years ago Closed 9 years ago

Remove Dict.jsm

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: gfritzsche, Assigned: babhishek21, Mentored)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [lang=js] [good next bug])

Attachments

(1 file, 6 obsolete files)

bug_1123309: rev_4: Final Cleanup. Tests passed.
32.89 KB, patch
Felipe
: review+
Felipe
: feedback+
Details | Diff | Splinter Review 
We have a custom "dictionary" implementation for browser JS code:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Dict.jsm

This is obsoleted by Map():
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map

We should remove the two uses of Dict() and port them to using Map():
https://dxr.mozilla.org/mozilla-central/search?q=Dict.jsm
Heads-up for nicely scoped mentored bug.
Side-note: whats your preferred way of heads-ups for this? needinfo, mail, ...?
Flags: needinfo?(mhoye)
Version: unspecified → Trunk
Needinfo works great! Let me see who is interested!
Flags: needinfo?(mhoye)
I'm interested! I'll do it.
Great, let me know if i can help you with anything.
So [1] references 7 matches for "Dict.jsm":
1. /browser/components/migration/MigrationUtils.jsm (1 match; requires porting)
2. /toolkit/modules/PropertyListUtils.jsm (4 matches; requires porting)
3. /toolkit/modules/tests/xpcshell/test_dict.js (1 match; requires porting)
4. /toolkit/modules/moz.build (1 match; no porting required)

Did I miss anything? If not then I'll start working.

[1] http://mxr.mozilla.org/mozilla-central/search?string=Dict.jsm&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central
(In reply to Abhishek Bhattacharya from comment #5)
> 3. /toolkit/modules/tests/xpcshell/test_dict.js (1 match; requires porting)

This doesn't require porting: it is the test for Dict.jsm, so we can just remove it.
You also need to remove the actual Dict.jsm file and the entry from the moz.build file you mentioned.
The rest looks fine.
Hey Georg (no "e" in "Georg"?),

(In reply to Georg Fritzsche [:gfritzsche] [slow to respond until Jan 19] from comment #6)
> (In reply to Abhishek Bhattacharya from comment #5)
> > 3. /toolkit/modules/tests/xpcshell/test_dict.js (1 match; requires porting)
> 
> This doesn't require porting: it is the test for Dict.jsm, so we can just
> remove it.
What if I edit test_dict.js to test Maps?(I was doing that ATM; although, I guess it would be redundant since Maps is inbuilt) So I guess I'll remove it.

> You also need to remove the actual Dict.jsm file and the entry from the
> moz.build file you mentioned.
Of course, that was implied.

BTW, can you please assign this bug to me? Or are you waiting for a patch first?
(In reply to Abhishek Bhattacharya from comment #8)
> Hey Georg (no "e" in "Georg"?),

Its the german version of George, hence no "e" at the end.
 
> (In reply to Georg Fritzsche [:gfritzsche] [slow to respond until Jan 19]
> from comment #6)
> > (In reply to Abhishek Bhattacharya from comment #5)
> > > 3. /toolkit/modules/tests/xpcshell/test_dict.js (1 match; requires porting)
> > 
> > This doesn't require porting: it is the test for Dict.jsm, so we can just
> > remove it.
> What if I edit test_dict.js to test Maps?(I was doing that ATM; although, I
> guess it would be redundant since Maps is inbuilt) So I guess I'll remove it.

Map is part of the web-facing APIs and is tested where that implementation lives. We also use it quite a bit across our source code already.
I don't think we need to keep this separate test around.
 
> BTW, can you please assign this bug to me?

Will do.
Assignee: nobody → babhishek21
Attached patch bug_1123309: Initial patch. (obsolete) — — Splinter Review 
So changes made to:
1. /browser/components/migration/MigrationUtils.jsm 
2. /toolkit/modules/PropertyListUtils.jsm
3. /toolkit/modules/moz.build

Files removed:
1. /toolkit/modules/tests/xpcshell/test_dict.js
2. /toolkit/modules/Dict.jsm

I got bold and ended up editing spelling mistakes and doxygen comments as well. I'm sorry if I overstepped. I just tried to make the comments more consistent. Please do check them.

I have NOT tested them yet with a build. I'll try and build tomorrow and comment the results.

Mean while, feel free to test and review.
Attachment #8551426 - Flags: review?(gfritzsche)
note that setAsLazyGetter is very different from set, you need to provide an alternative implementation for that. There are performance reasons behind that choice.
Comment on attachment 8551426 [details] [diff] [review]
bug_1123309: Initial patch.

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

::: browser/components/migration/MigrationUtils.jsm
@@ +214,5 @@
>          Services.obs.notifyObservers(null, aMsg, aItemType);
>        }
>  
>        notify("Migration:Started");
> +      resourcesGroupedByItems.keys().forEach(function(migrationType) {

keys() doesn't return an array, so this won't work.
You can test Map usage e.g. in the web console or in the scratch pad to confirm what it does.

::: toolkit/modules/PropertyListUtils.jsm
@@ +23,5 @@
>   * <date/>             NSDate         TYPE_DATE         Date
>   * <data/>             NSData         TYPE_UINT8_ARRAY  Uint8Array
>   * <array/>            NSArray        TYPE_ARRAY        Array
>   * Not Available       NSSet          TYPE_ARRAY        Array  [2][4]
> + * <dict/>             NSDictionary   TYPE_DICTIONARY   Map 

Nit: Trailing whitespace here.

@@ +511,5 @@
> +    /**
> +    * A dictionary in the binary format is stored as a list of references to
> +    * key-objects, followed by a list of references to the value-objects for
> +    * those keys. The size of each list is aNumberOfObjects * this._objectRefSize.
> +    */

There is no need to change this comment. Doc-style comments are used outside of functions to document them.

@@ +524,5 @@
>                               this._objectRefSize, aNumberOfObjects);
>      for (let i = 0; i < aNumberOfObjects; i++) {
>        let key = this._readObject(keyObjsRefs[i]);
>        let readBound = this._readObject.bind(this, valObjsRefs[i]);
> +      dict.set(key, readBound);

As mak mentioned, this is not equivalent.

@@ +731,5 @@
>          throw new Error("Invalid dictionary");
>  
>        let keyName = this._readObject(keyElem);
>        let readBound = this._readObject.bind(this, valElem);
> +      dict.set(keyName, readBound);

As mak mentioned, this is not equivalent.
Attachment #8551426 - Flags: review?(gfritzsche)
So I tried doing an implementation of setAsLazyGetter. However I'm not feeling too confident about it.

Trying to do an incremental build ATM.

Review required .
Attachment #8551426 - Attachment is obsolete: true
Attachment #8551774 - Flags: review?(gfritzsche)
Attachment #8551774 - Flags: review?(gfritzsche)
Attachment #8551774 [details] [diff] does not incorporate changes suggested by review in comment #12.

If you would like to check my setAsLazyGetter implementation for Map(), do view the patch. (Mentor please do)

Else, in all probability ignore it.
Please do some testing on your own first to see if things work.
We should continue once you verified its actually working or if you get stuck.
I think to reproduce the current behavior you could set a lazy getter as value for the key, and when invoked, it would replace (delete + set) the value with the final one.
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #16)
> I think to reproduce the current behavior you could set a lazy getter as
> value for the key, and when invoked, it would replace (delete + set) the
> value with the final one.

hm, even if actually that would still require to invoke the getter function, that I'm not sure it's so feasible (ideally we just want .get(key) to return a value and cache it at the same time... probably your solution that extends Map with custom properties could work. Or you could experiment with a Proxy... Sorry for the red herring.
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #17)
> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #16)
> > I think to reproduce the current behavior you could set a lazy getter as
> > value for the key, and when invoked, it would replace (delete + set) the
> > value with the final one.
> 
> hm, even if actually that would still require to invoke the getter function,
> that I'm not sure it's so feasible (ideally we just want .get(key) to return
> a value and cache it at the same time... probably your solution that extends
> Map with custom properties could work. Or you could experiment with a
> Proxy... Sorry for the red herring.

I'm feeling quite confused right now. I did try setting a lazy getter as value for key. It gives rise to one problem. The lazy getter would need to return the value from within the object instance. I think that would defeat the purpose of the lazy getter. The worst part is that the Map.protoype.set() is pre-defined and I cannot override it to pass function(key, function(args..){Object manipulation}) like what Dict.setAsLazyGetter() does with Dict.set(). 

BTW is there a way to test these files? The source builds just fine after the changes. And since the test_dict.js is no longer run, I see no other way to test. I am especially worried about /toolkit/modules/PropertyListUtils.jsm and the lazy getter function.
Flags: needinfo?(mak77)
Flags: needinfo?(gfritzsche)
(In reply to Abhishek Bhattacharya from comment #18)
> I'm feeling quite confused right now. I did try setting a lazy getter as
> value for key. It gives rise to one problem. The lazy getter would need to
> return the value from within the object instance. I think that would defeat
> the purpose of the lazy getter.

yeah, that's what I figured out in comment 17, I'm sorry, I was too optimistic.

I fear one way to do exactly the same could be to Proxy a Map (basically intercept requests and handle lazy getters differently), I didn't try though, it would require some experimenting. Or write a wrapper around Map (simpler than Dict, specific for plistutils)

>  I am especially worried about
> /toolkit/modules/PropertyListUtils.jsm and the lazy getter function.

toolkit/modules/tests/xpcshell/test_propertyListsUtils.js
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #19)
> (In reply to Abhishek Bhattacharya from comment #18)
> > I'm feeling quite confused right now. I did try setting a lazy getter as
> > value for key. It gives rise to one problem. The lazy getter would need to
> > return the value from within the object instance. I think that would defeat
> > the purpose of the lazy getter.
> 
> yeah, that's what I figured out in comment 17, I'm sorry, I was too
> optimistic.
> 
> I fear one way to do exactly the same could be to Proxy a Map (basically
> intercept requests and handle lazy getters differently), I didn't try
> though, it would require some experimenting. Or write a wrapper around Map
> (simpler than Dict, specific for plistutils)
I've never worked with wrappers, nor a proxy method. I'll have to look around I guess.
> >  I am especially worried about
> > /toolkit/modules/PropertyListUtils.jsm and the lazy getter function.
> 
> toolkit/modules/tests/xpcshell/test_propertyListsUtils.js
Isn't that test supposed to run at build-time ?! And I got no build errors.(running smooth as breeze) WTH? Or am I supposed to run these tests separately with the JS shell?
Flags: needinfo?(mak77)
(In reply to Abhishek Bhattacharya from comment #20)
> > >  I am especially worried about
> > > /toolkit/modules/PropertyListUtils.jsm and the lazy getter function.
> > 
> > toolkit/modules/tests/xpcshell/test_propertyListsUtils.js
> Isn't that test supposed to run at build-time ?! And I got no build
> errors.(running smooth as breeze) WTH? Or am I supposed to run these tests
> separately with the JS shell?

That is an xpcshell test, which is e.g. run via:
./mach xpcshell-test toolkit/modules/tests/xpcshell/test_propertyListsUtils.js

MigrationUtils should be covered by the tests in browser/components/migration/tests/unit/.

If you need more info on them: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests
Flags: needinfo?(gfritzsche)
Note: I didn't test this extensively, but it seems to work in Scratchpad. Feel free to consider this code public domain and do whatever you want with it.

let lazyMap = new Proxy(new Map(), {
  _lazyGetters: new Set(),
  get: function(target, name) {
    switch (name) {
      case "setAsLazyGetter":
        return (key, value) => {
          this._lazyGetters.add(key);
          target.set(key, value);
        };
      case "get":
        return key => {
          if (this._lazyGetters.has(key)) {
            target.set(key, target.get(key)());
          }
          return target.get(key);
        };
      case "delete":
        return key => {
          if (this._lazyGetters.has(key)) {
            this._lazyGetters.delete(key);
          }
          return target.delete(key);
        };
      default:
        return target[name];
    }
  }
});
Flags: needinfo?(mak77)
ah, already found a bug, after getting a lazyGetter key, it should be removed from _lazyGetters since it's not a plain value.
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #22)
> Note: I didn't test this extensively, but it seems to work in Scratchpad.
> Feel free to consider this code public domain and do whatever you want with
> it.
> 
> let lazyMap = new Proxy(new Map(), {
>   _lazyGetters: new Set(),
>   get: function(target, name) {
>     switch (name) {
>       case "setAsLazyGetter":
>         return (key, value) => {
>           this._lazyGetters.add(key);
.......
>         return key => {
>           if (this._lazyGetters.has(key)) {
>             this._lazyGetters.delete(key);
>           }
>           return target.delete(key);
>         };
>       default:
>         return target[name];
Wouldn't target.name be better (just in case |name| isn't a string literal)
>     }
>   }
> });
Scratchpad is happy. Will try this. 

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #23)
> ah, already found a bug, after getting a lazyGetter key, it should be
> removed from _lazyGetters since it's not a plain value.
However I don't see the need for caching the key, when you want to delete it after the proxy call anyway.

(I just realized that the simple |setAsLazyGetter:| that I wrote could use a new Set() to store |items|.)
Flags: needinfo?(mak77)
(In reply to Abhishek Bhattacharya from comment #24)
> >       default:
> >         return target[name];
> Wouldn't target.name be better (just in case |name| isn't a string literal)

This is not the same and won't work. Also, i don't think there is any real reason to limit keys to strings.

> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #23)
> > ah, already found a bug, after getting a lazyGetter key, it should be
> > removed from _lazyGetters since it's not a plain value.
> However I don't see the need for caching the key, when you want to delete it
> after the proxy call anyway.

That is not for caching, it is for telling which key actually has a lazy getter.
After it was resolved (i.e. the function invoked), we don't need that info anymore.
Flags: needinfo?(mak77)
That proxy approach looks pretty clean to me by the way, thanks mak.
Hey Marco,

> let lazyMap = new Proxy(new Map(), {
Wouldn't it be best to pass a global target (dict/map in that case) to the Proxy() call, instead of passing a |new Map()| as target object? Or did you write the |new Map()| just for prototype purposes?

I'm also having problems calling LazyMap. Could you write some call sequences? I'll continue trying on my end in the meantime.
Flags: needinfo?(mak77)
Changing status to ASSIGNED. Don't want people looking for open bugs getting disappointed. Is this ok?
Status: NEW → ASSIGNED
(In reply to Abhishek Bhattacharya from comment #27)
> I'm also having problems calling LazyMap. Could you write some call
> sequences? I'll continue trying on my end in the meantime.

You can just base it on the code in PropertyListUtils.jsm. E.g.:
dict.setAsLazyGetter("foo", () => {console.log("foo"); return 42 });
dict.get("foo"); // should return 42 and log "foo"
dict.get("foo"); // should return 42, not log anything
(In reply to Abhishek Bhattacharya from comment #27)
> > let lazyMap = new Proxy(new Map(), {
> Wouldn't it be best to pass a global target (dict/map in that case) to the
> Proxy() call, instead of passing a |new Map()| as target object? Or did you
> write the |new Map()| just for prototype purposes?
> 
> I'm also having problems calling LazyMap. Could you write some call
> sequences? I'll continue trying on my end in the meantime.

What I did was just for prototype purposes, you can likely convert that to an instantiatable object with a few changes, like you could create your somehowLazyMap (fancy name) and then use it like let dict = new somehowLazyMap().
Flags: needinfo?(mak77)
So, preliminary patch after major rewrites. Will start testing after further cleaning up (if required). 
1. Did an overhaul of PropertyUtils.jsm. 
2. Also fixed @iterable issue in MigrationUtils.jsm

OVERVIEW of 1: So I did some fishing and discovered [1]. Heartbreak. No more overriding Map with custom getters/setters.(This is what I had done in Attachment #8551774 [details] [diff])
So I took heed of Marco's Proxy technique and integrated it. I am hoping |SetAsLazyGetter| would work.

[1] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-map.prototype
Attachment #8551774 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Attachment #8553894 - Flags: review?(gfritzsche)
Attachment #8553894 - Flags: feedback?(mak77)
just as a side note, you don't need to both needinfo and feedback, one flag is enough to raise attention :)
Flags: needinfo?(mak77)
Comment on attachment 8553894 [details] [diff] [review]
bug_1123309: rev_2 (using Proxy calls for Lazy Initialisation)

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

this is an high level feedback, I didn't do a deep review

::: browser/components/migration/MigrationUtils.jsm
@@ +217,5 @@
>        notify("Migration:Started");
> +      let migrationTypeKeys = resourcesGroupedByItems.keys();
> +      for(let migrationType of migrationTypeKeys){ 
> +        function(migrationType){
> +          let itemResources = resourcesGroupedByItems.get(migrationType);

I don't understand why you need this additional function and this .get() call too.

why not just

for (let [migrationType, itemResources] of resourcesGroupedByItems.entries()) {
  ...
}

@@ +223,3 @@
>  
> +          let itemSuccess = false;
> +          itemResources.forEach(function(resource) {

for (let resource of itemResources)

@@ +442,5 @@
>      return PlacesUtils.bookmarks.createFolder(
>        aParentId, label, PlacesUtils.bookmarks.DEFAULT_INDEX);
>    },
>  
> +  get _migrators() gMigrators ? gMigrators : gMigrators = new Map(),

Expression closures are deprecated, so while here please change this to 

get _migrators() {
  return ...
},

see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Expression_closures

::: toolkit/modules/PropertyListUtils.jsm
@@ +525,5 @@
> +          return key => {
> +            if (this._lazyGetters.has(key)) {
> +              target.set(key, target.get(key)());
> +            }
> +            this._lazyGetters.delete(key);

should be inside the if

@@ +557,5 @@
> +   *
> +   * @note No context is provided for aThunk when it's invoked.
> +   *       Use Function.bind if you wish to run it in a certain context.
> +   */
> +  setAsLazyGetter: function Map_dict_setAsLazyGetter(aKey, aThunk, aMap) {

I don't understand why you need this, why don't you just make dict a lazy Map?

@@ +581,5 @@
>    _wrapDictionary: function(aObjectOffset, aNumberOfObjects) {
>      // A dictionary in the binary format is stored as a list of references to
>      // key-objects, followed by a list of references to the value-objects for
>      // those keys. The size of each list is aNumberOfObjects * this._objectRefSize.
> +    let dict = new Map();

that is, here just do something like

let dict = new Proxy(new Map(), this._lazyHandler);

and around use dict as a normal Map, also use dict.setAsLazyGetter
Attachment #8553894 - Flags: feedback?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #33)
> Comment on attachment 8553894 [details] [diff] [review]
> bug_1123309: rev_2 (using Proxy calls for Lazy Initialisation)
> 
> Review of attachment 8553894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this is an high level feedback, I didn't do a deep review
Yes, I know. I asked for a feedback.
> ::: browser/components/migration/MigrationUtils.jsm
> @@ +217,5 @@
> >        notify("Migration:Started");
> > +      let migrationTypeKeys = resourcesGroupedByItems.keys();
> > +      for(let migrationType of migrationTypeKeys){ 
> > +        function(migrationType){
> > +          let itemResources = resourcesGroupedByItems.get(migrationType);
> 
> I don't understand why you need this additional function and this .get()
> call too.
> 
> why not just
> 
> for (let [migrationType, itemResources] of
> resourcesGroupedByItems.entries()) {
>   ...
> }
A very simple explanation. This is how it came in the box. I was asked to remove Dict.jsm and resolve all dependencies. I haven't looked at better ways of writing code. I merely changed it enough to work with Maps. But if you want me to fix code-style and performance improvements, I could try that. But I think I'll wait confirmation from Georg. That alright?
> 
> @@ +223,3 @@
> >  
> > +          let itemSuccess = false;
> > +          itemResources.forEach(function(resource) {
> 
> for (let resource of itemResources)
I had a huge prick regarding this. If you look at this complete call sequence (its actually a Callback function), |itemResources| should be a single instance static (like int, string, etc.) and not iterable (list, set, any other object). Yet another callback has been called for each of its instances. This is possible only if each instance of |itemResources| is an iterable type itself. And I'm not sure that's the case.
> @@ +442,5 @@
> >      return PlacesUtils.bookmarks.createFolder(
> >        aParentId, label, PlacesUtils.bookmarks.DEFAULT_INDEX);
> >    },
> >  
> > +  get _migrators() gMigrators ? gMigrators : gMigrators = new Map(),
> 
> Expression closures are deprecated, so while here please change this to 
> 
> get _migrators() {
>   return ...
> },
> 
> see
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/
> Expression_closures
Again, came in the box.
> ::: toolkit/modules/PropertyListUtils.jsm
> @@ +525,5 @@
> > +          return key => {
> > +            if (this._lazyGetters.has(key)) {
> > +              target.set(key, target.get(key)());
> > +            }
> > +            this._lazyGetters.delete(key);
> 
> should be inside the if
Hmm, will investigate. 
> @@ +557,5 @@
> > +   *
> > +   * @note No context is provided for aThunk when it's invoked.
> > +   *       Use Function.bind if you wish to run it in a certain context.
> > +   */
> > +  setAsLazyGetter: function Map_dict_setAsLazyGetter(aKey, aThunk, aMap) {
> 
> I don't understand why you need this, why don't you just make dict a lazy
> Map?
> 
> @@ +581,5 @@
> >    _wrapDictionary: function(aObjectOffset, aNumberOfObjects) {
> >      // A dictionary in the binary format is stored as a list of references to
> >      // key-objects, followed by a list of references to the value-objects for
> >      // those keys. The size of each list is aNumberOfObjects * this._objectRefSize.
> > +    let dict = new Map();
> 
> that is, here just do something like
> 
> let dict = new Proxy(new Map(), this._lazyHandler);
> 
> and around use dict as a normal Map, also use dict.setAsLazyGetter
Ah, I was so busy fixing |setAsLazyGetter|, this thought never crossed my mind. :P Yes, that would be loads better.(Even performance-wise) I'll fix this.

Thanks for the feedback.
Flags: needinfo?(mak77)
(In reply to Abhishek Bhattacharya from comment #34)
> A very simple explanation. This is how it came in the box. I was asked to
> remove Dict.jsm and resolve all dependencies. I haven't looked at better
> ways of writing code. I merely changed it enough to work with Maps. But if
> you want me to fix code-style and performance improvements, I could try
> that. But I think I'll wait confirmation from Georg. That alright?

well, you removed a forEach, as a consequence to that, there's not reason to keeping around a function.

> > @@ +223,3 @@
> > >  
> > > +          let itemSuccess = false;
> > > +          itemResources.forEach(function(resource) {
> > 
> > for (let resource of itemResources)
> I had a huge prick regarding this. If you look at this complete call
> sequence (its actually a Callback function), |itemResources| should be a
> single instance static (like int, string, etc.) and not iterable (list, set,
> any other object). Yet another callback has been called for each of its
> instances. This is possible only if each instance of |itemResources| is an
> iterable type itself. And I'm not sure that's the case.

I'm sorry, I don't follow. itemResources is an array, and there's no complete sequence of callbacks if you use for...of and avoid the pointless function
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #35)
> (In reply to Abhishek Bhattacharya from comment #34)
> > A very simple explanation. This is how it came in the box. I was asked to
> > remove Dict.jsm and resolve all dependencies. I haven't looked at better
> > ways of writing code. I merely changed it enough to work with Maps. But if
> > you want me to fix code-style and performance improvements, I could try
> > that. But I think I'll wait confirmation from Georg. That alright?
> 
> well, you removed a forEach, as a consequence to that, there's not reason to
> keeping around a function.
I removed forEach in the first case because |resourcesGroupedByItems.keys()| is an @@iterable type and not an array. Infact I was willing to modify the statement to make forEach work. I guess I'll take a call.
> > > @@ +223,3 @@
> > > >  
> > > > +          let itemSuccess = false;
> > > > +          itemResources.forEach(function(resource) {
> > > 
> > > for (let resource of itemResources)
> > I had a huge prick regarding this. If you look at this complete call
> > sequence (its actually a Callback function), |itemResources| should be a
> > single instance static (like int, string, etc.) and not iterable (list, set,
> > any other object). Yet another callback has been called for each of its
> > instances. This is possible only if each instance of |itemResources| is an
> > iterable type itself. And I'm not sure that's the case.
> 
> I'm sorry, I don't follow. itemResources is an array, and there's no
> complete sequence of callbacks if you use for...of and avoid the pointless
> function
Hmm, I'll have to look into the functioning of the function.
Flags: needinfo?(mak77)
(In reply to Abhishek Bhattacharya [:babhishek21] from comment #36)
> (In reply to Marco Bonardo [::mak] (needinfo? me) from comment #35)
> > (In reply to Abhishek Bhattacharya from comment #34)
> > > A very simple explanation. This is how it came in the box. I was asked to
> > > remove Dict.jsm and resolve all dependencies. I haven't looked at better
> > > ways of writing code. I merely changed it enough to work with Maps. But if
> > > you want me to fix code-style and performance improvements, I could try
> > > that. But I think I'll wait confirmation from Georg. That alright?
> > 
> > well, you removed a forEach, as a consequence to that, there's not reason to
> > keeping around a function.
> I removed forEach in the first case because |resourcesGroupedByItems.keys()|
> is an @@iterable type and not an array. Infact I was willing to modify the
> statement to make forEach work. I guess I'll take a call.

There is no reason to keep the existing structure here. This can (and should) be simplified as suggested.
While we change existing code, we should also improve it for obvious issues.
(In reply to Georg Fritzsche [:gfritzsche] from comment #37)
> There is no reason to keep the existing structure here. This can (and
> should) be simplified as suggested.
> While we change existing code, we should also improve it for obvious issues.

Good to have it confirmed. Will start on fixing it.
Flags: needinfo?(mak77) → needinfo?
Flags: needinfo?
Comment on attachment 8553894 [details] [diff] [review]
bug_1123309: rev_2 (using Proxy calls for Lazy Initialisation)

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

I think mak already covered this well.

Note that there is some trailing whitespace in your changes that you should remove. You can see highlighted it in red when you follow the review links (or configure your editor to show it to you).
Attachment #8553894 - Flags: review?(gfritzsche)
Attached patch bug_1123309: rev_3 (Cleanup) (obsolete) — — Splinter Review 
So did some cleanup and made some style changes. Removed trailing whitespaces (at least according to my text-editor)

This should do well.
Attachment #8553894 - Attachment is obsolete: true
Attachment #8554286 - Flags: review?(gfritzsche)
Attachment #8554286 - Flags: feedback?(mak77)
Comment on attachment 8554286 [details] [diff] [review]
bug_1123309: rev_3 (Cleanup)

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

::: browser/components/migration/MigrationUtils.jsm
@@ +223,1 @@
>            let resourceDone = function(aSuccess) {

so, there is a very subtle bug here, due to the undefined (so far) let semantics in for loops (see bug 449811).

try this in scratchpad:

let m = new Map();
m.set("one", ["a", "b"]);
m.set("two", ["c", "d"]);
m.set("three", ["e", "f"]);
for (let [key, value] of m.entries()) {
  for (let val of value) {
    let resourceDone = function() {
      console.log(value);
    }
    Services.tm.mainThread.dispatch(() => {
      console.log(val);
      resourceDone();
    }, Ci.nsIThread.DISPATCH_NORMAL);
  }
}

The foreach was hiding the problem.
A possible workaround is to redefine a local var, like:

for (let [key, val] of resourcesGroupedByItems.entries()) {
  // TODO: (bug 449811).
  let migrationType = key, itemResources = val;

...

for (let r of itemResources) {
  let resource = r;

Or alternatively, just go back to the old foreach loop :(
Sorry, I noticed this just now, and it's not very easy to notice.

@@ +441,5 @@
>  
> +  get _migrators() {
> +    if (!gMigrators)
> +      return gMigrators = new Map();
> +    return gMigrators;

there should be no need to change the conditional operator, you can just retain

get _migrators() {
  return gMigrators ? gMigrators : gMigrators = new Map();
}

::: toolkit/modules/PropertyListUtils.jsm
@@ +573,5 @@
> +
> +      if (!dict.has(key))
> +        dict.setAsLazyGetter(key, readBound);
> +      else
> +        dict.setAsLazyGetter(key, dict.get(key));

why do you need to do this? setting a key over a key with the same name should just work.
It shouldn't make a difference in this case cause I think we never hit the else case, but this code could end up setting a key to the value it already has, when maybe the scope is to replace the lazy getter with an updated version, for whatever reason.

@@ +784,5 @@
> +
> +      if (!dict.has(keyName))
> +        dict.setAsLazyGetter(keyName, readBound);
> +      else
> +        dict.setAsLazyGetter(keyName, dict.get(key));

ditto
Attachment #8554286 - Flags: feedback?(mak77)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #41)
> Comment on attachment 8554286 [details] [diff] [review]
> bug_1123309: rev_3 (Cleanup)
> 
> Review of attachment 8554286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/migration/MigrationUtils.jsm
> @@ +223,1 @@
> >            let resourceDone = function(aSuccess) {
> 
> so, there is a very subtle bug here, due to the undefined (so far) let
> semantics in for loops (see bug 449811).
> 
> try this in scratchpad:
> 
> let m = new Map();
> m.set("one", ["a", "b"]);
> m.set("two", ["c", "d"]);
> m.set("three", ["e", "f"]);
> for (let [key, value] of m.entries()) {
>   for (let val of value) {
>     let resourceDone = function() {
>       console.log(value);
>     }
>     Services.tm.mainThread.dispatch(() => {
>       console.log(val);
>       resourceDone();
>     }, Ci.nsIThread.DISPATCH_NORMAL);
>   }
> }
> 
> The foreach was hiding the problem.
> A possible workaround is to redefine a local var, like:
> 
> for (let [key, val] of resourcesGroupedByItems.entries()) {
>   // TODO: (bug 449811).
>   let migrationType = key, itemResources = val;
> 
> ...
> 
> for (let r of itemResources) {
>   let resource = r;
> 
> Or alternatively, just go back to the old foreach loop :(
> Sorry, I noticed this just now, and it's not very easy to notice.
Haha. That's how it was in my previous patch. (Now I understand why explicit var declarations were made) I guess we go back now. 
> @@ +441,5 @@
> >  
> > +  get _migrators() {
> > +    if (!gMigrators)
> > +      return gMigrators = new Map();
> > +    return gMigrators;
> 
> there should be no need to change the conditional operator, you can just
> retain
> 
> get _migrators() {
>   return gMigrators ? gMigrators : gMigrators = new Map();
> }
OK. Although, it wouldn't make a difference I guess. 
> ::: toolkit/modules/PropertyListUtils.jsm
> @@ +573,5 @@
> > +
> > +      if (!dict.has(key))
> > +        dict.setAsLazyGetter(key, readBound);
> > +      else
> > +        dict.setAsLazyGetter(key, dict.get(key));
> 
> why do you need to do this? setting a key over a key with the same name
> should just work.
> It shouldn't make a difference in this case cause I think we never hit the
> else case, but this code could end up setting a key to the value it already
> has, when maybe the scope is to replace the lazy getter with an updated
> version, for whatever reason.
> 
Hmm. From what I understood about Lazy initialisation, we usually initialise an object or its attribute, the first time the object/attribute is actually used (just-in-time). And I believe |readBound|, may actually be a thunk of code. So the first time the |key| is mapped, its value is initialized to the thunk. The else call is to ensure that any subsequent calls to retrieve |key| result in its mapped value being initialized to the return value of running the thunk. Also since it was guaranteed that the thunk wouldn't be called more than once, Voila! we have lazy just-in-time initialisation of |key|'s value. Get it? Correct me if I screwed up.
Flags: needinfo?(mak77)
(In reply to Abhishek Bhattacharya [:babhishek21] from comment #42)
> Haha. That's how it was in my previous patch. (Now I understand why explicit
> var declarations were made) I guess we go back now. 

well apart that even in your previous patch it wouldn't have worked cause you were still defining a function inside a for...of.

> >   return gMigrators ? gMigrators : gMigrators = new Map();
> > }
> OK. Although, it wouldn't make a difference I guess. 

yes it wouldn't make a difference (apart it's one line instead of more)

> Hmm. From what I understood about Lazy initialisation, we usually initialise
> an object or its attribute, the first time the object/attribute is actually
> used (just-in-time). And I believe |readBound|, may actually be a thunk of
> code. So the first time the |key| is mapped, its value is initialized to the
> thunk. The else call is to ensure that any subsequent calls to retrieve
> |key| result in its mapped value being initialized to the return value of
> running the thunk.

yes, but why? in this case we are setting the lazy initializer, not trying to retrieve anything.

> Also since it was guaranteed that the thunk wouldn't be
> called more than once, Voila! we have lazy just-in-time initialisation of
> |key|'s value. Get it? Correct me if I screwed up.

The lazy initializer has performance reasons, while it's true we don't want it to be called more than once, it's also true that we want this object to work simply like an extented Map. A Map allows to just set a value over an existing key, the same should be true for lazy initializers, if I want to replace a lazy initializer I should be able to just by invoking setAsLazyGetter.
In this case, based on the code, there's no reason to complicate the API call to ensure 100% we won't call the inizializer more than once, cause the calling code already ensures that, and even if that happens, nothing breaks. So better having a simpler API call, imo.
Flags: needinfo?(mak77)
Comment on attachment 8554286 [details] [diff] [review]
bug_1123309: rev_3 (Cleanup)

It seems like this already has actionable feedback here thanks to mak.
Attachment #8554286 - Flags: review?(gfritzsche)
Right. Did some more changes here and there to accommodate Marco's feedback on Attachment 8554286 [details] [diff].

Review and/or feedback please.
Attachment #8554286 - Attachment is obsolete: true
Flags: needinfo?(mak77)
Flags: needinfo?(gfritzsche)
Attachment #8557928 - Flags: review?(gfritzsche)
Attachment #8557928 - Flags: feedback?(mak77)
Comment on attachment 8557928 [details] [diff] [review]
bug_1123309: rev_3.1: Some more cleanup.

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

off-hand it looks good, but notice I didn't do a line-by-line review, nor I tested the patch to ensure PropertyListUtils works correctly (unfortunately atm I have too many pending requests). I'm leaving the review to Georg.
Thanks for taking care of my feedback so far.
Attachment #8557928 - Flags: feedback?(mak77)
Flags: needinfo?(mak77)
I haven't had a chance to get here yet. I'm hoping to look over it tomorrow.

Side-note: you don't need to set both review & needinfo (or feedback and needinfo), one of them is enough to show up as requests.
Flags: needinfo?(gfritzsche)
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #46)
> Comment on attachment 8557928 [details] [diff] [review]
> bug_1123309: rev_3.1: Some more cleanup.
> 
> Review of attachment 8557928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> off-hand it looks good, but notice I didn't do a line-by-line review, nor I
> tested the patch to ensure PropertyListUtils works correctly (unfortunately
> atm I have too many pending requests). I'm leaving the review to Georg.
> Thanks for taking care of my feedback so far.
I already tested it. Working all right, so far. 

(In reply to Georg Fritzsche [:gfritzsche] from comment #47)
> I haven't had a chance to get here yet. I'm hoping to look over it tomorrow.
> 
> Side-note: you don't need to set both review & needinfo (or feedback and
> needinfo), one of them is enough to show up as requests.
No rush. Sorry about the needinfo. Marco explicitly states it in his nick (my original intention was to use it for him only)
Attachment #8557928 - Flags: review?(mak77)
Sorry, i was sick, but i'll try to get here tomorrow :-/

(In reply to Abhishek Bhattacharya [:babhishek21] from comment #48)
> > Side-note: you don't need to set both review & needinfo (or feedback and
> > needinfo), one of them is enough to show up as requests.
> No rush. Sorry about the needinfo. Marco explicitly states it in his nick
> (my original intention was to use it for him only)

The idea is to set needinfo? on people if you don't set other flags (like review? or feedback?).
If you set any of those flags, they show up in peoples queue and they will get to it when they can.
Comment on attachment 8557928 [details] [diff] [review]
bug_1123309: rev_3.1: Some more cleanup.

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

I think this still needs some work on PropertyListUtils.jsm.
Have you tried if test_propertyListsUtils.js still passes with your changes?

::: browser/components/migration/MigrationUtils.jsm
@@ +214,5 @@
>          Services.obs.notifyObservers(null, aMsg, aItemType);
>        }
>  
>        notify("Migration:Started");
> +      for (let [key, value] of resourcesGroupedByItems.entries()) {

for (let [key, value] of resourcesGroupedByItems)

@@ +238,4 @@
>                    notify("Migration:Ended");
>                }
>              }
>            };

Remove the semicolon.

@@ +250,5 @@
>                Cu.reportError(ex);
>                resourceDone(false);
>              }
>            }, Ci.nsIThread.DISPATCH_NORMAL);
> +        };

Remove the semicolon.

@@ +567,5 @@
>     *        browser selected, if it could be detected and if there is a
>     *        migrator for it, or with the first option selected as a fallback
>     *        (The first option is hardcoded to be the most common browser for
>     *         the OS we run on.  See migration.xul).
> +   *

I don't think we need to touch that line.

::: toolkit/modules/PropertyListUtils.jsm
@@ +515,5 @@
> +   *         Key to be set, retrieved or deleted. Keys are checked for laziness.
> +   * @return Returns value of "name" property of target by default. Otherwise returns
> +   *         updated target.
> +   */
> +  _lazyGetters: new Set(),

This means that all instances returned by _wrapDictionary() share the same set of lazy getters. That doesn't seem correct.

@@ +767,5 @@
>      //   <string>My String Key</string>
>      // </dict>
>      if (aDOMElt.children.length % 2 != 0)
>        throw new Error("Invalid dictionary");
> +    let dict = new Proxy(new Map(), this._lazyHandler);

_lazyHandler is on another object. We'll want to put the lazy map implementation on the top-level to use it in both BinaryPropertyListReader and XMLPropertyListReader.
Attachment #8557928 - Flags: review?(mak77)
Attachment #8557928 - Flags: review?(gfritzsche)
(In reply to Georg Fritzsche [:gfritzsche] from comment #50)
> Comment on attachment 8557928 [details] [diff] [review]
> bug_1123309: rev_3.1: Some more cleanup.
> 
> Review of attachment 8557928 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this still needs some work on PropertyListUtils.jsm.
> Have you tried if test_propertyListsUtils.js still passes with your changes?
> 
> ::: browser/components/migration/MigrationUtils.jsm
> @@ +214,5 @@
> >          Services.obs.notifyObservers(null, aMsg, aItemType);
> >        }
> >  
> >        notify("Migration:Started");
> > +      for (let [key, value] of resourcesGroupedByItems.entries()) {
> 
> for (let [key, value] of resourcesGroupedByItems)
OK. But what's the difference? 
> @@ +238,4 @@
> >                    notify("Migration:Ended");
> >                }
> >              }
> >            };
> 
> Remove the semicolon.
> 
> @@ +250,5 @@
> >                Cu.reportError(ex);
> >                resourceDone(false);
> >              }
> >            }, Ci.nsIThread.DISPATCH_NORMAL);
> > +        };
> 
> Remove the semicolon.
OK.
> @@ +567,5 @@
> >     *        browser selected, if it could be detected and if there is a
> >     *        migrator for it, or with the first option selected as a fallback
> >     *        (The first option is hardcoded to be the most common browser for
> >     *         the OS we run on.  See migration.xul).
> > +   *
> 
> I don't think we need to touch that line.
I never meant to. Its just that (AFAIR) this line existed but at some point, I accidentally removed it and my Diff Viewer started throwing it up. If its not required, I'll remove it. Maybe its for clarity.
> ::: toolkit/modules/PropertyListUtils.jsm
> @@ +515,5 @@
> > +   *         Key to be set, retrieved or deleted. Keys are checked for laziness.
> > +   * @return Returns value of "name" property of target by default. Otherwise returns
> > +   *         updated target.
> > +   */
> > +  _lazyGetters: new Set(),
> 
> This means that all instances returned by _wrapDictionary() share the same
> set of lazy getters. That doesn't seem correct.
Umm, If I declare the _lazyGetters set as a new Set() inside _wrapDictionary(), then every time I make a proxy association, a new Set() is created. How then would we cache already called keys? 
> @@ +767,5 @@
> >      //   <string>My String Key</string>
> >      // </dict>
> >      if (aDOMElt.children.length % 2 != 0)
> >        throw new Error("Invalid dictionary");
> > +    let dict = new Proxy(new Map(), this._lazyHandler);
> 
> _lazyHandler is on another object. We'll want to put the lazy map
> implementation on the top-level to use it in both BinaryPropertyListReader
> and XMLPropertyListReader.
Yes, silly of me. I'll put _lazyhandler in the global scope.
Flags: needinfo?(gfritzsche)
(In reply to Abhishek Bhattacharya [:babhishek21] from comment #51)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #50)
> > ::: browser/components/migration/MigrationUtils.jsm
> > @@ +214,5 @@
> > >          Services.obs.notifyObservers(null, aMsg, aItemType);
> > >        }
> > >  
> > >        notify("Migration:Started");
> > > +      for (let [key, value] of resourcesGroupedByItems.entries()) {
> > 
> > for (let [key, value] of resourcesGroupedByItems)
> OK. But what's the difference? 

That should do the same thing, so let's choose the shorter form.

> > @@ +567,5 @@
> > >     *        browser selected, if it could be detected and if there is a
> > >     *        migrator for it, or with the first option selected as a fallback
> > >     *        (The first option is hardcoded to be the most common browser for
> > >     *         the OS we run on.  See migration.xul).
> > > +   *
> > 
> > I don't think we need to touch that line.
> I never meant to. Its just that (AFAIR) this line existed but at some point,
> I accidentally removed it and my Diff Viewer started throwing it up. If its
> not required, I'll remove it. Maybe its for clarity.

If it's not relevant to this bug and not close to changes we already make, we don't really need to touch it.
Less code touched seems good (less lines to review, less code history affected, ...).
If we want to do clean-up we could always do it in separate patches or bugs.

> > ::: toolkit/modules/PropertyListUtils.jsm
> > @@ +515,5 @@
> > > +   *         Key to be set, retrieved or deleted. Keys are checked for laziness.
> > > +   * @return Returns value of "name" property of target by default. Otherwise returns
> > > +   *         updated target.
> > > +   */
> > > +  _lazyGetters: new Set(),
> > 
> > This means that all instances returned by _wrapDictionary() share the same
> > set of lazy getters. That doesn't seem correct.
> Umm, If I declare the _lazyGetters set as a new Set() inside
> _wrapDictionary(), then every time I make a proxy association, a new Set()
> is created. How then would we cache already called keys? 

But that would be the behavior that we want (and the existing behavior) - each instance returned by _wrapDictionary() is a new dictionary with its own lazy getters.
Flags: needinfo?(gfritzsche)
Attached patch bug_1123309: rev_3.2: Cleanup (obsolete) — — Splinter Review 
Hey Georg,
I have done some cleanup (as required by Comment #50). Over to you.

Sorry for the late patch.
Attachment #8557928 - Attachment is obsolete: true
Attachment #8567518 - Flags: review?(gfritzsche)
Sorry for the late response Abhishek.
I have a really full week here and will try to get to this review tomorrow.
(In reply to Georg Fritzsche [:gfritzsche] [away Feb 27 - March 15] from comment #54)
> Sorry for the late response Abhishek.
> I have a really full week here and will try to get to this review tomorrow.

No problem. Take as much time as you want. I can reply quick enough till 9th of March. Then again after 16th of March.

BTW Are you aware of any paid student projects/internships at Mozilla for the coming summer? I'd like to apply.
Flags: needinfo?(gfritzsche)
(In reply to Abhishek Bhattacharya [:babhishek21] from comment #55)
> BTW Are you aware of any paid student projects/internships at Mozilla for
> the coming summer? I'd like to apply.
Thank you for your interest in working with Mozilla. There are several opportunities to do so:

Google Summer of Code 2015:
Please read the info page https://wiki.mozilla.org/SummerOfCode and the formal ideas page https://wiki.mozilla.org/Community:SummerOfCode15
You can also suggest your own idea but will have to find a mentor for it.

Internship at Mozilla:
Please read http://careers.mozilla.org/en-US/ for this. The open internship positions are also listed there. As far as I know, the interns worked in the Mozilla San Francisco, Mountain View or Toronto offices in the past.
I didnt find time, but Felipe has kindly offered to take a look :)
Mentor: felipc
Flags: needinfo?(gfritzsche)
Comment on attachment 8567518 [details] [diff] [review]
bug_1123309: rev_3.2: Cleanup

Hey Abhishek, I'll help out with this review while Georg is away. I'll look at this review tonight. I'm marking my name here so I don't lose track of it.
Attachment #8567518 - Flags: review?(felipc)
Comment on attachment 8567518 [details] [diff] [review]
bug_1123309: rev_3.2: Cleanup

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

Hi Abhishek, thanks for the patch :) Who would have thought that removing Dict.jsm would be so much more complicated than at first glance? Good work on this one! Sorry for the delay, it took me a while to read through the detailed comments here and understand all the context behind this patch, specially the proxy for PropertyListUtils. There are some things left to change, but it's definitely getting close.

Also, did you manage to run the PropertyListUtils tests as Georg mentioned in comment 21? (using `./mach xpcshell-test`)? If you need help with that let me now.

::: toolkit/modules/PropertyListUtils.jsm
@@ +254,5 @@
>    this._objects = [];
>  }
>  
> +/**
> +   * Simple handler to handle proxy calls to dict/Map to process Thunks.

The comments here could be a bit more reflective of the real purpose of this object. You can say that it is intended to add the setAsLazyGetter function to the Map object, and no need to mention thunks (you're talking about subroutines, right?), which is just an implementation detail.

@@ +257,5 @@
> +/**
> +   * Simple handler to handle proxy calls to dict/Map to process Thunks.
> +   * @member _lazyGetters
> +   *         Set() object to hold keys invoking LazyGetter.
> +   * @member get

get is a function. I don't think @member is the correct javadoc way to document it, right?

@@ +270,5 @@
> +   *         Key to be set, retrieved or deleted. Keys are checked for laziness.
> +   * @return Returns value of "name" property of target by default. Otherwise returns
> +   *         updated target.
> +   */
> +_lazyHandler : {

Invalid syntax here, I believe because you moved this outside of the BinaryPropertyListReader.prototype object.  But both BinaryPropertyListReader.prototype and XMLPropertyListReader.prototype will use this, so you're correct in having moved this object to the outside scope. But you'll need to update the syntax to make it work. Also, this is not a good location in the file to add this block of code, because it is in between the definition of "function BinaryPropertyListReader" and "BinaryPropertyListReader.prototype". I think adding it to the end of the file is better.

Also please use a more descriptive name, like lazyMapProxyHandler. But read the next comment..

@@ +271,5 @@
> +   * @return Returns value of "name" property of target by default. Otherwise returns
> +   *         updated target.
> +   */
> +_lazyHandler : {
> +    _lazyGetters: new Set(),

The proxy is being used twice, but _lazyHandler is a single object that is reused. This means that the two Dicts using this proxy will share the set of _lazyGetters, which is wrong.

So, instead of defining _lazyMapProxyHandler as an object, you'll need to define it as a function that returns a fresh object each time it's called, and then call it on the Proxy creation.

For example:

function LazyMapProxyHandler() {
  return {
    get...
  }
}

and then

let dict = new Proxy(new Map(), LazyMapProxyHandler());


Makes sense?
Attachment #8567518 - Flags: review?(gfritzsche)
Attachment #8567518 - Flags: review?(felipc)
Attachment #8567518 - Flags: feedback+
(In reply to :Felipe Gomes from comment #59)
> Comment on attachment 8567518 [details] [diff] [review]
> bug_1123309: rev_3.2: Cleanup
> 
> Review of attachment 8567518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Abhishek, thanks for the patch :) Who would have thought that removing
> Dict.jsm would be so much more complicated than at first glance? Good work
> on this one! Sorry for the delay, it took me a while to read through the
> detailed comments here and understand all the context behind this patch,
> specially the proxy for PropertyListUtils. There are some things left to
> change, but it's definitely getting close.
Using the proxy was Marco's idea. 
> Also, did you manage to run the PropertyListUtils tests as Georg mentioned
> in comment 21? (using `./mach xpcshell-test`)? If you need help with that
> let me now.
I tried running them the first time around when I did some major changes. I couldn't make heads or tails of the output log. 
> ::: toolkit/modules/PropertyListUtils.jsm
> @@ +254,5 @@
> >    this._objects = [];
> >  }
> >  
> > +/**
> > +   * Simple handler to handle proxy calls to dict/Map to process Thunks.
> 
> The comments here could be a bit more reflective of the real purpose of this
> object. You can say that it is intended to add the setAsLazyGetter function
> to the Map object, and no need to mention thunks (you're talking about
> subroutines, right?), which is just an implementation detail.
> 
> @@ +257,5 @@
> > +/**
> > +   * Simple handler to handle proxy calls to dict/Map to process Thunks.
> > +   * @member _lazyGetters
> > +   *         Set() object to hold keys invoking LazyGetter.
> > +   * @member get
> 
> get is a function. I don't think @member is the correct javadoc way to
> document it, right?
Will look into it. 
> @@ +270,5 @@
> > +   *         Key to be set, retrieved or deleted. Keys are checked for laziness.
> > +   * @return Returns value of "name" property of target by default. Otherwise returns
> > +   *         updated target.
> > +   */
> > +_lazyHandler : {
> 
> Invalid syntax here, I believe because you moved this outside of the
> BinaryPropertyListReader.prototype object.  But both
> BinaryPropertyListReader.prototype and XMLPropertyListReader.prototype will
> use this, so you're correct in having moved this object to the outside
> scope. But you'll need to update the syntax to make it work. Also, this is
> not a good location in the file to add this block of code, because it is in
> between the definition of "function BinaryPropertyListReader" and
> "BinaryPropertyListReader.prototype". I think adding it to the end of the
> file is better.
End of file? But what about lifetime of the object? Shouldn't it be declared at the top of the file? If I declare a function, then I think the prototype must be declared on top.
> Also please use a more descriptive name, like lazyMapProxyHandler. But read
> the next comment..
> 
> @@ +271,5 @@
> > +   * @return Returns value of "name" property of target by default. Otherwise returns
> > +   *         updated target.
> > +   */
> > +_lazyHandler : {
> > +    _lazyGetters: new Set(),
> 
> The proxy is being used twice, but _lazyHandler is a single object that is
> reused. This means that the two Dicts using this proxy will share the set of
> _lazyGetters, which is wrong.
Isn't _lazyGetters defined inside our proxy handler. So whenever a new proxy binding is established, a new _lazyGetters Set() should be made? Or am I wrong?
> So, instead of defining _lazyMapProxyHandler as an object, you'll need to
> define it as a function that returns a fresh object each time it's called,
> and then call it on the Proxy creation.
> 
> For example:
> 
> function LazyMapProxyHandler() {
>   return {
>     get...
>   }
> }
> 
> and then
> 
> let dict = new Proxy(new Map(), LazyMapProxyHandler());
> 
> 
> Makes sense?
Declaring a function makes sense though.
Flags: needinfo?(felipc)
(In reply to Abhishek Bhattacharya [:babhishek21] from comment #60)
> > Also, did you manage to run the PropertyListUtils tests as Georg mentioned
> > in comment 21? (using `./mach xpcshell-test`)? If you need help with that
> > let me now.
>
> I tried running them the first time around when I did some major changes. I
> couldn't make heads or tails of the output log. 

One idea to help with this is: you first run this test without your patch applied (i.e., a clean build) to see the normal output, and then run it again with your changes. At the end of the output there should be a clear message saying if your test passed or not, so if you got confused by the log it's likely that there was a syntax error or something like that that broke things. If you have any trouble running it, let us know and we can help further. But it is very important for anyone writing patches to be able to test their changes and verify that they work.


> > @@ +270,5 @@
> > > +   *         Key to be set, retrieved or deleted. Keys are checked for laziness.
> > > +   * @return Returns value of "name" property of target by default. Otherwise returns
> > > +   *         updated target.
> > > +   */
> > > +_lazyHandler : {
> > 
> > Invalid syntax here, I believe because you moved this outside of the
> > BinaryPropertyListReader.prototype object.  But both
> > BinaryPropertyListReader.prototype and XMLPropertyListReader.prototype will
> > use this, so you're correct in having moved this object to the outside
> > scope. But you'll need to update the syntax to make it work. Also, this is
> > not a good location in the file to add this block of code, because it is in
> > between the definition of "function BinaryPropertyListReader" and
> > "BinaryPropertyListReader.prototype". I think adding it to the end of the
> > file is better.
>
> End of file? But what about lifetime of the object? Shouldn't it be declared
> at the top of the file? If I declare a function, then I think the prototype
> must be declared on top.

The position on the file doesn't determine the lifetime of the object. Everything that is in the top-level scope will run when the file is loaded and make the functions, prototypes and objects defined at the same time. The only ordering that matters is this example:

    line 1:  Car.prototype = {
    line 2:    accelerate: function() { ... }
    line 3:  }
    line 4:  
    line 5:  function Car() { ... }

This doesn't work because Car is not a known name at line 1, so you can modify its properties (in this case, "prototype"). You need to have declared the function first.


> > 
> > The proxy is being used twice, but _lazyHandler is a single object that is
> > reused. This means that the two Dicts using this proxy will share the set of
> > _lazyGetters, which is wrong.
>
> Isn't _lazyGetters defined inside our proxy handler. So whenever a new proxy
> binding is established, a new _lazyGetters Set() should be made? Or am I
> wrong?

No, because _lazyHandler is only one object passed by reference in the two calls to "new Proxy(...)", so we only have one copy of _lazyHandler, with one _lazyGetters.
Flags: needinfo?(felipc)
Hi Abhishek,
have you succeeded to check the tests on this?
Or can we help you get unstuck with something?
Flags: needinfo?(babhishek21)
(In reply to Georg Fritzsche [:gfritzsche] from comment #62)
> Hi Abhishek,
> have you succeeded to check the tests on this?
> Or can we help you get unstuck with something?

Sorry I was a bit busy with college. I started looking at it again yesterday. Would probably make a build tonight/tomorrow and test it. I'll ask for help if required. 

BTW do you have any system that checks for syntax validity in files? Building each time is cumbersome.
Flags: needinfo?(babhishek21) → needinfo?(gfritzsche)
If you have |export MOZ_PACKAGE_JSSHELL=1| in your mozconfig, you can use |<objdir>/dist/bin/js --compileonly path/to/js-file|.

The best check though is still to run the test on it.
Flags: needinfo?(gfritzsche)
So I ran all the required XPC-shell tests. All passed. I suggest you take a look at the javadoc comments in PropertyListUtils.jsm (ref: Comment 59) to make them more conformant to Mozilla doc quality standards. Other than that, its good to go on the try server.

BTW, how does one apply for try-server access and lowest level commit-access?
And are we supposed to shift to MozReview? [https://reviewboard.mozilla.org/]
Attachment #8567518 - Attachment is obsolete: true
Flags: needinfo?(gfritzsche)
Attachment #8585635 - Flags: review?(gfritzsche)
Attachment #8585635 - Flags: feedback?(felipc)
Comment on attachment 8585635 [details] [diff] [review]
bug_1123309: rev_4: Final Cleanup. Tests passed.

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

Hi Abhishek, thanks for this newest patch. It covered everything that I had mentioned, great! I'll push it to try and if it all works I'll give r+.

It's not necessary to move to ReviewBoard yet, it's still on testing stages.. 

To get commit access to tryserver, you need to file a bug (like this one: bug 1116226). You'll need someone to vouch for you. Just CC me in the bug and I'll do it. More information here: https://www.mozilla.org/en-US/about/governance/policies/commit/

::: toolkit/modules/PropertyListUtils.jsm
@@ +781,5 @@
> +   * @return Returns value of "name" property of target by default. Otherwise returns
> +   *         updated target.
> +   */
> +function LazyMapProxyHandler () {
> +	return {

the only nit is that this block of code ended up using Tabs instead of Spaces for indentation. If you post an updated patch with this fixed it would be awesome, but if results from tryserver come earlier than that, I can fix it when landing.
Attachment #8585635 - Flags: feedback?(felipc) → feedback+
(In reply to :Felipe Gomes from comment #67)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=08917eab4117

Its busted in a few places (Linux debug, Android 4.0 and B2G Windows & ICS). I wonder. 

(In reply to :Felipe Gomes from comment #66)
> ::: toolkit/modules/PropertyListUtils.jsm
> @@ +781,5 @@
> > +   * @return Returns value of "name" property of target by default. Otherwise returns
> > +   *         updated target.
> > +   */
> > +function LazyMapProxyHandler () {
> > +	return {
> 
> the only nit is that this block of code ended up using Tabs instead of
> Spaces for indentation. If you post an updated patch with this fixed it
> would be awesome, but if results from tryserver come earlier than that, I
> can fix it when landing.
Sublime Text keeps doing that. Its starting to get rather irritating. :P
Flags: needinfo?(felipc)
Actually all those test failures were existing intermittent failures that are unrelated to your patch!
Therefore, landed:

https://hg.mozilla.org/integration/fx-team/rev/1c66ef669a05

Thanks for the contribution and the patience to work on this bug!
Flags: needinfo?(gfritzsche)
Flags: needinfo?(felipc)
(In reply to Abhishek Bhattacharya [:babhishek21] from comment #68)
> Sublime Text keeps doing that. Its starting to get rather irritating. :P

Did you set these options?
	"tab_size": 2,
	"translate_tabs_to_spaces": true
(In reply to Marco Bonardo [::mak] from comment #70)
> (In reply to Abhishek Bhattacharya [:babhishek21] from comment #68)
> > Sublime Text keeps doing that. Its starting to get rather irritating. :P
> 
> Did you set these options?
> 	"tab_size": 2,
> 	"translate_tabs_to_spaces": true

I did. Apparently when Sublime updated to the latest build, my user settings were corrupted (:/). Vim mode got activated and I had to wade through hell to get it back running as I wished.
https://hg.mozilla.org/mozilla-central/rev/1c66ef669a05
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Comment on attachment 8585635 [details] [diff] [review]
bug_1123309: rev_4: Final Cleanup. Tests passed.

(On comment 66 I gave feedback+ and mentioned that everything was fine, I was just waiting on tryserver to mark this r+. So I'm now updating the bug to reflect that)
Attachment #8585635 - Flags: review?(gfritzsche) → review+
Great. Patch landed. This one went on long too (~3 months).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: