[webkit-reviews] review granted: [Bug 110352] Use DOM ordering for list counts : [Attachment 194600] Patch

[bugzilla-daemon at webkit.org]

Elliott Sprehn <esprehn at chromium.org> has granted Andrei Bucur
<abucur at adobe.com>'s request for review:
Bug 110352: Use DOM ordering for list counts
https://bugs.webkit.org/show_bug.cgi?id=110352

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

------- Additional Comments from Elliott Sprehn <esprehn at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=194600&action=review


You don't actually handle ShadowRoot hosts properly, and ShadowRoot will never
pass the isList test. Please fix that comment and file a follow up bug and
mention it in the ChangeLog before landing.

> Source/WebCore/ChangeLog:25
> +	   In later patches I'll add tests that should cover the regions and
shadow DOM use cases.

File a bug about needing these tests and add a link here.

> Source/WebCore/rendering/RenderCounter.cpp:83
> +    Element* self = toElement(object->node());

nit: Don't need the extra local.

> Source/WebCore/rendering/RenderListItem.cpp:106
> +    for (Node* parent = listItemNode->parentNode(); parent; parent =
parent->parentNode()) {

This loop doesn't do what you think it does for ShadowRoot. ShadowRoot is a
Node but it's parentNode() is also always null and it'll never match isList
since that's about tags. You'd need to check if parent->isShadowRoot() and
isList(toShadowRoot(parent)->host()). I'd add a FIXME here. The comment makes
it sound like this handles ShadowRoot when it doesn't.