Closed Bug 621564 Opened 11 years ago Closed 10 years ago

Use Map in nsContentPrefService's cache

Categories

(Firefox :: General, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: Felipe, Assigned: andreshm)

References

Details

(Whiteboard: [good first bug][mentor=felipe][lang=js])

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 559992
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=felipe]
Whiteboard: [good first bug][mentor=felipe] → [good first bug][mentor=felipe][lang=js]
I'll take a look at this one.
Assignee: nobody → andres
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Tests are running fine.
Attachment #643926 - Flags: review?(felipc)
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)
s/_inPrivBrowsingMode/_privModeStorage/
Attached patch Patch v2 (obsolete) — Splinter Review
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?
Attachment #643926 - Attachment is obsolete: true
Attachment #643926 - Flags: review?(felipc)
Attachment #648060 - Flags: review?(felipc)
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)
Attachment #648060 - Flags: review?(felipc) → review+
Attached patch Patch v3Splinter Review
Patch updated to use Map instead of Dict.
Attachment #648060 - Attachment is obsolete: true
Attachment #648727 - Flags: review?(felipc)
Summary: Use Dict.jsm in nsContentPrefService's cache → Use Map in nsContentPrefService's cache
Comment on attachment 648727 [details] [diff] [review]
Patch v3

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

Great, thanks Andres!
Attachment #648727 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/d19bc0ab5a98
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.