[webkit-reviews] review denied: [Bug 120769] [CSS Regions] Selection focusNode set to the "region" block, instead of the "source" block : [Attachment 214161] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Javier Fernandez
<jfernandez at igalia.com>'s request for review:
Bug 120769: [CSS Regions] Selection focusNode set to the "region" block,
instead of the "source" block
https://bugs.webkit.org/show_bug.cgi?id=120769

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

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


r- for the writing mode issues. I want a vertical writing mode test added for
the next patch because of this.

> Source/WebCore/rendering/RenderBlock.cpp:3644
> +    const RenderBlock &block = box.isRenderBlock() ? toRenderBlock(box) :
*box.containingBlock();

Surprised the style code didn't complain on this line. Move the & to be next to
RenderBlock, i.e.,

const RenderBlock& block

> Source/WebCore/rendering/RenderRegion.cpp:72
> +    if (point.y() < 0)
> +	   return LayoutPoint(0, m_flowThreadPortionRect.y());

Not writing-mode-aware. For vertical writing modes it is x you need to check.

> Source/WebCore/rendering/RenderRegion.cpp:74
> +    if (point.y() > m_flowThreadPortionRect.height()) {

Same problem.

> Source/WebCore/rendering/RenderRegion.cpp:82
> +    return LayoutPoint(point.x(), point.y() + m_flowThreadPortionRect.y());

Same problem, etc.

> Source/WebCore/rendering/RenderRegion.cpp:88
> +	   return RenderBlock::positionForPoint(point);

Shouldn't this be RenderBlockFlow::positionForPoint?