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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted 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 101368: patch w/ layout test
https://bugs.webkit.org/attachment.cgi?id=101368&action=review

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


> Source/WebCore/editing/htmlediting.cpp:1144
> +    if (editingIgnoresContent(node) || !node->isTextNode()) {

It appears that all we need to check is !node->isTextNode().  Since
editingIgnoresContent calls a virtual function, we should get rid of this
function call if possible.

> Source/WebCore/editing/visible_units.cpp:1539
> +

Nit: Please get rid of this blank line.

> Source/WebCore/editing/visible_units.cpp:1541
> +    const InlineFlowBox* leftLineBox = (blockDirection == LTR) ?
rootBox->prevLineBox() :
> +	   rootBox->nextLineBox();

Nit: we can fit this in one line if we had added a local variable like "const
bool isBlockLTR = blockDirection == LTR;"

> Source/WebCore/editing/visible_units.cpp:1550
> +    const RootInlineBox* leftRootInlineBox;
> +    if (blockDirection == LTR)
> +	   leftRootInlineBox = previousRootInlineBox(box);
> +    else
> +	   leftRootInlineBox = nextRootInlineBox(box);

Nit: I would have used tertiary.

> Source/WebCore/editing/visible_units.cpp:1563
> +
> +    const InlineFlowBox* rightLineBox = (blockDirection == LTR) ?
rootBox->nextLineBox() :
> +	   rootBox->prevLineBox();

Ditto.

> Source/WebCore/editing/visible_units.cpp:1572
> +    const RootInlineBox* rightRootInlineBox;
> +    if (blockDirection == LTR)
> +	   rightRootInlineBox = nextRootInlineBox(box);
> +    else
> +	   rightRootInlineBox = previousRootInlineBox(box);

Ditto.

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

I'm afraid that specifying "50px" will make this test platform-dependent. I'd
at least use "ex" or "em" and only use "x" or some other letter since mixing
letters will make the text width more dependent on font metrics.

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

Ditto about width.

>
LayoutTests/editing/selection/move-by-word-visually-single-space-sigle-line.htm
l:68
> -<div dir=ltr id="div_1" class="test_move_by_word" title="[div_1, 0][div_1,
3]|[span_1, 2][div_1, 3][div_1,0]" contenteditable>אאא <span
id="span_1">בב</span></div>
> -<div dir=rtl id="div_2" class="test_move_by_word" title="[span_2, 2][div_2,
4][div_2, 0]|[div_2, 0][div_2, 4]" contenteditable>אאא <span
id="span_2">בב</span></div>
> +<div dir=ltr id="d_1" class="test_move_by_word" title="[d_1, 0][d_1,
3]|[s_1, 2][d_1, 3][d_1,0]" contenteditable>אאא <span
id="s_1">בב</span></div>
> +<div dir=rtl id="d_2" class="test_move_by_word" title="[s_2, 2][d_2, 4][d_2,
0]|[d_2, 0][d_2, 4]" contenteditable>אאא <span id="s_2">בב</span></div>

Why don't I see these changes in expected.txt?

> LayoutTests/editing/selection/resources/move-by-word-visually.js:247
> +	   var span = document.getElementById("span_size");
> +	   var length = span.offsetWidth;
> +	   test.style.width = length + "px";

Mn... given this code maybe you didn't need to set width property in above test
cases anyways?