1182963 - Use nsTHashTable::Iterator in layout/base/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are 21 occurrences of EnumerateEntries() in layout/base/ to be dealt with.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in FrameLayerBuilder. (v1)

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in FramePropertyTable. (v1)

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in MaskLayerImageCache. (v1)

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsPresArena. (v1)

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsRefreshDriver. (v1)

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 8632829 [details] [diff] [review]
Use nsTHashTable::Iterator in FrameLayerBuilder. (v1)

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

Thank you for working on this bug!

One of the good things about converting from enumeration to iteration is that you can put the iterator loop right where you need it, not in a separate function. This saves having to declare the functions, and pass arguments, and it's generally easier to read that way. Sometimes you can't do that, e.g. because the same enumeration/iteration occurs in two different places, or you have issues with access to private members across classes. But I don't think any of those are relevant here.

So I'd like to you get rid of DumpDisplayItemDataForFrame, ProcessRemovedDisplayItems, RestoreDisplayItemData, and RestorePaintedLayerItemEntries by inlining them to their individual call sites. This will reduce the code size further and make it easier to read. I will re-review once you do that. Thank you.

::: layout/base/FrameLayerBuilder.cpp
@@ +1721,3 @@
>      }
> +
> +    ComputeGeometryChangeForItem(data);

Please remove the |continue| and put the ComputeGeometryChangeForItem() call in an |else| block. That control flow is easier to understand.

@@ +1734,3 @@
>  
> +    nsAutoCString prefix;
> +    prefix += aPrefix

You're missing a semicolon here. Have you compiled this patch?

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 8632834 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresArena. (v1)

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

Again, I think inlining the iterator loops directly would make the code shorter and easier to read. And it will allow to you remove EnumerateData.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8632835 [details] [diff] [review]
Use nsTHashTable::Iterator in nsRefreshDriver. (v1)

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

::: layout/base/nsRefreshDriver.cpp
@@ +1894,5 @@
>      // the nearest multiple of the delay and move all the images in this table
>      // to the main requests table.
>      if (prevMultiple != static_cast<uint32_t>(curr.ToMilliseconds()) / aDelay) {
>        parms->mDesired = start + TimeDuration::FromMilliseconds(prevMultiple * aDelay);
> +      BeginRefreshingImages(aData->mEntries, parms);

This is a good example where the iterator loop has to be in its own function, because it's used in more than one place.

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

(In reply to Nicholas Nethercote [:njn] from comment #6)

> So I'd like to you get rid of DumpDisplayItemDataForFrame,
> ProcessRemovedDisplayItems, RestoreDisplayItemData, and
> RestorePaintedLayerItemEntries by inlining them to their individual call
> sites. This will reduce the code size further and make it easier to read. I
> will re-review once you do that. Thank you.

I'll inline those functions in the next patch.

> 
> @@ +1734,3 @@
> >  
> > +    nsAutoCString prefix;
> > +    prefix += aPrefix
> 
> You're missing a semicolon here. Have you compiled this patch?

Ouch. I should #define DEBUG_DISPLAY_ITEM_DATA and build again.

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in FrameLayerBuilder. (v2)

Inline DumpDisplayItemDataForFrame, ProcessRemovedDisplayItems,
RestoreDisplayItemData, and RestorePaintedLayerItemEntries.

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in MaskLayerImageCache. (v2, carry :njn r+)

Rebase due to bug 1182744

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsPresArena. (v2)

Inline UnpoisonFreeList and FreeListEnumerator.

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsPresShell. (v1)

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsPresContext. (v1)

Comment 15

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsPresContext. (v2)

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c86b399124bf

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8633536 [details] [diff] [review]
Use nsTHashTable::Iterator in FrameLayerBuilder. (v2)

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

::: layout/base/FrameLayerBuilder.cpp
@@ +203,5 @@
>  
>  /**
>   * This is the userdata we associate with a layer manager.
>   */
> +extern uint8_t gLayerManagerUserData;

This surprised me; it took me a minute to work out that you're effectively just forward declaring it. Seeing |extern| for a variable that's not shared between modules is odd. Maybe just move the definitions of gLayerManagerUserData (and perhaps the similar other constants) forward instead?

@@ +234,5 @@
> +
> +      const char* layerState;
> +      switch (data->mLayerState) {
> +      case LAYER_NONE:
> +        layerState = "LAYER_NONE"; break;

Nit: the |case| labels should be indented too :/  So says https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +5098,5 @@
> +        }
> +
> +        for (uint32_t i = 0; i < entry->mItems.Length(); i++) {
> +          if (entry->mItems[i].mContainerLayerGeneration >= mContainerLayerGeneration) {
> +            entry->mItems.TruncateLength(i);

This one is tricky! The original code returned PL_DHASH_NEXT here, thus exiting this for-loop early. Your new code doesn't do that. So you should |break| out of this loop after calling TruncateLength().

Actually... because entry->mItems has been truncated I guess the loop condition will fail the next time around. But that's rather subtle, and if somebody hoisted the entry->mItems.Length() into a local variable and then used that variable in the loop condition it would change the meaning. So I think adding the |break| is still a good idea because it makes things more obvious.

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8633541 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresArena. (v2)

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

::: layout/base/nsPresArena.cpp
@@ +156,5 @@
> +    size_t* p;
> +
> +    switch (NS_PTR_TO_INT32(entry->mKey)) {
> +#define FRAME_ID(classname)                               \
> +    case nsQueryFrame::classname##_id:                    \

Again, |case|s should be indented.

@@ +184,5 @@
> +    *p += totalSize;
> +    totalSizeInFreeLists += totalSize;
> +  }
> +
> +  aArenaStats->mOther += (mallocSize - totalSizeInFreeLists);

Nit: the parentheses are unnecessary.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8633542 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresShell. (v1)

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

::: layout/base/nsPresShell.cpp
@@ +9056,5 @@
> +    for (auto iter = mFramesToDirty.Iter(); !iter.Done(); iter.Next()) {
> +      // Mark frames dirty until target frame.
> +      nsPtrHashKey<nsIFrame>* p = iter.Get();
> +      for (nsIFrame* f = p->GetKey(); f && !NS_SUBTREE_DIRTY(f);
> +           f = f->GetParent()) {

Nit: if a for loop header doesn't fit on one line, it's best to break it over three:

> for (nsIFrame* f = p->GetKey();
>      f && !NS_SUBTREE_DIRTY(f);
>      f = f->GetParent()) {

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 8633553 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresContext. (v2)

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

::: layout/base/nsPresContext.cpp
@@ +3014,5 @@
>  {
> +  for (auto iter = aPlugins.Iter(); !iter.Done(); iter.Next()) {
> +    auto f = static_cast<nsPluginFrame*>(iter.Get()->GetKey()->GetPrimaryFrame());
> +    if (!f) {
> +      NS_WARNING("Null frame in PluginsGeometryUpdate");

s/PluginsGeometryUpdate/PluginGetGeometryUpdate/

Comment 21

[Nicholas Nethercote [inactive]]

Thank you for the patches.

Comment 22

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in FrameLayerBuilder. (v3, carry :njn r+)

Addressed comment #17:
* Move gPaintedDisplayItemLayerUserData, gColorLayerUserData, etc. to the
front of the file since LayerManagerData::Dump() references to one of them.
* Indent |case| labels.
* Add |break| after calling TruncateLength().

Comment 23

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsPresArena. (v3, carry :njn r+)

Addressed comment #18.
* Indent |case| labels.
* Remove parentheses after +=

Comment 24

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsPresShell. (v2, carry :njn r+)

Addressed comment #19:
* Break the long for-loop into three lines.

Comment 25

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Use nsTHashTable::Iterator in nsPresContext. (v3, carry :njn r+)

Addressed comment #20:
* s/PluginsGeometryUpdate/PluginGetGeometryUpdate/

Comment 26

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Nicholas, 

Thank you for reviewing my patches. Your comments are very helpful :)

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34bfaa0e81d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/13d5c3d8982a
https://hg.mozilla.org/integration/mozilla-inbound/rev/56c962946e5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4a52bd7393
https://hg.mozilla.org/integration/mozilla-inbound/rev/15077ee67520
https://hg.mozilla.org/integration/mozilla-inbound/rev/60070da355a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/900d0863e9a4

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34bfaa0e81d6
https://hg.mozilla.org/mozilla-central/rev/13d5c3d8982a
https://hg.mozilla.org/mozilla-central/rev/56c962946e5e
https://hg.mozilla.org/mozilla-central/rev/8b4a52bd7393
https://hg.mozilla.org/mozilla-central/rev/15077ee67520
https://hg.mozilla.org/mozilla-central/rev/60070da355a6
https://hg.mozilla.org/mozilla-central/rev/900d0863e9a4