[webkit-reviews] review denied: [Bug 61344] --webkit-visual-word does not work in multi-line : [Attachment 100723] patch w/ layout test

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Xiaomei Ji <xji at chromium.org>'s
request for review:
Bug 61344: --webkit-visual-word does not work in multi-line
https://bugs.webkit.org/show_bug.cgi?id=61344

Attachment 100723: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=100723&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=100723&action=review


I've got a feeling that previousBlockWithLineBoxes/nextBlockWithLineBoxes are
broken for flex boxes.	Please look into it.

> Source/WebCore/ChangeLog:15
> +	   Reviewed by NOBODY (OOPS!).

This should appear before the long description.

> Source/WebCore/editing/htmlediting.h:150
> +    return Position(node, offset, Position::PositionIsOffsetInAnchor);

Can this node be non-text?  If so, we'll be introducing new blocker for
https://bugs.webkit.org/show_bug.cgi?id=63040.	If not , you should use the one
that doesn't take the anchor type. r- due to this change.

To give you some background, we're trying to avoid instantiating a position
with (node, offset) pair when the node is not a text.  Instead, we use
positions before/after node or positions before/after children.

> Source/WebCore/editing/visible_units.cpp:1325
> +				  
createPositionAvoidingIgnoredNode(box->renderer()->node(),
box->caretMinOffset());

Nit: should be intended by 4 spaces.

> Source/WebCore/editing/visible_units.cpp:1481
> +static const RenderBlock* previousBlockWithLineBoxes(const RenderBlock*
root)

I'm not sure if root is a good name for this.  I'd call it startingBlock.

> Source/WebCore/editing/visible_units.cpp:1483
> +    for (const RenderBlock* previous = root; previous; previous =
toRenderBlock(previous->previousSibling())) {

"previous" is a relative term.	I get confused by its use in this function. 
Can we just call it block or currentBlock?

> Source/WebCore/editing/visible_units.cpp:1485
> +	   // Renderer having inline children with contents has root line
boxes. 
> +	   if (previous->childrenInline() && previous->firstRootBox())

I don't think this comment adds any new information because you're indeed
checking that the first root box isn't null.  Please remove it.

> Source/WebCore/editing/visible_units.cpp:1509
> +    while (renderer) {
> +	   while (renderer && !renderer->isRenderBlock())
> +	       renderer = renderer->parent();
> +
> +	   if (!renderer)
> +	       return 0;
> +
> +	   if (const RenderBlock* blockWithLineBoxes =
previousBlockWithLineBoxes(toRenderBlock(renderer->previousSibling())))
> +	       return blockWithLineBoxes->lastRootBox();
> +
> +	   renderer = renderer->parent();
> +    }

I started to think that these nested loops can be flattened as follows:

for (RenderObject* renderer = node->renderer(); renderer; renderer =
renderer->parent()) {
    if (renderer->isRenderBlock()) {
	if (const RenderBlock* blockWithLineBoxes =
previousBlockWithLineBoxes(toRenderBlock(renderer->previousSibling()))
	    return blockWithLineBoxes->lastRootBox();
    }
}

no?

> Source/WebCore/editing/visible_units.cpp:1516
> +	   // Renderer having inline children with contents has root line
boxes. 

Ditto about the comment.

> Source/WebCore/editing/visible_units.cpp:1532
> +    while (renderer) {
> +	   while (renderer && !renderer->isRenderBlock())
> +	       renderer = renderer->parent();

Ditto about flattening the nested loop.

> Source/WebCore/editing/visible_units.cpp:1552
> +								   
rootBox->nextLineBox();

Nit: indentation.

> Source/WebCore/editing/visible_units.cpp:1564
> +    if (leftRootInlineBox)
> +	   return leftRootInlineBox->lastLeafChild();
> +    return 0;

I'd use tertiary here.

> Source/WebCore/editing/visible_units.cpp:1574
> +								    
rootBox->prevLineBox();

Nit: indentation.

> Source/WebCore/editing/visible_units.cpp:1586
> +    if (rightRootInlineBox)
> +	   return rightRootInlineBox->firstLeafChild();
> +    return 0;

Nit: tertiary.

> Source/WebCore/editing/visible_units.cpp:1704
> +    VisiblePosition rightWordBreak =
rightWordPositionAcrossBoundary(visiblePosition);

You should check the nullity of visiblePosition here.

> LayoutTests/editing/selection/move-by-word-visually-others.html:94
> +<div contenteditable dir=ltr id="multi_line_1" class="test_move_by_word
fix_width" style="width:50px" title="[multi_line_1, 0][multi_line_1,
4][multi_line_1, 8][multi_line_1, 12][multi_line_1, 16][multi_line_1, 0,
5][multi_line_1, 4, 5][multi_line_1, 8, 5][multi_line_1, 12, 5]|[multi_line_1,
15, 5][multi_line_1, 12, 5][multi_line_1, 8, 5][multi_line_1, 4,
5][multi_line_1, 0, 5][multi_line_1, 16][multi_line_1, 12][multi_line_1,
8][multi_line_1, 4][multi_line_1, 0]">abc def ghi jkl mn <br/><br/><br/>opq rst
uvw xyz</div>

Can we give a shorter id to make the expected result shorter?