[webkit-reviews] review denied: [Bug 33769] [Chromium] layout_test on linux fails on doubleclick-beside-cr-span.html : [Attachment 46758] now passes both on linux chromium and on mac

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied MORITA Hajime
<morrita at gmail.com>'s request for review:
Bug 33769: [Chromium] layout_test on linux fails on
doubleclick-beside-cr-span.html
https://bugs.webkit.org/show_bug.cgi?id=33769

Attachment 46758: now passes both on linux chromium and on mac
https://bugs.webkit.org/attachment.cgi?id=46758&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Thanks for fixing! Some nitpicks and questions.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> +	   [Chromium] layout_test on linux fails on
doubleclick-beside-cr-span.html
> +	   
> +	   https://bugs.webkit.org/show_bug.cgi?id=33769

It would be better to explain how did you fix this failure.

> -    eventSender.mouseMoveTo(pos.x + 2, pos.y + 2);
> +    // We choose value "4" heuristically - will longer than single tab,
shorter than a word.
> +    // So cases should follow this assumption.

I'd say "Out test cases will have at most a single leading tab followed by a
word whose length is more than 4. So, pos.x + CHAR_WIDTH * 4 always hits the
words" or something.

> +    eventSender.mouseMoveTo(pos.x + CHAR_WIDTH*4, pos.y + LINE_HEIGHT/2);

I think we usually put white-spaces around '*' and '/'.

> @@ -160,14 +155,14 @@ function runTests()
>	   doTest("totest_multiple_word_in_span", "select9 ");
>	   doTest("totest_word_before_here_in_line", "select10 ");
>	   doTest("totest_span_first_half", "select11 ");
> -	   doTest("totest_span_second_half", "select12 ");
> +	   doTest("totest_span_second_half", "selectHere12 ");

I'm not sure why this "Here" is necessary. I think it would be nice if you put
a comment for this line.

> -<div style="width:100pt">
> -abcd efgh ijkl mnop qrst sel<b id="totest_span_second_half">ect12</b> notyet

> +<div style="width:150pt">
> +abcd efgh ijkl mnop qrst uvwx yz123 sel<b
id="totest_span_second_half">ectHere12</b> notyet

Why did we change here?

> -abcd efgh ijkl mnop<b id="totest_multiple_whitespaces_in_pre">    select6   
</b>nottoselect
> +abcd efgh ijkl mnop<b id="totest_multiple_whitespaces_in_pre">   select6  
</b>nottoselect

Ditto.