Closed Bug 1182964 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: njn, Assigned: heycam)

References

Details

Attachments

(1 file)

Because iterators are so much nicer than enumerate functions.

There are six occurrences of EnumerateEntries() in layout/{style,svg}/ to be dealt with.
Attached patch iterSplinter Review
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
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8632702 - Flags: review?(n.nethercote)
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.
Attachment #8632702 - Flags: review?(n.nethercote) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/0bcc47d215e7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.