Use Map in nsContentPrefService's cache

RESOLVED FIXED in Firefox 17

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Felipe, Assigned: andreshm)

Tracking

unspecified
Firefox 17
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Depends on: 559992

Updated

7 years ago
Whiteboard: [good first bug]
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug] → [good first bug][mentor=felipe]

Updated

6 years ago
Whiteboard: [good first bug][mentor=felipe] → [good first bug][mentor=felipe][lang=js]
(Assignee)

Comment 1

5 years ago
I'll take a look at this one.
Assignee: nobody → andres
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 643926 [details] [diff] [review]
Patch v1

Tests are running fine.
Attachment #643926 - Flags: review?(felipc)
(Reporter)

Comment 3

5 years ago
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)
(Reporter)

Comment 4

5 years ago
s/_inPrivBrowsingMode/_privModeStorage/
(Assignee)

Comment 5

5 years ago
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?
Attachment #643926 - Attachment is obsolete: true
Attachment #643926 - Flags: review?(felipc)
Attachment #648060 - Flags: review?(felipc)
(Reporter)

Comment 6

5 years ago
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+
(Assignee)

Comment 7

5 years ago
Created attachment 648727 [details] [diff] [review]
Patch v3

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
(Reporter)

Comment 8

5 years ago
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+
(Reporter)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d19bc0ab5a98
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/d19bc0ab5a98
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.