[webkit-reviews] review denied: [Bug 73349] compareBorders() is called too often during painting : [Attachment 120555] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 73349: compareBorders() is called too often during painting
https://bugs.webkit.org/show_bug.cgi?id=73349

Attachment 120555: Patch
https://bugs.webkit.org/attachment.cgi?id=120555&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120555&action=review


I like the new approach a bit better as it guarantees we won't miss the cache.
The logic could be made a lot more readable though.

> Source/WebCore/rendering/RenderTable.cpp:299
> +    // and cached when recomputing the table's width.

s/width/logical width/.

Also the comment is actually lacking half of the explanation: computing the
logical width assures that we do resolve the collapsed borders in the inline
base direction (before/after cells) not in the block flow direction.

I would really love to have a comment explaining why *all* borders will be
computed during layout. This would make sense as unfortunately there is no
safe-guard against forgetting to compute a cell's border in your new caching
code. I am not sure how difficult this would be to add so I am leaving this for
you to see.

> Source/WebCore/rendering/RenderTable.h:203
> +    bool hasInvalidCollapsedBorders() { return !m_collapsedBordersValid; }

should be |const|

> Source/WebCore/rendering/RenderTableCell.cpp:422
> -	       return result;
> +	       return cellCollapsedBorders->start(result);
>      } else if (isStartColumn) {
>	   // (3) Our row's start border.
>	   result = chooseBorder(result,
CollapsedBorderValue(parent()->style()->borderStart(), includeColor ?
parent()->style()->visitedDependentColor(startColorProperty) : Color(), BROW));

>	   if (!result.exists())
> -	       return result;
> -	   
> +	       return cellCollapsedBorders->start(result);
> +
>	   // (4) Our row group's start border.
>	   result = chooseBorder(result,
CollapsedBorderValue(section()->style()->borderStart(), includeColor ?
section()->style()->visitedDependentColor(startColorProperty) : Color(),
BROWGROUP));
>	   if (!result.exists())
> -	       return result;
> +	       return cellCollapsedBorders->start(result);

This is globally an anti-pattern. You have to catch each and every |return| to
properly cache your new value which makes the code hard to read and very
sloppy. The computation should be moved to a new method
(computeCollapsed*Border) and collapsed*Border should be the one querying the
cache and recomputing as needed.

> Source/WebCore/rendering/RenderTableCell.cpp:900
>  void
RenderTableCell::collectBorderValues(RenderTable::CollapsedBorderValues&
borderValues) const
>  {
> +    ASSERT(table()->needsLayout());

If I am reading the code correctly, collectBorderValues can only be called
during painting which makes this ASSERT very strange.

> Source/WebCore/rendering/RenderTableCell.cpp:945
> -	
> +    

Unneeded & unrelated change. You are also not going far enough in your white
space removal.

> Source/WebCore/rendering/RenderTableSection.cpp:1347
> +    return &result.first->second;

Your logic is quite twisted here. I am not sure why it should be fine to
request a collapsed border that is not in our cache.

> Source/WebCore/rendering/RenderTableSection.h:133
> +    CellCollapsedBorders* cellCollapsedBorders(const RenderTableCell*);

You don't expect this function to return 0 or some garbage. It looks like it
should return a reference in this case.

> Source/WebCore/rendering/style/CollapsedBorderValue.h:82
> +    CollapsedBorderValue end(CollapsedBorderValue end) { m_end = end; return
m_end; }

Setters should start with "set" per our coding style! It would make your change
more readable.

I am really not a huge fan of returning the value in those setters...

> Source/WebCore/rendering/style/CollapsedBorderValue.h:87
> +    bool exists() const { return m_start.exists() && m_end.exists() &&
m_before.exists() && m_end.exists(); }

This function is unused.