[webkit-reviews] review denied: [Bug 113276] [CSSRegions] Implement offsetParent for elements inside named flow : [Attachment 200893] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 7 08:36:17 PDT 2013


Alexandru Chiculita <achicu at adobe.com> has denied Radu Stavila
<stavila at adobe.com>'s request for review:
Bug 113276: [CSSRegions] Implement offsetParent for elements inside named flow
https://bugs.webkit.org/show_bug.cgi?id=113276

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

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=200893&action=review


Looks good! I have a few small nits below.

> LayoutTests/ChangeLog:8
> +	   Reviewed by NOBODY (OOPS!).

Nit: this line usually comes after the bug link.

> LayoutTests/fast/regions/offsetLeft-offsetTop-in-flow-thread.html:4
> +		padding: 10px;

I think it's better to specify a font size here. This way you avoid differences
between default styles. Also, Ahem font could reduce the differences across
platforms.

> LayoutTests/fast/regions/offsetLeft-offsetTop-in-flow-thread.html:56
> +		shouldBe("table.offsetTop", "18");

Testing for an exact value in the test file would make it hard to rebate line
on other platforms.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:500
> +	       // In the offsetParent algorithm, the nearest ancestor search
skips from the topmost named flow elements directly to the body element.

There is no element for the names flows. They are only render objects.

> Source/WebCore/rendering/RenderObject.cpp:3015
> +    // If this element is inside a named flow thread, the nearest ancestor
search skips

I would avoid to use "element" when talking about render objects. It is a
little confusing.


More information about the webkit-reviews mailing list