621564 - Use Map in nsContentPrefService's cache

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

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

I'll take a look at this one.

Comment 2

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Tests are running fine.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

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 for replies!)]

s/_inPrivBrowsingMode/_privModeStorage/

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: 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 for replies!)]

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

Attachment: Patch v3

Patch updated to use Map instead of Dict.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 648727 [details] [diff] [review]
Patch v3

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

Great, thanks Andres!

Comment 9

[:Felipe Gomes (needinfo for replies!)]

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

Comment 10

[Ed Morley [:emorley]]

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