1186809 - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are three occurrences of EnumerateRead() in this directory.

A note to the assignee: to preserve existing behaviour, you should probably use
nsBaseHashtable::Iterator::UserData() rather than nsBaseHashtable::Iterator::Data(). (The latter should be used when replacing nsBaseHashtable::Enumerate()).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

(renamed a local variable)

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

Comment 5

[Jan Varga [:janv]]

Comment on attachment 8679795 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

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

::: dom/quota/QuotaManager.cpp
@@ +3964,5 @@
>  
> +    for (auto iter = mGroupInfoPairs.Iter(); !iter.Done(); iter.Next()) {
> +      GroupInfoPair* pair = iter.UserData();
> +
> +      NS_ASSERTION(!iter.Key().IsEmpty(), "Empty key!");

Please convert this to |MOZ_ASSERT(!iter.Key().IsEmpty());| here and below.

@@ +3983,5 @@
> +      }
> +
> +      if (groupUsage > 0) {
> +        QuotaManager* quotaManager = QuotaManager::Get();
> +        NS_ASSERTION(quotaManager, "Shouldn't be null!");

dtto

Comment 6

[Jan Varga [:janv]]

Comment on attachment 8679795 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

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

::: dom/quota/QuotaManager.cpp
@@ -3928,5 @@
>  }
>  
>  // static
>  PLDHashOperator
> -QuotaManager::GetOriginsExceedingGroupLimit(const nsACString& aKey,

And please remove this method from QuotaManager.h too.

Comment 7

[Jan Varga [:janv]]

Comment on attachment 8679796 [details] [diff] [review]
(part 3) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

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

::: dom/quota/QuotaManager.cpp
@@ -3928,5 @@
>  }
>  
> -// static
> -PLDHashOperator
> -QuotaManager::GetAllTemporaryStorageOrigins(const nsACString& aKey,

Remove from QuotaManager.h too.

@@ +3995,5 @@
> +      for (auto iter = mGroupInfoPairs.Iter(); !iter.Done(); iter.Next()) {
> +        GroupInfoPair* pair = iter.UserData();
> +
> +        NS_ASSERTION(!iter.Key().IsEmpty(), "Empty key!");
> +        NS_ASSERTION(pair, "Null pointer!");

MOZ_ASSERT()

Comment 8

[Jan Varga [:janv]]

Comment on attachment 8679794 [details] [diff] [review]
(part 1) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

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

::: dom/quota/QuotaManager.cpp
@@ +2230,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(aLocks.IsEmpty());
>  
> +  struct MOZ_STACK_CLASS GetInactiveOriginInfos final

I would rename this to "Helper" and keep the method name below.

@@ +2236,2 @@
>      static void
> +    Func(nsTArray<RefPtr<OriginInfo>>& aOriginInfos,

I wouldn't change the method name here.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to Jan Varga [:janv] from comment #5)
> Comment on attachment 8679795 [details] [diff] [review]
> (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with
> iterators
> 
> Review of attachment 8679795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/QuotaManager.cpp
> @@ +3964,5 @@
> >  
> > +    for (auto iter = mGroupInfoPairs.Iter(); !iter.Done(); iter.Next()) {
> > +      GroupInfoPair* pair = iter.UserData();
> > +
> > +      NS_ASSERTION(!iter.Key().IsEmpty(), "Empty key!");
> 
> Please convert this to |MOZ_ASSERT(!iter.Key().IsEmpty());| here and below.

I can't tell what you mean by "and below". This is the only such assertion that I changed in this patch. There are a couple of similar assertions that I didn't touch, one in RemoveQuotaCallback() and one in GetAllTemporaryStorageOrigins() -- did you mean those ones?

Comment 10

[Nicholas Nethercote [inactive]]

> > Please convert this to |MOZ_ASSERT(!iter.Key().IsEmpty());| here and below.
> 
> I can't tell what you mean by "and below".

Oh, you probably meant the similar one in part 3, mentioned in comment 7. Ok, done.

Comment 11

[Jan Varga [:janv]]

Comment on attachment 8679795 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators

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

::: dom/quota/QuotaManager.cpp
@@ +3965,5 @@
> +    for (auto iter = mGroupInfoPairs.Iter(); !iter.Done(); iter.Next()) {
> +      GroupInfoPair* pair = iter.UserData();
> +
> +      NS_ASSERTION(!iter.Key().IsEmpty(), "Empty key!");
> +      NS_ASSERTION(pair, "Null pointer!");

Here's the other assertion, the same patch, just one line beneath.

Comment 12

[Nicholas Nethercote [inactive]]

> Here's the other assertion, the same patch, just one line beneath.

Got it, thanks.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1e73d59cb2853d78ab0d9d3078f61fa675535e
Bug 1186809 (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators. r=janv.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb38548f7e3309c0fc3f1e33b899420388f707a8
Bug 1186809 (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators. r=janv.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce07357c5f7e458b0b5cfe1daad8e8eb772f765
Bug 1186809 (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in dom/quota/ with iterators. r=janv.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f1e73d59cb2
https://hg.mozilla.org/mozilla-central/rev/bb38548f7e33
https://hg.mozilla.org/mozilla-central/rev/5ce07357c5f7