Closed
Bug 1187144
Opened 9 years ago
Closed 8 years ago
Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(10 files)
2.64 KB,
patch
|
heycam
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
dholbert
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
3.61 KB,
patch
|
dholbert
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
5.32 KB,
patch
|
dholbert
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
4.45 KB,
patch
|
dholbert
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
3.90 KB,
patch
|
dholbert
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
dholbert
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
3.18 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
3.53 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
8.84 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
Because iterators are so much nicer than enumerate functions. There are 16 occurrences of Enumerate() in this directory. A note to the assignee: to preserve existing behaviour, you should probably use nsBaseHashtable::Iterator::Data() rather than nsBaseHashtable::Iterator::UserData(). (The latter should be used when replacing nsBaseHashtable::EnumerateRead()).
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8682801 -
Flags: review?(cam)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Updated•9 years ago
|
Attachment #8682801 -
Flags: review?(cam) → review+
Assignee | ||
Comment 2•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/95e0c4e57a78a6857907636b88c2f3118249f220 Bug 1187144 (part 1) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=heycam.
Comment 3•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/95e0c4e57a78
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Comment 4•9 years ago
|
||
Whoops, there's more work to be done here.
Assignee | ||
Updated•9 years ago
|
Attachment #8682801 -
Flags: checkin+
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8691144 -
Flags: review?(dholbert)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8691147 -
Flags: review?(dholbert)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8691148 -
Flags: review?(dholbert)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8691150 -
Flags: review?(dholbert)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8691154 -
Flags: review?(dholbert)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8691157 -
Flags: review?(dholbert)
Comment 11•9 years ago
|
||
Comment on attachment 8691147 [details] [diff] [review] (part 2) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators Review of attachment 8691147 [details] [diff] [review]: ----------------------------------------------------------------- Commit message has r=heycam here in & various other patches -- that needs updating to r=dholbert. (I imagine you were intending this for heycam and then discovered he was on vacation? :)) r=me with that, and the following: ::: layout/style/FontFaceSet.cpp @@ +727,5 @@ > // Remove any residual families that have no font entries (i.e., they were > // not defined at all by the updated set of @font-face rules). > + for (auto it = mUserFontSet->mFontFamilies.Iter(); !it.Done(); it.Next()) { > + if (it.Data()->GetFontList().Length() == 0) { > + it.Remove(); I believe we generally prefer IsEmpty() over explicit Length() == 0 checks, to determine if a nsTArray is empty. So, let's make this if (it.Data()->GetFontList().IsEmpty()) {
Attachment #8691147 -
Flags: review?(dholbert) → review+
Comment 12•9 years ago
|
||
Also, one other commit-message nit: it looks like all of the commit messages here are identical aside from part-number:
"Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators"
Ideally it'd be nice if they had something patch-specific in each patch (hinting at what makes that patch distinct from the 6 other ones here). I think you're doing this on a file-by-file basis, so maybe mentioning the filename at the end of the commit message? So e.g. part 2 would be:
> Bug 1187144 (part 2) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators (in FontFaceSet.cpp). r=dholbert
That way, if this patch ends up causing a regression in e.g. font code, and we narrow it to this bug's push, we'll have a slightly better chance at guessing which cset is responsible, as compared to if we just had a pile of 7 commits with identical commit messages.
Comment 13•9 years ago
|
||
Comment on attachment 8691148 [details] [diff] [review] (part 3) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators Review of attachment 8691148 [details] [diff] [review]: ----------------------------------------------------------------- r=me on part 3, with the comment typo addressed (and the variable renamed if you like): ::: layout/style/CounterStyleManager.cpp @@ +2063,5 @@ > + } else { > + if (!style->IsCustomStyle()) { > + toBeRemoved = true; > + } else { > + auto customStyle = static_cast<CustomCounterStyle*>(style.get()); [/me notices you've renamed this variable with s/style/customStyle/, to make way for your other s/aStyle/style/ renaming.] Perhaps consider calling this "custom" instead of "customStyle", for internal consistency within this function -- because, towards the end of this function, we have (in an unrelated scope): CustomCounterStyle* custom = static_cast<CustomCounterStyle*>(style); @@ +2084,5 @@ > + static_cast<CustomCounterStyle*>(style.get())->ResetDependentData(); > + } > + // The object has to be held here so that it will not be released > + // before all pointers that refer to it are reset. It will be > + // released when mTouchSensitiveRegion goes out of scope at the end I think "mTouchSensitiveRegion" is a typo here, right? I don't see any variable by that name nearby. In the current-mozilla-central version of this code, which you moved here, this comment says: "...when the MarkAndCleanData goes out of scope" (not mTouchSensitiveRegion). That also doesn't make sense, because I don't see anything called MarkAndCleanData, either. I'm pretty sure it wants to refer to "InvalidateOldStyleData", which *does* exist in current code, but which you're removing in this patch and replacing with kungFuDeathGrip. So: your comment here should probably be referring to kungFuDeathGrip, not to the bogus "mTouchSensitiveRegion", right?
Attachment #8691148 -
Flags: review?(dholbert) → review+
Comment 14•9 years ago
|
||
Comment on attachment 8691150 [details] [diff] [review] (part 4) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators Review of attachment 8691150 [details] [diff] [review]: ----------------------------------------------------------------- r=me, with the tab-width modeline tweak reverted, or with a convincing explanation for why we should take it. (If it were just bringing this file into consistency with other files, I'd be ok with slipping it into this patch; but it's doing the opposite, as noted below. Similarly, if there were a strong reason to favor 8, I'd be ok doing so here as part of a broader conversion; but I don't see a reason to favor 8 over 2.) ::: layout/style/nsCSSValue.cpp @@ +2482,5 @@ > + for (auto iter = mRequests.Iter(); !iter.Done(); iter.Next()) { > + nsISupports* key = iter.Key(); > + RefPtr<imgRequestProxy>& proxy = iter.Data(); > + > + nsIDocument* doc = static_cast<nsIDocument*>(key); <TANGENT> I'm curious why we bother making the key a nsISupports*-which-gets-static-cast-to-nsIDocument here... Filed bug 1227377 on cleaning that up. </TANGENT> ::: layout/style/nsHTMLCSSStyleSheet.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ Why the change to "tab-width: 8" here (from 2)? grep shows we have 699 files in layout with "tab-width: 2" (what this was previously), and only 40 files with "tab-width: 8". @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */ (Similarly, we have > 3x as many files that use ts=2 as compared to ts=8, in layout at least). (Not sure what this even maps to -- does this control how wide a tab character would render in vim/emacs, if we accidentally ended up inserting one? I don't see any benefit to changing this file to use 8 for that, other than maybe making tab characters more obvious so we can kill them. That doesn't really seem related to the rest of this bug, though.)
Attachment #8691150 -
Flags: review?(dholbert) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8691154 [details] [diff] [review] (part 5) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators Review of attachment 8691154 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following addressed: ::: layout/style/Loader.cpp @@ +606,5 @@ > // start any pending alternates that aren't alternates anymore > if (mSheets) { > LoadDataArray arr(mSheets->mPendingDatas.Count()); > + for (auto iter = mSheets->mPendingDatas.Iter(); !iter.Done(); iter.Next()) { > + SheetLoadData*& data = iter.Data(); Can we drop the "&" here, and just make this "SheetLoadData*" ? "reference to a pointer to a thing" seems unnecessarily complex here. (That may technically be what Data() returns, but we can also just pretend it returns a pointer, I think, without losing anything.) @@ +613,5 @@ > + // Note that we don't want to affect what the selected style set is, > + // so use true for aHasAlternateRel. > + if (data->mLoader->IsAlternate(data->mTitle, true)) { > + continue; > + } No need to bother with "continue" here -- let's just flip the logic of the "if" check so this flows naturally. (This logic structure made sense in the old version when we had PL_DHASH_NEXT instead of "continue", but now that we've got a "for" loop, we can just make the continuing implicit.) So let's do: for(...) { ... if (!data->mLoader->IsAlternate(data->mTitle, true)) { arr.AppendElement(data); iter.Remove(); } }
Attachment #8691154 -
Flags: review?(dholbert) → review+
Comment 16•9 years ago
|
||
Comment on attachment 8691157 [details] [diff] [review] (part 6) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators Review of attachment 8691157 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following nits addressed: ::: layout/style/Loader.cpp @@ +2484,3 @@ > { > + for (auto iter = aDatas.Iter(); !iter.Done(); iter.Next()) { > + SheetLoadData*& data = iter.Data(); As above, I'd marginally prefer SheetLoadData* here (drop the "&"), for simplicity. It looks like the code you're replacing did use "SheetLoadData*& aData", but that's likely just because we had to, to match the function-signature that Enumerate() expects for its function-pointer arg. (It wasn't because "SheetLoadData*&" is a particularly useful type to be working with here.) @@ +2484,4 @@ > { > + for (auto iter = aDatas.Iter(); !iter.Done(); iter.Next()) { > + SheetLoadData*& data = iter.Data(); > + NS_PRECONDITION(data, "Must have a data!"); Let's convert this to a MOZ_ASSERT (like you did for a similar precondition in the previous patch), since: (1) if this assertion fails, we're about to crash anyway; so we might as well abort with a useful message (in debug builds) (2) NS_PRECONDITION is only supposed to be at the beginning of functions, and this is now in the middle of a function.
Attachment #8691157 -
Flags: review?(dholbert) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8691144 [details] [diff] [review] (part 7) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators Review of attachment 8691144 [details] [diff] [review]: ----------------------------------------------------------------- Part 7 looks good; r=me (Note that this patch is one of the ones with r=heycam in the commit message -- that needs tweaking to r=dholbert)
Attachment #8691144 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 18•9 years ago
|
||
> r=me, with the tab-width modeline tweak reverted, or with a convincing > explanation for why we should take it. > > (If it were just bringing this file into consistency with other files, I'd > be ok with slipping it into this patch; but it's doing the opposite, as > noted below. Similarly, if there were a strong reason to favor 8, I'd be ok > doing so here as part of a broader conversion; but I don't see a reason to > favor 8 over 2.) Because the style guide says to do it that way. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line > (Not sure what this even maps to -- does this control how wide a tab > character would render in vim/emacs, if we accidentally ended up inserting > one? I don't see any benefit to changing this file to use 8 for that, other > than maybe making tab characters more obvious so we can kill them. Yes, that's the reason. > That doesn't really seem related to the rest of this bug, though.) I often fix modelines in files that I'm modifying. Sometimes reviewers object and in those cases I undo the change. I'll do that here.
Comment 19•9 years ago
|
||
Ah, thanks for that clarification -- I was guessing it was an adding-my-standard-vim-modeline-so-i-can-edit-this-file-(and-making-the-emacs-modeline-match-it-on-this-subtle-point-for-consistency) thing, which made it seem a bit arbitrary. I'm OK with that modeline tweak here, then, if you'd like to keep it / put it back in.
Assignee | ||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e323f3987aa6a0a9e1324ebfaa2d486c1f3c558b Bug 1187144 (part 2) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=dholbert. https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8bf590fd8d20e374ddf29d135f02d0481ed2e0 Bug 1187144 (part 3) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=dholbert. https://hg.mozilla.org/integration/mozilla-inbound/rev/ff74d6705fac88f02a3a154c4098e17f64947b99 Bug 1187144 (part 4) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=dholbert. https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0dcc343f0ecd9c377ec2af57701eafcaa219f8 Bug 1187144 (part 5) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=dholbert. https://hg.mozilla.org/integration/mozilla-inbound/rev/401a371009a848d45a9e996fabd4b5361c867efd Bug 1187144 (part 6) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=dholbert. https://hg.mozilla.org/integration/mozilla-inbound/rev/3d03fbb04ef9e1031a713a3e67ac3f85e2c485e2 Bug 1187144 (part 7) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=dholbert.
Assignee | ||
Updated•9 years ago
|
Attachment #8691144 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8691147 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8691148 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8691150 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8691154 -
Flags: checkin+
Assignee | ||
Updated•9 years ago
|
Attachment #8691157 -
Flags: checkin+
Comment 21•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e323f3987aa6 https://hg.mozilla.org/mozilla-central/rev/0e8bf590fd8d https://hg.mozilla.org/mozilla-central/rev/ff74d6705fac https://hg.mozilla.org/mozilla-central/rev/0f0dcc343f0e https://hg.mozilla.org/mozilla-central/rev/401a371009a8 https://hg.mozilla.org/mozilla-central/rev/3d03fbb04ef9
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8711544 -
Flags: review?(cam)
Assignee | ||
Comment 23•8 years ago
|
||
Attachment #8711545 -
Flags: review?(cam)
Assignee | ||
Comment 24•8 years ago
|
||
Attachment #8711546 -
Flags: review?(cam)
Updated•8 years ago
|
Attachment #8711544 -
Flags: review?(cam) → review+
Updated•8 years ago
|
Attachment #8711545 -
Flags: review?(cam) → review+
Comment 25•8 years ago
|
||
Comment on attachment 8711546 [details] [diff] [review] (part 10) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators Review of attachment 8711546 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleTracker.cpp @@ +289,5 @@ > + uint32_t count = 0; > +#endif > + for (auto iter = mPendingRestyles.Iter(); !iter.Done(); iter.Next()) { > + auto element = static_cast<dom::Element*>(iter.Key()); > + nsAutoPtr<RestyleTracker::RestyleData>& aData = iter.Data(); Probably clearer just to turn that into: RestyleTracker::RestyleData* data = iter.Data(); (And then reword the comments later in the function to refer to "data" rather than "aData".) @@ +297,5 @@ > + // It's important to not mess with the flags on entries not in our > + // document. > + if (element->GetCrossShadowCurrentDoc() != Document() || > + !element->HasFlag(RestyleBit())) { > + LOG_RESTYLE_IF(this, true, LOG_RESTYLE_IF(this, true, ...) can become LOG_RESTYLE(...) now we're back in a RestyleTracker member function. @@ +298,5 @@ > + // document. > + if (element->GetCrossShadowCurrentDoc() != Document() || > + !element->HasFlag(RestyleBit())) { > + LOG_RESTYLE_IF(this, true, > + "skipping pending restyle %s, already restyled or no longer " Can you rewrap this to fit 80 columns? @@ +307,5 @@ > + NS_ASSERTION(!element->HasFlag(RootBit()) || > + // Maybe we're just not reachable via the frame tree? > + (element->GetFlattenedTreeParent() && > + (!element->GetFlattenedTreeParent()->GetPrimaryFrame() || > + element->GetFlattenedTreeParent()->GetPrimaryFrame()->IsLeaf() || Here too. Feel free to reindent the entire assertion to: NS_ASSERT( !element->HasFlag(...) || ...
Attachment #8711546 -
Flags: review?(cam) → review+
Assignee | ||
Comment 26•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/893ab0c0e8ed49b47c891d141ad0e9e3c3cec897 Bug 1187144 (part 8) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/275fc8ed3d86030d648262b662b3d0b12195221a Bug 1187144 (part 9) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=heycam. https://hg.mozilla.org/integration/mozilla-inbound/rev/bf3bf9b99f51caa73938e25758b97f0479f6660a Bug 1187144 (part 10) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators. r=heycam.
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/893ab0c0e8ed https://hg.mozilla.org/mozilla-central/rev/275fc8ed3d86 https://hg.mozilla.org/mozilla-central/rev/bf3bf9b99f51
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/893ab0c0e8ed https://hg.mozilla.org/mozilla-central/rev/275fc8ed3d86 https://hg.mozilla.org/mozilla-central/rev/bf3bf9b99f51
You need to log in
before you can comment on or make changes to this bug.
Description
•