1187151 - Replace nsBaseHashtable::Enumerate() calls in dom/ 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 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()).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Replace nsBaseHashtable::Enumerate() calls in dom/base/ with iterators

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Replace nsBaseHashtable::Enumerate() calls in dom/base/ with iterators

Might as well do this now while bug 1186809 is fresh in our minds.

Comment 3

[Jan Varga [:janv]]

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!

Comment 4

[Nicholas Nethercote [inactive]]

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

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a90a4d33cbb

Comment 6

[Nicholas Nethercote [inactive]]

Lots more to do here, still.

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15d02bac344356a36d1097969f0853159a379609
Bug 1187151 (part 2) - Replace nsBaseHashtable::Enumerate() calls in dom/base/ with iterators. r=janv.

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/15d02bac3443

Comment 9

[Nicholas Nethercote [inactive]]

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

Comment 10

[Nicholas Nethercote [inactive]]

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

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

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

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.

Comment 13

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

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

Comment 14

[Nicholas Nethercote [inactive]]

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.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/50ed3852d003
https://hg.mozilla.org/mozilla-central/rev/f2c4d5ae4fd0
https://hg.mozilla.org/mozilla-central/rev/c336a29d0318

Comment 16

[Nicholas Nethercote [inactive]]

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

Comment 17

[Nicholas Nethercote [inactive]]

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

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: (part 12) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 23

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

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.

Comment 24

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

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

Comment 25

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

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.

Comment 26

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

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.

Comment 27

[Nicholas Nethercote [inactive]]

> > +      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())) {

Comment 28

[Nicholas Nethercote [inactive]]

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.

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: (part 13) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: (part 14) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 31

[Nicholas Nethercote [inactive]]

Attachment: (part 15) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 32

[Nicholas Nethercote [inactive]]

Attachment: (part 16) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 33

[Nicholas Nethercote [inactive]]

Attachment: (part 17) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 34

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

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.

Comment 35

[Nicholas Nethercote [inactive]]

Attachment: (part 18) - Replace nsBaseHashtable::Enumerate() calls in dom/ with iterators

Comment 36

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

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.

Comment 37

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

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?

Comment 38

[Nicholas Nethercote [inactive]]

> ::: 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.

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/523744a3dd21
https://hg.mozilla.org/mozilla-central/rev/b278a9bbdd9f
https://hg.mozilla.org/mozilla-central/rev/55fbc715ea6f
https://hg.mozilla.org/mozilla-central/rev/3627ce5ee064
https://hg.mozilla.org/mozilla-central/rev/e98f8768c722
https://hg.mozilla.org/mozilla-central/rev/9a3703f27c25
https://hg.mozilla.org/mozilla-central/rev/6100ca864e53

Comment 40

[Nicholas Nethercote [inactive]]

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.

Comment 41

[Nicholas Nethercote [inactive]]

Thank you for the fast reviews.

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76a118e5e55f
https://hg.mozilla.org/mozilla-central/rev/ff540453161f
https://hg.mozilla.org/mozilla-central/rev/551b83ec95b6
https://hg.mozilla.org/mozilla-central/rev/92ff760423d5
https://hg.mozilla.org/mozilla-central/rev/5a8c26a2a8b1
https://hg.mozilla.org/mozilla-central/rev/1be563571842