1187144 - Replace nsBaseHashtable::Enumerate() 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 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()).

Comment 1

[Nicholas Nethercote [inactive]]

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

Comment 2

[Nicholas Nethercote [inactive]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/95e0c4e57a78

Comment 4

[Nicholas Nethercote [inactive]]

Whoops, there's more work to be done here.

Comment 5

[Nicholas Nethercote [inactive]]

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

Comment 6

[Nicholas Nethercote [inactive]]

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

Comment 7

[Nicholas Nethercote [inactive]]

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

Comment 8

[Nicholas Nethercote [inactive]]

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

Comment 9

[Nicholas Nethercote [inactive]]

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

Comment 10

[Nicholas Nethercote [inactive]]

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

Comment 11

[Daniel Holbert [:dholbert]]

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

Comment 12

[Daniel Holbert [:dholbert]]

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

[Daniel Holbert [:dholbert]]

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?

Comment 14

[Daniel Holbert [:dholbert]]

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

Comment 15

[Daniel Holbert [:dholbert]]

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();
    }
  }

Comment 16

[Daniel Holbert [:dholbert]]

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.

Comment 17

[Daniel Holbert [:dholbert]]

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)

Comment 18

[Nicholas Nethercote [inactive]]

> 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

[Daniel Holbert [:dholbert]]

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.

Comment 20

[Nicholas Nethercote [inactive]]

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.

Comment 21

[Carsten Book [:Tomcat]]

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

Comment 22

[Nicholas Nethercote [inactive]]

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

Comment 23

[Nicholas Nethercote [inactive]]

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

Comment 24

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Replace nsBaseHashtable::Enumerate() calls in layout/ with iterators

Comment 25

[Cameron McCormack (:heycam)]

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

Comment 26

[Nicholas Nethercote [inactive]]

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.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/893ab0c0e8ed
https://hg.mozilla.org/mozilla-central/rev/275fc8ed3d86
https://hg.mozilla.org/mozilla-central/rev/bf3bf9b99f51

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/893ab0c0e8ed
https://hg.mozilla.org/mozilla-central/rev/275fc8ed3d86
https://hg.mozilla.org/mozilla-central/rev/bf3bf9b99f51