1182964 - Use nsTHashTable::Iterator in layout/{style,svg}/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are six occurrences of EnumerateEntries() in layout/{style,svg}/ to be dealt with.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: iter

The only functional change I made was to replace always-returning-PL_DHASH_REMOVE iterator functions with a single Clear() call after the new iterator loop.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16f7d0399c7f

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8632702 [details] [diff] [review]
iter

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

Thank you for helping with this task.

r=me with the missing Clear() calls added.

::: layout/style/FontFaceSet.cpp
@@ +129,5 @@
>  {
>    MOZ_COUNT_DTOR(FontFaceSet);
>  
>    Disconnect();
> +  for (auto it = mLoaders.Iter(); !it.Done(); it.Next()) {

Nit: I've been using |iter| rather than |it| because I think it's a bit more obvious. But I'll let you decide what to go with.

@@ +131,5 @@
>  
>    Disconnect();
> +  for (auto it = mLoaders.Iter(); !it.Done(); it.Next()) {
> +    it.Get()->GetKey()->Cancel();
> +  }

You didn't add a Clear() call here.

::: layout/style/ImageLoader.cpp
@@ +59,5 @@
> +    if (request) {
> +      request->CancelAndForgetObserver(NS_BINDING_ABORTED);
> +    }
> +    image->mRequests.Remove(mDocument);
> +  }

You didn't add a Clear() call here.

::: layout/svg/nsSVGEffects.cpp
@@ +671,5 @@
> +  for (auto it = mObservers.Iter(); !it.Done(); it.Next()) {
> +    observers.AppendElement(it.Get()->GetKey());
> +  }
> +
> +  mObservers.Clear();

Nit: I'd remove the blank line before the Clear() call because this is closely related to the loop. But YMMV.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> ::: layout/style/FontFaceSet.cpp
> @@ +131,5 @@
> >  
> >    Disconnect();
> > +  for (auto it = mLoaders.Iter(); !it.Done(); it.Next()) {
> > +    it.Get()->GetKey()->Cancel();
> > +  }
> 
> You didn't add a Clear() call here.

This one doesn't need a Clear() since we're in the destructor and the table will be cleared automatically.

> ::: layout/style/ImageLoader.cpp
> @@ +59,5 @@
> > +    if (request) {
> > +      request->CancelAndForgetObserver(NS_BINDING_ABORTED);
> > +    }
> > +    image->mRequests.Remove(mDocument);
> > +  }
> 
> You didn't add a Clear() call here.

This one I did miss.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bcc47d215e7

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0bcc47d215e7