1187116 - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are 11 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/indexedDB/ with iterators

Comment 2

[Nicholas Nethercote [inactive]]

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

Comment 3

[Nicholas Nethercote [inactive]]

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

Comment 4

[Nicholas Nethercote [inactive]]

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

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

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

::: dom/indexedDB/ActorsParent.cpp
@@ +439,5 @@
> +  template <class Enumerable>
> +  static void
> +  MatchHelper(const Enumerable& aEnumerable, SelfType* aClosure)
> +  {
> +    AssertIsOnBackgroundThread();

Assert aClosure too, just to be consistent.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

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

I would prefer that you axe the Helper class entirely here, unless it is used in multiple places.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

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

ibid

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8681778 [details] [diff] [review]
(part 4) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

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

And here.

Comment 10

[Nicholas Nethercote [inactive]]

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

|Helper| is now gone.

Comment 11

[Nicholas Nethercote [inactive]]

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

Now with less |Helper|.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

|Helper| wasn't so helpful.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d10c8fdb13906859625f50e0809af6ef903c689
Bug 1187116 (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/487424631dbd06e8c05295fadb4e21f4665d48fb
Bug 1187116 (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3711aea0b742d90fb877ec209aea56018cf9eeae
Bug 1187116 (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eef525eb807915bb07f1c330cbebfd0484e20b7e
Bug 1187116 (part 4) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators. r=khuey.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d10c8fdb139
https://hg.mozilla.org/mozilla-central/rev/487424631dbd
https://hg.mozilla.org/mozilla-central/rev/3711aea0b742
https://hg.mozilla.org/mozilla-central/rev/eef525eb8079

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d10c8fdb139
https://hg.mozilla.org/mozilla-central/rev/487424631dbd
https://hg.mozilla.org/mozilla-central/rev/3711aea0b742
https://hg.mozilla.org/mozilla-central/rev/eef525eb8079

Comment 16

[Nicholas Nethercote [inactive]]

Oh, there are more to do here.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

Comment 20

[Nicholas Nethercote [inactive]]

Diff for parts 5--7:

 1 file changed, 103 insertions(+), 237 deletions(-)

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 8689326 [details] [diff] [review]
(part 5) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

I updated and rebased and part 5 disappeared because that code got removed elsewhere. I'll renumber parts 6 and 7 before landing, but I won't bother updating the patches here.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8689327 [details] [diff] [review]
(part 6) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

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

::: dom/indexedDB/ActorsParent.cpp
@@ +21093,5 @@
>    MOZ_ASSERT(mMetadata);
>  
> +  aSpec.metadata() = mMetadata->mCommonMetadata;
> +
> +  for (auto iter1 = mMetadata->mObjectStores.ConstIter();

how about osIter or objectStoreIter?

@@ +21104,5 @@
> +    // XXX This should really be fallible...
> +    ObjectStoreSpec* objectStoreSpec = aSpec.objectStores().AppendElement();
> +    objectStoreSpec->metadata() = metadata->mCommonMetadata;
> +
> +    for (auto iter2 = metadata->mIndexes.Iter(); !iter2.Done(); iter2.Next()) {

similarly indexIter?

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8689328 [details] [diff] [review]
(part 7) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators

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

::: dom/indexedDB/ActorsParent.cpp
@@ +21147,5 @@
>    MOZ_ASSERT(thisDB->mNextIndexId <= otherDB->mNextIndexId);
>  
>    MOZ_ASSERT(thisDB->mObjectStores.Count() == otherDB->mObjectStores.Count());
>  
> +  for (auto it1 = thisDB->mObjectStores.ConstIter(); !it1.Done(); it1.Next()) {

is there a reason this isn't iter?

Comment 24

[Nicholas Nethercote [inactive]]

> is there a reason this isn't iter?

I've been shortening |iter| to |it| when necessary to make the loop header fit within 80 chars.

Comment 25

[Nicholas Nethercote [inactive]]

> I've been shortening |iter| to |it| when necessary to make the loop header
> fit within 80 chars.

But I can change it to objectStoreIter/indexIter.

Comment 26

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78211ac0edc87f73f11e32b94c9e373dff508d1f
Bug 1187116 (part 5) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e0af18244ff31a29eaf9a5f3146628cb3ebbbd
Bug 1187116 (part 6) - Replace nsBaseHashtable::EnumerateRead() calls in dom/indexedDB/ with iterators. r=khuey.

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/78211ac0edc8
https://hg.mozilla.org/mozilla-central/rev/c2e0af18244f