Last Comment Bug 621564 - Use Map in nsContentPrefService's cache
: Use Map in nsContentPrefService's cache
Status: RESOLVED FIXED
[good first bug][mentor=felipe][lang=js]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: Firefox 17
Assigned To: Andres Hernandez [:andreshm]
:
Mentors:
Depends on: 559992 621204
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-27 11:22 PST by :Felipe Gomes (needinfo me!) [offline until Jun 24]
Modified: 2012-08-04 11:17 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (5.13 KB, patch)
2012-07-19 10:52 PDT, Andres Hernandez [:andreshm]
no flags Details | Diff | Review
Patch v2 (5.09 KB, patch)
2012-08-01 13:37 PDT, Andres Hernandez [:andreshm]
felipc: review+
Details | Diff | Review
Patch v3 (4.36 KB, patch)
2012-08-03 08:45 PDT, Andres Hernandez [:andreshm]
felipc: review+
Details | Diff | Review

Description :Felipe Gomes (needinfo me!) [offline until Jun 24] 2010-12-27 11:22:47 PST
A useful Dict.jsm was added to comm-central and Bug 621204 will move it to toolkit. We want to use it to improve the contentprefs svc cache. See bug 559992 comment 34.
Comment 1 Andres Hernandez [:andreshm] 2012-07-17 13:59:22 PDT
I'll take a look at this one.
Comment 2 Andres Hernandez [:andreshm] 2012-07-19 10:52:01 PDT
Created attachment 643926 [details] [diff] [review]
Patch v1

Tests are running fine.
Comment 3 :Felipe Gomes (needinfo me!) [offline until Jun 24] 2012-07-30 21:00:05 PDT
Comment on attachment 643926 [details] [diff] [review]
Patch v1

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

Looking good. But there's something that I don't understand completely: I'm a bit worried about how this changes _inPrivBrowsingMode.groupsForName, because it will change the type of the elements in the return array.  The thing is, groupsForName doesn't make any sense to me (I thought 1 group == 1 name).  Also, that function doesn't even use the parameter passed to it!  But I don't understand what its callers are trying to do with it either.  Any thoughts on this?

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +12,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Dict",
> +  "resource://gre/modules/Dict.jsm");
> +

we traditionally align the arguments past the parens at every line on a function call

@@ +603,1 @@
>            }

instead of using .listkeys() which returns an array, you could use the .items generator and simplify as:

for (let [entry, value] of d.items) {
  prefs.setProperty(entry, value);
}

(the .hasOwnProperty check there was to ensure that we weren't getting anything extra coming from the object's prototype, but with the Dict this it's not necessary)
Comment 4 :Felipe Gomes (needinfo me!) [offline until Jun 24] 2012-07-30 21:00:52 PDT
s/_inPrivBrowsingMode/_privModeStorage/
Comment 5 Andres Hernandez [:andreshm] 2012-08-01 13:37:37 PDT
Created attachment 648060 [details] [diff] [review]
Patch v2

Applied changes. 

(In reply to :Felipe Gomes [offline 20-24, slow resp. 25-28] from comment #3) 
> Looking good. But there's something that I don't understand completely: I'm
> a bit worried about how this changes _inPrivBrowsingMode.groupsForName,
> because it will change the type of the elements in the return array.  The
> thing is, groupsForName doesn't make any sense to me (I thought 1 group == 1
> name).  Also, that function doesn't even use the parameter passed to it! 
> But I don't understand what its callers are trying to do with it either. 
> Any thoughts on this?
> 

This patch doesn't affect the groupsForName method, since the patch only changes the object created for each group by a Dict object. The _prefCache object remains the same as an {} object. 

Now, it's true that groupsForName only returns the keys or names in the _prefCache (except for '__GlobalPrefs__') and it's not using the param aName at all. For clarity, I think we can change method name as 'listKeys' or 'getNames', wyt?
Comment 6 :Felipe Gomes (needinfo me!) [offline until Jun 24] 2012-08-01 17:45:19 PDT
Comment on attachment 648060 [details] [diff] [review]
Patch v2

(In reply to Andres Hernandez [:andreshm] from comment #5)
> This patch doesn't affect the groupsForName method, since the patch only
> changes the object created for each group by a Dict object. The _prefCache
> object remains the same as an {} object. 
> 
> Now, it's true that groupsForName only returns the keys or names in the
> _prefCache (except for '__GlobalPrefs__') and it's not using the param aName
> at all. For clarity, I think we can change method name as 'listKeys' or
> 'getNames', wyt?

Oh I thought that groupsForName returned each object (thus would change from returning {} to Dict), but I see it now that it only returns the objects' names, so it doesn't change its behavior.

That function is weird but we don't need to change it here, I just needed some assurance that it wasn't completely broken. Thanks Andres!

---

I'm gonna suggest another change here, if you want to do it: Unfocused just reminded me that now we don't even need to use Dict.jsm since now the JS engine has built-in support for Map (it didn't exist when I filed this bug). Do you wanna change it to that?  They are very similar, .get .set .has all work the same, and .count is .size().

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Map

Although the MDN page doesn't say it yet, iterating also works (implemented in bug 725909), you just need to iterate on the map object itself (no need to use .items)
Comment 7 Andres Hernandez [:andreshm] 2012-08-03 08:45:23 PDT
Created attachment 648727 [details] [diff] [review]
Patch v3

Patch updated to use Map instead of Dict.
Comment 8 :Felipe Gomes (needinfo me!) [offline until Jun 24] 2012-08-03 19:22:42 PDT
Comment on attachment 648727 [details] [diff] [review]
Patch v3

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

Great, thanks Andres!
Comment 9 :Felipe Gomes (needinfo me!) [offline until Jun 24] 2012-08-03 20:44:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d19bc0ab5a98
Comment 10 Ed Morley [:emorley] 2012-08-04 11:17:28 PDT
https://hg.mozilla.org/mozilla-central/rev/d19bc0ab5a98

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