Use nsTHashTable::Iterator in layout/base/

RESOLVED FIXED in Firefox 42

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: TYLin)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(7 attachments, 8 obsolete attachments)

1.75 KB, patch
njn
: review+
Details | Diff | Splinter Review
7.06 KB, patch
njn
: review+
Details | Diff | Splinter Review
2.02 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
17.84 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
6.39 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
5.74 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
6.93 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
Because iterators are so much nicer than enumerate functions.

There are 21 occurrences of EnumerateEntries() in layout/base/ to be dealt with.
Assignee: nobody → tlin
Created attachment 8632829 [details] [diff] [review]
Use nsTHashTable::Iterator in FrameLayerBuilder. (v1)
Attachment #8632829 - Flags: review?(roc)
Attachment #8632829 - Flags: review?(n.nethercote)
Created attachment 8632831 [details] [diff] [review]
Use nsTHashTable::Iterator in FramePropertyTable. (v1)
Attachment #8632831 - Flags: review?(n.nethercote)
Created attachment 8632833 [details] [diff] [review]
Use nsTHashTable::Iterator in MaskLayerImageCache. (v1)
Attachment #8632833 - Flags: review?(n.nethercote)
Created attachment 8632834 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresArena. (v1)
Attachment #8632834 - Flags: review?(n.nethercote)
Created attachment 8632835 [details] [diff] [review]
Use nsTHashTable::Iterator in nsRefreshDriver. (v1)
Attachment #8632835 - Flags: review?(n.nethercote)
(Reporter)

Updated

3 years ago
Attachment #8632831 - Flags: review?(n.nethercote) → review+
(Reporter)

Updated

3 years ago
Attachment #8632833 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 6

3 years ago
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?
Attachment #8632829 - Flags: review?(n.nethercote) → feedback+
(Reporter)

Comment 7

3 years ago
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.
Attachment #8632834 - Flags: review?(n.nethercote) → feedback+
(Reporter)

Comment 8

3 years ago
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.
Attachment #8632835 - Flags: review?(n.nethercote) → review+
(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.
Created attachment 8633536 [details] [diff] [review]
Use nsTHashTable::Iterator in FrameLayerBuilder. (v2)

Inline DumpDisplayItemDataForFrame, ProcessRemovedDisplayItems,
RestoreDisplayItemData, and RestorePaintedLayerItemEntries.
Attachment #8632829 - Attachment is obsolete: true
Attachment #8633536 - Flags: review?(n.nethercote)
Created attachment 8633539 [details] [diff] [review]
Use nsTHashTable::Iterator in MaskLayerImageCache. (v2, carry :njn r+)

Rebase due to bug 1182744
Attachment #8632833 - Attachment is obsolete: true
Attachment #8633539 - Flags: review+
Created attachment 8633541 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresArena. (v2)

Inline UnpoisonFreeList and FreeListEnumerator.
Attachment #8632834 - Attachment is obsolete: true
Attachment #8633541 - Flags: review?(n.nethercote)
Created attachment 8633542 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresShell. (v1)
Attachment #8633542 - Flags: review?(n.nethercote)
Created attachment 8633543 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresContext. (v1)
Attachment #8633543 - Flags: review?(n.nethercote)
Created attachment 8633553 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresContext. (v2)
Attachment #8633543 - Attachment is obsolete: true
Attachment #8633543 - Flags: review?(n.nethercote)
Attachment #8633553 - Flags: review?(n.nethercote)
(Reporter)

Comment 17

3 years ago
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.
Attachment #8633536 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 18

3 years ago
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.
Attachment #8633541 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 19

3 years ago
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()) {
Attachment #8633542 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 20

3 years ago
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/
Attachment #8633553 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 21

3 years ago
Thank you for the patches.
Created attachment 8633883 [details] [diff] [review]
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().
Attachment #8633536 - Attachment is obsolete: true
Attachment #8633883 - Flags: review+
Created attachment 8633884 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresArena. (v3, carry :njn r+)

Addressed comment #18.
* Indent |case| labels.
* Remove parentheses after +=
Attachment #8633541 - Attachment is obsolete: true
Attachment #8633884 - Flags: review+
Created attachment 8633886 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresShell. (v2, carry :njn r+)

Addressed comment #19:
* Break the long for-loop into three lines.
Attachment #8633542 - Attachment is obsolete: true
Attachment #8633886 - Flags: review+
Created attachment 8633887 [details] [diff] [review]
Use nsTHashTable::Iterator in nsPresContext. (v3, carry :njn r+)

Addressed comment #20:
* s/PluginsGeometryUpdate/PluginGetGeometryUpdate/
Attachment #8633553 - Attachment is obsolete: true
Attachment #8633887 - Flags: review+
Nicholas, 

Thank you for reviewing my patches. Your comments are very helpful :)
You need to log in before you can comment on or make changes to this bug.