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

[bugzilla-daemon at webkit.org]

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 201335: Patch
https://bugs.webkit.org/attachment.cgi?id=201335&action=review

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


Thanks for reworking this. I still have a couple nits below.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:503
> +	       // In the offsetParent algorithm, the nearest ancestor search
skips from the topmost named flow objects directly

nit: I would mention this only applies on elements inside the regions flow: "In
the offsetParent algorithm defined for elements inside CSS Regions flows," .
Also add a link to that section here.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:504
> +	       // to the body element, so the offsetParent might be the body

nit: I think "so the offsetParent might be the body" is not necessary. Make
sure the phrases end with a dot.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:510
> +	       while (!curr->isRenderNamedFlowThread() && curr != offsetParent)
{

Can offsetParent be null here? In that case you may need to change the order of
the checks to avoid a null dereference.

> Source/WebCore/rendering/RenderObject.cpp:3019
> +	   // The search reached the named flow thread, skip to the body
element

nit: Missing dot.
http://www.webkit.org/coding/coding-style.html#comments-sentences

> Source/WebCore/rendering/RenderObject.cpp:3020
> +	   return document()->body()->renderBoxModelObject();

I think body() can return 0 in some early cases. You might as well just patch
"curr" and let it go through.