Closed Bug 1187151 Opened 4 years ago Closed 4 years ago

Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
firefox47 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(18 files)

1.69 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.16 KB, patch
janv
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.15 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
1.56 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.39 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.01 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.55 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.95 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.15 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.21 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.04 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
4.97 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.56 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.86 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
4.06 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
4.74 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
2.54 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
9.28 KB, patch
baku
: review+
njn
: checkin+
Details | Diff | Splinter Review
Because iterators are so much nicer than enumerate functions.

There are 48 occurrences of Enumerate() in this directory.

A note to the assignee: to preserve existing behaviour, you should probably use
nsBaseHashtable::Iterator::Data() rather than nsBaseHashtable::Iterator::UserData(). (The latter should be used when replacing nsBaseHashtable::EnumerateRead()).
Depends on: 1205351
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Might as well do this now while bug 1186809 is fresh in our minds.
Attachment #8685655 - Flags: review?(Jan.Varga)
Comment on attachment 8685655 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::Enumerate() calls in dom/base/ with iterators

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

Thanks!
Attachment #8685655 - Flags: review?(Jan.Varga) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a90a4d33cbb2980b632eaae6e16a1f93fca533f
Bug 1187151 (part 1) - Replace nsBaseHashtable::Enumerate() calls in dom/base/ with iterators. r=khuey.
https://hg.mozilla.org/mozilla-central/rev/1a90a4d33cbb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Lots more to do here, still.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/15d02bac344356a36d1097969f0853159a379609
Bug 1187151 (part 2) - Replace nsBaseHashtable::Enumerate() calls in dom/base/ with iterators. r=janv.
Attachment #8682810 - Flags: checkin+
Attachment #8685655 - Flags: checkin+
Comment on attachment 8691215 [details] [diff] [review]
(part 5) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/indexedDB/ActorsParent.cpp
@@ +15905,5 @@
>    MOZ_ASSERT(!mInvalidated);
>    mInvalidated = true;
>  
> +  for (auto iter = mFileInfos.Iter(); !iter.Done(); iter.Next()) {
> +    FileInfo*& info = iter.Data();

There's really no reason for this to be a reference.
Attachment #8691215 - Flags: review?(khuey) → review+
Comment on attachment 8691216 [details] [diff] [review]
(part 3) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/indexedDB/IDBDatabase.cpp
@@ +1026,5 @@
>    MOZ_ASSERT(aFileActor);
>  
> +  for (auto iter = mFileActors.Iter(); !iter.Done(); iter.Next()) {
> +    MOZ_ASSERT(iter.Key());
> +    PBackgroundIDBDatabaseFileChild*& actor = iter.Data();

here too

@@ +1153,5 @@
>  
> +  if (mBackgroundActor && mFileActors.Count()) {
> +    for (auto iter = mFileActors.Iter(); !iter.Done(); iter.Next()) {
> +      nsISupports* key = iter.Key();
> +      PBackgroundIDBDatabaseFileChild*& actor = iter.Data();

and here
Attachment #8691216 - Flags: review?(khuey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/50ed3852d0030edda16a5de6abb6d50d52d53a18
Bug 1187151 (part 3) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c4d5ae4fd0ffb9bb90c4cb65e480a0a728967d
Bug 1187151 (part 4) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c336a29d0318f730be6cd814586371d116e52241
Bug 1187151 (part 5) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.
Attachment #8691211 - Flags: checkin+
Attachment #8691215 - Flags: checkin+
Attachment #8691216 - Flags: checkin+
Comment on attachment 8712498 [details] [diff] [review]
(part 6) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/datastore/DataStoreService.cpp
@@ +174,5 @@
> +        DeleteDatabase(info->mName, info->mManifestURL);
> +        iter2.Remove();
> +      }
> +    }
> +    if (apps->Count() == 0) {

nit: \n before the secon if please.
Attachment #8712498 - Flags: review?(khuey) → review+
Comment on attachment 8712499 [details] [diff] [review]
(part 7) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/html/HTMLFormElement.cpp
@@ +1499,5 @@
>    // the past names map.
>    if (aRemoveReason == ElementRemoved) {
>      uint32_t oldCount = mPastNameLookupTable.Count();
> +    for (auto iter = mPastNameLookupTable.Iter(); !iter.Done(); iter.Next()) {
> +      if (static_cast<void*>(aElement) == iter.Data()) {

Can you do if (SameCOMIdentity(aElement, iter.Data())?  I would sleep better at night.

@@ +2525,5 @@
>    // If the element is being removed from the form, we have to remove it from
>    // the past names map.
>    if (aRemoveReason == ElementRemoved) {
> +    for (auto iter = mPastNameLookupTable.Iter(); !iter.Done(); iter.Next()) {
> +      if (static_cast<void*>(aElement) == iter.Data()) {

ibid
Attachment #8712499 - Flags: review?(khuey) → review+
Comment on attachment 8712502 [details] [diff] [review]
(part 10) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/xul/nsXULPrototypeCache.cpp
@@ +594,5 @@
>  nsXULPrototypeCache::MarkInGC(JSTracer* aTrc)
>  {
> +    for (auto iter = mScriptTable.Iter(); !iter.Done(); iter.Next()) {
> +        JS::Heap<JSScript*>& script = iter.Data();
> +        JS::TraceEdge(aTrc, &script, "nsXULPrototypeCache script");

I don't know if aliasing script locally here is worth it, but I don't care much.
Attachment #8712502 - Flags: review?(khuey) → review+
Comment on attachment 8712503 [details] [diff] [review]
(part 11) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/xul/templates/nsXULTemplateBuilder.cpp
@@ +105,5 @@
>  }
>  
> +static void
> +DestroyMatchMap(
> +    nsDataHashtable<nsISupportsHashKey, nsTemplateMatch*>& aMatchMap)

I kinda feel like this should become a member function.
Attachment #8712503 - Flags: review?(khuey) → review+
> > +      if (static_cast<void*>(aElement) == iter.Data()) {
> 
> Can you do if (SameCOMIdentity(aElement, iter.Data())?  I would sleep better
> at night.

That gives me awful compile errors:

> 0:19.17 /home/njn/moz/mi2/dom/html/HTMLFormElement.cpp:1503:27: error: ambiguous conversion from derived class 'nsGenericHTMLFormElement' to base class 'nsISupports':
> 0:19.17     class nsGenericHTMLFormElement -> class nsGenericHTMLElement -> nsGenericHTMLElementBase -> nsMappedAttributeElementBase -> class nsStyledElementNotElementCSSInlineStyle -> nsStyledElementBase -> class mozilla::dom::FragmentOrElement -> class nsIContent -> class nsINode -> mozilla::dom:
> 0:19.17     class nsGenericHTMLFormElement -> class nsGenericHTMLElement -> class nsIDOMHTMLElement -> class nsIDOMElement -> class nsIDOMNode -> class nsISupports
> 0:19.17     class nsGenericHTMLFormElement -> class nsIFormControl -> class nsISupports
> 0:19.18       if (SameCOMIdentity(aElement, iter.Data())) {
> 0:19.18                           ^~~~~~~~
> 0:19.18 /home/njn/moz/mi2/dom/html/HTMLFormElement.cpp:2529:27: error: ambiguous conversion from derived class 'mozilla::dom::HTMLImageElement' to base class 'nsISupports':
> 0:19.18     class mozilla::dom::HTMLImageElement -> class nsGenericHTMLElement -> nsGenericHTMLElementBase -> nsMappedAttributeElementBase -> class nsStyledElementNotElementCSSInlineStyle -> nsStyledElementBase -> class mozilla::dom::FragmentOrElement -> class nsIContent -> class nsINode -> mozilla
> 0:19.18     class mozilla::dom::HTMLImageElement -> class nsGenericHTMLElement -> class nsIDOMHTMLElement -> class nsIDOMElement -> class nsIDOMNode -> class nsISupports
> 0:19.18     class mozilla::dom::HTMLImageElement -> class nsImageLoadingContent -> class nsIImageLoadingContent -> class imgINotificationObserver -> class nsISupports
> 0:19.18     class mozilla::dom::HTMLImageElement -> class nsImageLoadingContent -> class imgIOnloadBlocker -> class nsISupports
> 0:19.18     class mozilla::dom::HTMLImageElement -> class nsIDOMHTMLImageElement -> class nsISupports
 0:19.18       if (SameCOMIdentity(aElement, iter.Data())) {
https://hg.mozilla.org/integration/mozilla-inbound/rev/523744a3dd21bfc9a0a5f27e609fb1abf4ba77e6
Bug 1187151 (part 6) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b278a9bbdd9f63d427e7187a70ae1bedb975ab86
Bug 1187151 (part 7) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/55fbc715ea6fc438356dd84f8064926fed11591e
Bug 1187151 (part 8) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3627ce5ee0648a84553d219a3ce64bf7bb026ff3
Bug 1187151 (part 9) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e98f8768c7225c86c622bde5b01038567ed8cf04
Bug 1187151 (part 10) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a3703f27c25fcc34694407ba4a9fa05f08d40dc
Bug 1187151 (part 11) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6100ca864e53fa93b958ffa420abdbf0fd7557b5
Bug 1187151 (part 12) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=khuey.
Attachment #8712498 - Flags: checkin+
Attachment #8712499 - Flags: checkin+
Attachment #8712500 - Flags: checkin+
Attachment #8712501 - Flags: checkin+
Attachment #8712502 - Flags: checkin+
Attachment #8712503 - Flags: checkin+
Attachment #8712505 - Flags: checkin+
Comment on attachment 8713455 [details] [diff] [review]
(part 15) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/base/nsFrameMessageManager.cpp
@@ +1633,5 @@
>      NS_ASSERTION(sCachedScripts != nullptr, "Need cached scripts");
> +    for (auto iter = sCachedScripts->Iter(); !iter.Done(); iter.Next()) {
> +      delete iter.Data();
> +      iter.Remove();
> +    }

You could just iter.Clear() afterwards.  Or even let sCachedScript's dtor take care of it.
Attachment #8713455 - Flags: review?(amarchesini) → review+
Comment on attachment 8713457 [details] [diff] [review]
(part 16) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/bluetooth/common/BluetoothService.cpp
@@ +307,5 @@
> +    ol->RemoveObserver(aHandler);
> +    // We shouldn't have duplicate instances in the ObserverList, but there's
> +    // no appropriate way to do duplication check while registering, so
> +    // assertions are added here.
> +    MOZ_ASSERT(!ol->RemoveObserver(aHandler));

Yuck.
Attachment #8713457 - Flags: review?(continuation) → review+
Comment on attachment 8713459 [details] [diff] [review]
(part 17) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

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

::: dom/base/nsDOMAttributeMap.cpp
@@ +47,5 @@
>  {
> +  for (auto iter = mAttributeCache.Iter(); !iter.Done(); iter.Next()) {
> +    iter.Data()->SetMap(nullptr);
> +    iter.Remove();
> +  }

Just clear the table afterwards?
Attachment #8713459 - Flags: review?(continuation) → review+
> ::: dom/base/nsFrameMessageManager.cpp
> @@ +1633,5 @@
> >      NS_ASSERTION(sCachedScripts != nullptr, "Need cached scripts");
> > +    for (auto iter = sCachedScripts->Iter(); !iter.Done(); iter.Next()) {
> > +      delete iter.Data();
> > +      iter.Remove();
> > +    }
> 
> You could just iter.Clear() afterwards.  Or even let sCachedScript's dtor
> take care of it.

I think I'll leave it as is. I like how this code minimizes the amount of time the value remains in the table once deleted. Same with comment 37.
Attachment #8713464 - Flags: review?(continuation) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/76a118e5e55f00126d70ee6197b638a09bf5d797
Bug 1187151 (part 13) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=baku.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff540453161fc931e5d7a403e98ea88571ff4d26
Bug 1187151 (part 14) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=baku.

https://hg.mozilla.org/integration/mozilla-inbound/rev/551b83ec95b69f89eca66a1491fbec3f9a2fc2a2
Bug 1187151 (part 15) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=baku.

https://hg.mozilla.org/integration/mozilla-inbound/rev/92ff760423d5d1ea280edb6367e2d243f6a7b68b
Bug 1187151 (part 16) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=mccr8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8c26a2a8b165a7539123b834f4dabf8764adf7
Bug 1187151 (part 17) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=mccr8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1be563571842c2114978990de2db75aee7e36304
Bug 1187151 (part 18) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators. r=mccr8.
Attachment #8713452 - Flags: checkin+
Attachment #8713453 - Flags: checkin+
Attachment #8713455 - Flags: checkin+
Attachment #8713457 - Flags: checkin+
Attachment #8713459 - Flags: checkin+
Attachment #8713464 - Flags: checkin+
Thank you for the fast reviews.
Keywords: leave-open
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.