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

[bugzilla-daemon at webkit.org]

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

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

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


Looks generally good. Some things that must be fixed before landing. Someone a
bit more expert in rendering might have comments on the basic approach beyond
what I have said here.

> Source/WebCore/rendering/RenderBlock.cpp:4482
> +static inline bool isChildHitTestCandidate(RenderBox* box, RenderRegion
*region, const LayoutPoint& point)

The RenderBox* argument should be RenderBox&, since it’s never null.

The RenderRegion* argument is formatted incorrectly. The * should be next to
the type, not the argument name.

> Source/WebCore/rendering/RenderBlock.cpp:4489
> +	   if (box->isRenderBlock())
> +	       inRegion = toRenderBlock(box)->regionAtBlockOffset(point.y()) ==
region;
> +	   else
> +	       inRegion =
box->containingBlock()->regionAtBlockOffset(point.y()) == region;

I think this is better:

    if (!isChildHitTestCandidate(box))
	return false;
    if (!region)
	return true;
    RenderBlock& block = box.isRenderBlock() ? toRenderBlock(box) :
*box.containingBlock();
    return block.regionAtBlockOffset(point.y()) == region;

For one thing, we save extra work when isChildHitTestCandidate is false.

> Source/WebCore/rendering/RenderRegion.cpp:68
> +    LayoutPoint newPoint(0, point.y());

The approach here of computing a point within the flow thread portion rect
seems peculiar to me.

> Source/WebCore/rendering/RenderRegion.cpp:69
> +    if (point.y() > m_flowThreadPortionRect.height()) // hitting bottom
margin, padding, border, ...

No idea what these "…" mean. I don’t think these comments are helping much.

> Source/WebCore/rendering/RenderRegion.cpp:70
> +	   newPoint.setY(m_flowThreadPortionRect.maxY() - 1);

The - 1 here seems quite strange to me and points to the approach possibly
being incorrect. I understand the desire to pass in a point "at the bottom",
but keep in mind that with fractional positioning there could be a box that is
less than 1 point thick and it also is not clear to me that hit testing is
always about a 1x1 box with the point at the top left corner.

> Source/WebCore/rendering/RenderRegion.cpp:71
> +    else if (point.y() < 0) // hitting top margin, padding, border, ...

Same thought here about the "…".

> Source/WebCore/rendering/RenderRegion.cpp:76
> +    if (m_flowThread->firstChild()) // checing out for empty region blocks

No idea what "checing out" means.

> Source/WebCore/rendering/RenderRegion.h:109
> +    virtual VisiblePosition positionForPoint(const LayoutPoint&);

Need to say OVERRIDE here. Also, is there a reason this needs to be public? If
not, it should be private instead for now.

> LayoutTests/fast/regions/position-for-point.html:94
> +		   clearSelection();
> +
> +		   selectFromIdToPosition("word1", 100, 140);
> +		   checkResult("result1", "word2", "5", "word1 inside region
inside region inside region inside region word2");
> +
> +		   clearSelection();
> +
> +		   selectFromIdToPosition("word1", 100, 151);
> +		   checkResult("result2", "word2", "5", "word1 inside region
inside region inside region inside region word2");
> +
> +		   clearSelection();
> +
> +		   selectFromIdToPosition("word3", 100, 330);
> +		   checkResult("result3", "word5", "0", "word5 inside region
inside region inside region inside region word6 ");
> +
> +		   clearSelection();
> +
> +		   selectFromIdToPosition("word5", 100, 440);
> +		   checkResult("result4", "word6", "5", "word5 inside region
inside region inside region inside region word6");

Do these five tests really cover all the branches in the new code?