Closed Bug 1186809 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(3 files, 1 obsolete file)

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()).
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #8679792 - Attachment is obsolete: true
Attachment #8679792 - Flags: review?(Jan.Varga)
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
Attachment #8679795 - Flags: review?(Jan.Varga) → review+
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 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()
Attachment #8679796 - Flags: review?(Jan.Varga) → review+
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.
Attachment #8679794 - Flags: review?(Jan.Varga) → review+
(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?
> > 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 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.
> Here's the other assertion, the same patch, just one line beneath.

Got it, thanks.
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.
https://hg.mozilla.org/mozilla-central/rev/2f1e73d59cb2
https://hg.mozilla.org/mozilla-central/rev/bb38548f7e33
https://hg.mozilla.org/mozilla-central/rev/5ce07357c5f7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.