Closed Bug 1187116 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

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

References

Details

Attachments

(6 files, 5 obsolete files)

2.76 KB, patch
khuey
: review+
n.nethercote
: checkin+
Details | Diff | Splinter Review
3.29 KB, patch
khuey
: review+
n.nethercote
: checkin+
Details | Diff | Splinter Review
3.14 KB, patch
khuey
: review+
n.nethercote
: checkin+
Details | Diff | Splinter Review
3.76 KB, patch
khuey
: review+
n.nethercote
: checkin+
Details | Diff | Splinter Review
3.54 KB, patch
khuey
: review+
Details | Diff | Splinter Review
8.02 KB, patch
khuey
: review+
Details | Diff | Splinter Review
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()).
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #8681776 - Attachment is obsolete: true
Attachment #8681776 - Flags: review?(khuey)
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.
Attachment #8681774 - Flags: review?(khuey) → review+
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.
Attachment #8681775 - Flags: review?(khuey) → review-
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
Attachment #8681777 - Flags: review?(khuey) → review-
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.
Attachment #8681778 - Flags: review?(khuey) → review-
Attachment #8681775 - Attachment is obsolete: true
Attachment #8681777 - Attachment is obsolete: true
Attachment #8681778 - Attachment is obsolete: true
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.
Oh, there are more to do here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8681774 - Flags: checkin+
Attachment #8683428 - Flags: checkin+
Attachment #8683429 - Flags: checkin+
Attachment #8683432 - Flags: checkin+
Diff for parts 5--7:

 1 file changed, 103 insertions(+), 237 deletions(-)
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.
Attachment #8689326 - Attachment is obsolete: true
Attachment #8689326 - Flags: review?(khuey)
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?
Attachment #8689327 - Flags: review?(khuey) → review+
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?
Attachment #8689328 - Flags: review?(khuey) → review+
> 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.
> 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.
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.
You need to log in before you can comment on or make changes to this bug.