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.
Was reported at: http://code.google.com/p/chromium/issues/detail?id=30816
Created attachment 46758 [details] now passes both on linux chromium and on mac
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.
Created attachment 46872 [details] update
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...
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.
Committed r53466: <http://trac.webkit.org/changeset/53466>
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.