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

Tue May 14 09:55:04 PDT 2013

Darin Adler <darin at apple.com> has granted 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 201697: Patch
https://bugs.webkit.org/attachment.cgi?id=201697&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201697&action=review

> Source/WebCore/rendering/RenderBoxModelObject.cpp:509
> +	       // In the offsetParent algorithm defined for elements inside CSS
Regions flows,
> +	       // the nearest ancestor search skips from the topmost named flow
object directly
> +	       // to the body element.
> +	       // However, the RenderNamedFlowThread is attached to the
RenderView directly,
> +	       // meaning that we are going to bypass the <body>'s RenderObject
while iterating the parents.
> +	       // As such, if the RenderNamedFlowThread is reached while
iterating the parents, end this loop.
> +	       // See
http://dev.w3.org/csswg/css-regions/#cssomview-offset-attributes

I like the idea of this comment, but it would be much more useful if we could
write a smaller one. There is so much text here that it’s hard to read. Is
there a more concise way of saying the same thing? Could you follow this up
with a patch to improve this comment by making it much shorter? I suggest this
comment:

    // CSS region specification says that region flows should return the body
element as their offsetParent.
    // Since we will bypass the body’s renderer anyway, just end the loop if we
encounter a region flow (named flow thread).

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

The comment would be better if it was worded slightly differently. I would just
say this:

    // CSS region specification says that region flows should return the body
element as their offsetParent.

> Source/WebCore/rendering/RenderObject.cpp:3016
> +    // http://dev.w3.org/csswg/css-regions/#cssomview-offset-attributes

I don’t think the link to the spec is all that useful.

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

This comment just repeats what the code does and should be removed.