[webkit-reviews] review denied: [Bug 71244] CSS 2.1 failure: border-conflict-element-* : [Attachment 115437] 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 71244: CSS 2.1 failure: border-conflict-element-*
https://bugs.webkit.org/show_bug.cgi?id=71244

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

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


r- mostly for the int <-> unsigned issues. Also some inconsistencies.

> Source/WebCore/rendering/RenderTableCell.cpp:456
>	   colElt = table->colElement(col() -1, &startColEdge, &endColEdge);

Nit: space between '-' and '1'.

> Source/WebCore/rendering/RenderTableCell.cpp:484
> +    RenderTableCell* nextCell = table->cellAfter(this);

That doesn't look like a good change as you don't use nextCell outside the if.

> Source/WebCore/rendering/RenderTableCell.cpp:572
> +	       result = chooseBorder(result,
CollapsedBorderValue(prevRow->style()->borderAfter(),
prevRow->style()->visitedDependentColor(after), BROW, col(), prevCell->row(),
BAFTEREDGE));

Shouldn't it be prevCell->col() here for consistency?

> Source/WebCore/rendering/RenderTableCell.cpp:582
> +	   result = chooseBorder(result,
CollapsedBorderValue(currSection->style()->borderBefore(),
currSection->style()->visitedDependentColor(before), BROWGROUP, col(), row(),
BBEFOREEDGE));

Shouldn't you add the rowSpan() here to match section (6)?

> Source/WebCore/rendering/RenderTableCell.cpp:589
> +	       result =
chooseBorder(CollapsedBorderValue(currSection->style()->borderAfter(),
currSection->style()->visitedDependentColor(after), BROWGROUP, col(), row() -
rowSpan(), BAFTEREDGE), result);

Nothing ensures that row() > rowSpan() here. This sounds like you could
underflow your unsigned.

> Source/WebCore/rendering/RenderTableCell.cpp:643
> +	   result = chooseBorder(result,
CollapsedBorderValue(nextCell->parent()->style()->borderBefore(),
nextCell->parent()->style()->visitedDependentColor(before), BROW, col(),
nextCell->row(), BBEFOREEDGE));

Again shouldn't it be nextCell->col() for consistency?

> Source/WebCore/rendering/style/CollapsedBorderValue.h:39
> +    CollapsedBorderValue(const BorderValue& b, Color c, EBorderPrecedence p,
int rowOffset, int columnOffset, EEdgePrecedence edge)

Those should be unsigned too.

> Source/WebCore/rendering/style/CollapsedBorderValue.h:66
> +    int rowOffset() const { return m_rowOffset; }

Those 2 should return an unsigned too.

> Source/WebCore/rendering/style/CollapsedBorderValue.h:79
> +    unsigned int m_columnOffset;

Unsigned (not int) is the preferred usage in WebKit.

> Source/WebCore/rendering/style/RenderStyleConstants.h:101
> +enum EEdgePrecedence { BAFTEREDGE, BENDEDGE, BBEFOREEDGE, BSTARTEDGE};

Space before the final }