[webkit-reviews] review granted: [Bug 47237] Make selection work with vertical text. : [Attachment 73118] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Dave Hyatt <hyatt at apple.com>'s request for review:
Bug 47237: Make selection work with vertical text.
https://bugs.webkit.org/show_bug.cgi?id=47237

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

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=73118&action=review

Found one mistake.

> WebCore/rendering/RenderBlock.cpp:2709
> +	   if (!paintInfo || (style()->isHorizontalWritingMode() &&
physicalRect.y() < paintInfo->rect.bottom() && physicalRect.bottom() >
paintInfo->rect.y()
> +	       || (!style()->isHorizontalWritingMode() && physicalRect.x() <
paintInfo->rect.right() && physicalRect.right() > paintInfo->rect.x())))

Redundant parentheses there.

> WebCore/rendering/RenderBlock.h:121
> +    IntRect logicalLeftSelectionGap(RenderBlock* rootBlock, const IntPoint&
rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock,
> +				       RenderObject* selObj, int logicalLeft,
int logicalTop, int logicalHeight, const PaintInfo* paintInfo);
> +    IntRect logicalRightSelectionGap(RenderBlock* rootBlock, const IntPoint&
rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock,
> +					RenderObject* selObj, int logicalRight,
int logicalTop, int logicalHeight, const PaintInfo* paintInfo);

Don’t add the argument name “paintInfo”.

> WebCore/rendering/RenderBlock.h:541
> +    GapRects selectionGaps(RenderBlock* rootBlock, const IntPoint&
rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock,
> +			      int& lastLogicalTop, int& lastLogicalLeft, int&
lastLogicalRight, const PaintInfo* paintInfo = 0);
> +    GapRects inlineSelectionGaps(RenderBlock* rootBlock, const IntPoint&
rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock,
> +			      int& lastLogicalTop, int& lastLogicalLeft, int&
lastLogicalRight, const PaintInfo* paintInfo);
> +    GapRects blockSelectionGaps(RenderBlock* rootBlock, const IntPoint&
rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock,
> +			      int& lastLogicalTop, int& lastLogicalLeft, int&
lastLogicalRight, const PaintInfo* paintInfo);
> +    IntRect blockSelectionGap(RenderBlock* rootBlock, const IntPoint&
rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock,
> +				 int lastLogicalTop, int lastLogicalLeft, int
lastLogicalRight, int logicalBottom, const PaintInfo* paintInfo);
>      int logicalLeftSelectionOffset(RenderBlock* rootBlock, int position);

Ditto.

> WebCore/rendering/RenderBlock.h:543
> +    

whitespace!

> WebCore/rendering/RenderBox.cpp:3245
> +    return style()->isHorizontalWritingMode() ? IntSize(offset.width(),
logicalHeight() - offset.height()) : IntSize(logicalHeight() - offset.width(),
offset.height());

Should be logicalWidth() - offset.width() in the second branch.

> WebCore/rendering/RootInlineBox.cpp:254
> +GapRects RootInlineBox::lineSelectionGap(RenderBlock* rootBlock, const
IntPoint& rootBlockPhysicalPosition, const IntSize& offsetFromRootBlock, 
> +					    int selTop, int selHeight, const
PaintInfo* paintInfo)

Can omit “paintInfo”.