1187784 - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are 17 occurrences of EnumerateRead() in these directories. (This excludes the 2 in layout/style/ImageLoader.cpp that khuey is already in the process of removing in bug 1186780; no need to touch them.)

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()).

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Replace nsBaseHashtable::EnumerateRead() calls in

layout/ with iterators. r=dholbert.

This fixes a type bug in CSSVariableDeclarations::MapRuleInfoInto(). The
existing code passes aRuleData->mVariables.get(), which has type
|CSSVariableDeclarations*|, into the |void*| parameter to EnumerateRead(). It
then extracts that in EnumerateVariableForMapRuleInfoInto() via a cast to a
different type, |nsDataHashtable<nsStringHashKey, nsString>*|. It's missing an
intermediate access of CSSVariableDeclarations::mVariables.

It's likely that this hasn't (seemingly) caused problems prior to now because
mVariables is the only field in CSSVariableDeclarations, so
mVariables->mVariables is at the same address as mVariables.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

(fixed the log message)

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8677858 [details] [diff] [review]
(part 8) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

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

Stealing this since it's Variables-related.

::: layout/style/CSSVariableDeclarations.cpp
@@ +162,5 @@
> +                     false);
> +    } else if (value.EqualsLiteral(INHERIT_VALUE) ||
> +               value.EqualsLiteral(UNSET_VALUE)) {
> +      // Values of 'inherit' and 'unset' don't need any handling, since it means
> +      // we just need to keep whatever value is currently in the resolver.

Can you remove these two duplicated lines while moving this code?

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8677854 [details] [diff] [review]
(part 6) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

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

May as well take these too while I'm here.

r=me with the below answered/addressed.

::: layout/base/nsPresShell.cpp
@@ +4400,5 @@
>    // If it does we should release the pointer capture for the elements.
> +  if (aChild) {
> +    for (auto iter = gPointerCaptureList->Iter(); !iter.Done(); iter.Next()) {
> +      nsIPresShell::PointerCaptureInfo* data = iter.UserData();
> +      if (data->mOverrideContent) {

Did you confirm it's safe to remove the data null check?

@@ +4401,5 @@
> +  if (aChild) {
> +    for (auto iter = gPointerCaptureList->Iter(); !iter.Done(); iter.Next()) {
> +      nsIPresShell::PointerCaptureInfo* data = iter.UserData();
> +      if (data->mOverrideContent) {
> +        if (nsIContent* content = static_cast<nsIContent*>(aChild)) {

If you've hoisted the aChild null check out of the loop, I don't think we need this one in here now.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8677855 [details] [diff] [review]
(part 7) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

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

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ -79,5 @@
>  
> -// Enumeration function that cancels all the image requests in our cache
> -static PLDHashOperator
> -CancelImageRequest(const nsAString& aKey,
> -                   nsTreeImageCacheEntry aEntry, void* aData)

I wonder if it was deliberate that aEntry was being passed by value.  (Slightly wasteful refcounting of its members, if not.)

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

Comment 15

[Nicholas Nethercote [inactive]]

> Did you confirm it's safe to remove the data null check?

I was thinking about nsTHashtable::Iter::Get(), which never returns null, but this one can. I'll put the check back.

> @@ +4401,5 @@
> > +  if (aChild) {
> > +    for (auto iter = gPointerCaptureList->Iter(); !iter.Done(); iter.Next()) {
> > +      nsIPresShell::PointerCaptureInfo* data = iter.UserData();
> > +      if (data->mOverrideContent) {
> > +        if (nsIContent* content = static_cast<nsIContent*>(aChild)) {
> 
> If you've hoisted the aChild null check out of the loop, I don't think we
> need this one in here now.

True, and then the two |if|s can be combined with |&&|. Nice.

Comment 16

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=787adaf4c573

Comment 17

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3217a8f5f4a3327b6709d6aeebd3f202602525
Bug 1187784 (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c86fcfbfc1c503b617279efac7d5a48328bdaa8b
Bug 1187784 (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

https://hg.mozilla.org/integration/mozilla-inbound/rev/079e5922519897e7d275558c454ef3b9df8f945c
Bug 1187784 (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

https://hg.mozilla.org/integration/mozilla-inbound/rev/890488d5686905985a5c4336baaa171e129cabe3
Bug 1187784 (part 4) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0feac727d417ace01a33042badafbcda6bbc0dc
Bug 1187784 (part 5) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e17cabd59b3441b5340269b659bfe4217ff1e465
Bug 1187784 (part 6) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c24a4d0cd9d1c4e23785feb276870ee6f1f248dc
Bug 1187784 (part 7) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

https://hg.mozilla.org/integration/mozilla-inbound/rev/588e410c0ddb76ce261c9c0bd344b1ad634fcb32
Bug 1187784 (part 8) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2938e64f6798d3f20f1869428cc7145f5f52507a
Bug 1187784 (part 9) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators. r=heycam.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f3217a8f5f4
https://hg.mozilla.org/mozilla-central/rev/c86fcfbfc1c5
https://hg.mozilla.org/mozilla-central/rev/079e59225198
https://hg.mozilla.org/mozilla-central/rev/890488d56869
https://hg.mozilla.org/mozilla-central/rev/d0feac727d41
https://hg.mozilla.org/mozilla-central/rev/e17cabd59b34
https://hg.mozilla.org/mozilla-central/rev/c24a4d0cd9d1
https://hg.mozilla.org/mozilla-central/rev/588e410c0ddb
https://hg.mozilla.org/mozilla-central/rev/2938e64f6798