[webkit-reviews] review denied: [Bug 69605] CSS Counters have wrong values : [Attachment 110102] Proposed Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Carol Szabo <carol at webkit.org>'s request for review:
Bug 69605: CSS Counters have wrong values
https://bugs.webkit.org/show_bug.cgi?id=69605

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

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=110102&action=review


The general seems correct, but the I am not sure about the execution.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:803
> +RenderObject* RenderBoxModelObject::rendererOfAfterPseudoElement()

This should return a RenderBoxModelObject*

> Source/WebCore/rendering/RenderBoxModelObject.cpp:806
> +    while (RenderObject* continuation =
static_cast<RenderBoxModelObject*>(lastContinuation)->continuation())

The correct way to write this cast is using the function
toRenderBoxModelObject. But there is no reason to do so here, since
lastContinuation can be a RenderBoxModelObject* anyway.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:810
> +

Instead of a virtual RenderObject member function, this could be a non-virtual
function using virtualContinuation(). It should also start by checking if the
document uses before/after rules.

> Source/WebCore/rendering/RenderBoxModelObject.h:125
> +    // This function goes through continuations if any to retrieve the
"after"
> +    // pseudoElement.

This comment can fit on one line. The term is “pseudo-element”.

> Source/WebCore/rendering/RenderObject.h:161
> +    // This function only returns the renderer of the "after" pseudoElement
> +    // If it is a virtualChild of this renderer. If "continuations" exist,
> +    // the function returns 0 even if the element that generated this
renderer
> +    // has an "after" pseudoElement.

There’s som weird terminology in this comment. There is no such thing as
“virtualChild”, and “pseudoElement” should be written “pseudo-element”.

> Source/WebCore/rendering/RenderObject.h:170
> +    // This function goes through continuations if any to retrieve the
"after"
> +    // pseudoElement.

This comment can fit on one line.