Closed
Bug 1187784
Opened 9 years ago
Closed 9 years ago
Replace nsBaseHashtable::EnumerateRead() calls in layout/ with iterators
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(9 files, 2 obsolete files)
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 |
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•9 years ago
|
||
Attachment #8677849 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8677850 -
Flags: review?(cam)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8677851 -
Flags: review?(cam)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8677852 -
Flags: review?(cam)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8677853 -
Flags: review?(cam)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8677854 -
Flags: review?(dholbert)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8677855 -
Flags: review?(dholbert)
Assignee | ||
Comment 8•9 years ago
|
||
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•9 years ago
|
||
(fixed the log message)
Attachment #8677858 -
Flags: review?(dholbert)
Assignee | ||
Updated•9 years ago
|
Attachment #8677857 -
Attachment is obsolete: true
Attachment #8677857 -
Flags: review?(dholbert)
Updated•9 years ago
|
Attachment #8677849 -
Flags: review?(cam) → review+
Updated•9 years ago
|
Attachment #8677850 -
Flags: review?(cam) → review+
Updated•9 years ago
|
Attachment #8677851 -
Flags: review?(cam) → review+
Updated•9 years ago
|
Attachment #8677853 -
Flags: review?(cam) → review+
Updated•9 years ago
|
Attachment #8677852 -
Flags: review?(cam) → review+
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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 12•9 years ago
|
||
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•9 years ago
|
||
Attachment #8677869 -
Flags: review?(cam)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8677870 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Attachment #8677869 -
Attachment is obsolete: true
Attachment #8677869 -
Flags: review?(cam)
Updated•9 years ago
|
Attachment #8677870 -
Flags: review?(cam) → review+
Assignee | ||
Comment 15•9 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 16•9 years ago
|
||
Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=787adaf4c573
Assignee | ||
Comment 17•9 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.
Comment 18•9 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in
before you can comment on or make changes to this bug.
Description
•