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

RESOLVED FIXED in Firefox 44

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(9 attachments, 2 obsolete attachments)

2.56 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.30 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.06 KB, patch
heycam
: review+
Details | Diff | Splinter Review
8.17 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.83 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.93 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.25 KB, patch
heycam
: review+
Details | Diff | Splinter Review
8.85 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.87 KB, patch
heycam
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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()).
(Assignee)

Comment 1

3 years ago
Created attachment 8677849 [details] [diff] [review]
(part 1) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677849 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8677850 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677850 - Flags: review?(cam)
(Assignee)

Comment 3

3 years ago
Created attachment 8677851 [details] [diff] [review]
(part 3) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677851 - Flags: review?(cam)
(Assignee)

Comment 4

3 years ago
Created attachment 8677852 [details] [diff] [review]
(part 4) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677852 - Flags: review?(cam)
(Assignee)

Comment 5

3 years ago
Created attachment 8677853 [details] [diff] [review]
(part 5) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677853 - Flags: review?(cam)
(Assignee)

Comment 6

3 years ago
Created attachment 8677854 [details] [diff] [review]
(part 6) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677854 - Flags: review?(dholbert)
(Assignee)

Comment 7

3 years ago
Created attachment 8677855 [details] [diff] [review]
(part 7) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677855 - Flags: review?(dholbert)
(Assignee)

Comment 8

3 years ago
Created attachment 8677857 [details] [diff] [review]
(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.
Attachment #8677857 - Flags: review?(dholbert)
(Assignee)

Comment 9

3 years ago
Created attachment 8677858 [details] [diff] [review]
(part 8) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators

(fixed the log message)
Attachment #8677858 - Flags: review?(dholbert)
(Assignee)

Updated

3 years ago
Attachment #8677857 - Attachment is obsolete: true
Attachment #8677857 - Flags: review?(dholbert)
Attachment #8677849 - Flags: review?(cam) → review+
Attachment #8677850 - Flags: review?(cam) → review+
Attachment #8677851 - Flags: review?(cam) → review+
Attachment #8677853 - Flags: review?(cam) → review+
Attachment #8677852 - Flags: review?(cam) → review+
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?
Attachment #8677858 - Flags: review?(dholbert) → review+
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.
Attachment #8677854 - Flags: review?(dholbert) → review+
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.)
Attachment #8677855 - Flags: review?(dholbert) → review+
(Assignee)

Comment 13

3 years ago
Created attachment 8677869 [details] [diff] [review]
(part 9) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677869 - Flags: review?(cam)
(Assignee)

Comment 14

3 years ago
Created attachment 8677870 [details] [diff] [review]
(part 9) - Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Attachment #8677870 - Flags: review?(cam)
(Assignee)

Updated

3 years ago
Attachment #8677869 - Attachment is obsolete: true
Attachment #8677869 - Flags: review?(cam)
Attachment #8677870 - Flags: review?(cam) → review+
(Assignee)

Comment 15

3 years ago
> 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.
(Assignee)

Comment 17

3 years ago
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.
You need to log in before you can comment on or make changes to this bug.