Bug 33769 – [Chromium] layout_test on linux fails on doubleclick-beside-cr-span.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[MORITA Hajime]

Layout test case "editing/selection/doubleclick-beside-cr-span.html" fails on chromium linux.
It seems due to difference of tab rendering between each platform.

[MORITA Hajime]

Was reported at:
http://code.google.com/p/chromium/issues/detail?id=30816

[MORITA Hajime]

Created attachment 46758 [details]
now passes both on linux chromium and on mac

[Shinichiro Hamaji]

Comment on attachment 46758 [details]
now passes both on linux chromium and on mac

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.

[MORITA Hajime]

Created attachment 46872 [details]
update

[MORITA Hajime]

Thank you for reviewing!
I updated the patch.

> It would be better to explain how did you fix this failure.
Added an explanation.

> 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.
Fixed. Thank you to clarify.

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

> I'm not sure why this "Here" is necessary. I think it would be nice if you put
> a comment for this line.
> 
(snip)
> 
> Why did we change here?
Added an explanation in the comment

> 
> > -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
> 
Removed.
At first, clarifying whitespace length limitation by doing this looked good idea,
but it isn't...

[Shinichiro Hamaji]

Comment on attachment 46872 [details]
update

Looks good. Thanks for fixing this issue! I'll land this patch manually after I check this patch with several environments.

[Shinichiro Hamaji]

Committed r53466: <http://trac.webkit.org/changeset/53466>

[Eric Seidel (no email)]

This test is timing out on Windows:
http://build.webkit.org/results/Windows%20Debug%20(Tests)/r53786%20(8972)/results.html

Not sure why.  It started failing while the windows bot was down.