1127084 - Remove __iterator__ from ContentPrefStore

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Tom S [:evilpie]]

Attachment: fix-contentprefstore

Not sure if this patch works/applies, hopefully somebody can pick this up.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Remove __iterator__ from ContentPrefStore.

Passed the try run with minor change :)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5872dfe3dbe6

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8563611 [details] [diff] [review]
Remove __iterator__ from ContentPrefStore.

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

It looks good overall, but there are still a couple things to fix. it's very very close to be ready.

::: toolkit/components/contentprefs/ContentPrefStore.jsm
@@ +41,5 @@
>    },
>  
>    get: function CPS_get(group, name) {
>      if (group) {
> +      if (this._groups.has(group))

These 2 ifs can now be merged

@@ +52,5 @@
>      if (group) {
> +      if (this._groups.has(group)) {
> +        this._groups.get(group).delete(name);
> +        if (this._groups.size == 0)
> +          this._groups.delete(group);

This looks wrong, it should be:

if (this._groups.get(group).size == 0)

@@ +80,3 @@
>    },
>  
> +  [Symbol.iterator]: function* () {

nit:
* [Symbol.iterator]() {

@@ +89,5 @@
>        yield [null, name, val];
>      }
>    },
>  
> +  match: function* CPS_match(group, name, includeSubdomains) {

nit: while touching this:
* match(group, name, includeSubdomains) {

@@ +96,5 @@
>          yield [sgroup, this.get(sgroup, name)];
>      }
>    },
>  
> +  matchGroups: function* CPS_matchGroups(group, includeSubdomains) {

nit: while touching this:
* matchGroups(group, includeSubdomains) {

@@ +104,1 @@
>            if (sgroup) {

this looks different.
Before it was iterating "this", that means it was getting [group, name, val] or (null, name, val) tuples,
now you are iterating this._groups, that gives you [ group, Map() ]

This code is confusing and lacks comments, to say the minimum. It might work regardless, indeed iterating "this" may return duplicates and doesn't look like anything is expecting them.
Though if you keep iterating _groups, you need to fix for (let [sgroup, ]) and remove the if (sgroup) check.

Please verify if what I said sounds right to you.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Remove __iterator__ from ContentPrefStore.

Thank you for reviewing!
Fixed commented lines.

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #2)
> @@ +104,1 @@
> >            if (sgroup) {
> 
> this looks different.
> Before it was iterating "this", that means it was getting [group, name, val]
> or (null, name, val) tuples,
> now you are iterating this._groups, that gives you [ group, Map() ]
> 
> This code is confusing and lacks comments, to say the minimum. It might work
> regardless, indeed iterating "this" may return duplicates and doesn't look
> like anything is expecting them.
> Though if you keep iterating _groups, you need to fix for (let [sgroup, ])
> and remove the if (sgroup) check.
> 
> Please verify if what I said sounds right to you.

Reverted the change to iterate over `this` (of course with for-of), to keep same behavior.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e517370b58

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8564580 [details] [diff] [review]
Remove __iterator__ from ContentPrefStore.

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

LGTM!
please remember to add the review tag to the commit message before setting the checkin-needed keyword.

Comment 5

[Tooru Fujisawa [:arai]]

Thank you! :D
https://hg.mozilla.org/integration/mozilla-inbound/rev/31d67d0ed3cb

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31d67d0ed3cb