[webkit-reviews] review denied: [Bug 130715] [CSS Regions] Overflow selection doesn't work properly : [Attachment 229541] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Radu Stavila <stavila at adobe.com>'s
request for review:
Bug 130715: [CSS Regions] Overflow selection doesn't work properly
https://bugs.webkit.org/show_bug.cgi?id=130715

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229541&action=review


I'm kind of alarmed by the intrusiveness of this change. If there's no other
way, then I would at least like it if you could avoid allowing the region to be
null. We don't want people to mess up when they write new version of this
method that have to recur into children.

> Source/WebCore/rendering/RenderBlock.cpp:2419
> +	   if
(!toRenderFlowThread(paintInfo->paintContainer)->objectShouldPaintInFlowRegion(
this, paintInfo->renderNamedFlowFragment))

Consider renaming this method: objectShouldPaintInFlowRegion. You're using it
for hit tests, so I don't think "paint" is a good term to be using.

> Source/WebCore/rendering/RenderBlock.cpp:3233
>      if (childrenInline())
>	   return positionForPointWithInlineChildren(pointInLogicalContents);

Why are inline children not affected by this issue? They can be in different
regions.

> Source/WebCore/rendering/RenderBlock.h:186
> +    virtual VisiblePosition positionForPoint(const LayoutPoint&, const
RenderRegion* = nullptr) override;

It's really strange to me that this allows a nullptr. The problem I have with
this is that someone could forget to pass the second argument when recurring
into children, and it won't be a compile error. This seems fragile and
error-prone to me.