Bug 80879 - Mock scrollbars differ by 1px in DRT vs. WKTR
Summary: Mock scrollbars differ by 1px in DRT vs. WKTR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-12 13:42 PDT by Tim Horton
Modified: 2012-06-07 01:10 PDT (History)
3 users (show)

See Also:


Attachments
difference image (555 bytes, image/png)
2012-03-12 13:42 PDT, Tim Horton
no flags Details
actual image (81.73 KB, image/png)
2012-03-12 13:43 PDT, Tim Horton
no flags Details
expected image (81.19 KB, image/png)
2012-03-12 13:43 PDT, Tim Horton
no flags Details
patch (1.57 KB, patch)
2012-06-04 15:22 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2012-03-12 13:42:07 PDT
Running SVG pixel tests with tolerance=0 between DRT and WKTR, I noticed that in every case there's a scrollbar in the test, the thumb is 1px longer in DRT. (I assume, but have not tested, that this affects non-SVG tests as well). This causes the tests to fail.

Example: svg/dom/SVGLengthList-basics-diffs.html

Attaching two images from mac-lion.
Comment 1 Tim Horton 2012-03-12 13:42:40 PDT
Created attachment 131397 [details]
difference image
Comment 2 Tim Horton 2012-03-12 13:43:13 PDT
Created attachment 131398 [details]
actual image
Comment 3 Tim Horton 2012-03-12 13:43:38 PDT
Created attachment 131399 [details]
expected image
Comment 4 Simon Fraser (smfr) 2012-03-12 14:05:22 PDT
Hmm, i wonder if the total scrollbar length is different by a pixel or two.
Comment 5 Tim Horton 2012-06-04 13:42:31 PDT
(In reply to comment #4)
> Hmm, i wonder if the total scrollbar length is different by a pixel or two.

Nope:


WKTR:

6/4/12 1:37:25.526 PM WebProcess[76497]: trackRect height: 600
6/4/12 1:37:25.527 PM WebProcess[76497]: thumbRect height: 158

DRT:

6/4/12 1:41:44.910 PM DumpRenderTree[77380]: knobRect height: 159.000000 bounds height: 600.000000


I'm guessing rounding?
Comment 6 Tim Horton 2012-06-04 13:55:37 PDT
(In reply to comment #5)
> I'm guessing rounding?

Yep, WebCore::ScrollbarThemeComposite truncates, DRTMockScroller rounds. (the result of proportion * total track length).

Which do you think we should change, Simon?
Comment 7 Simon Fraser (smfr) 2012-06-04 14:04:05 PDT
Rounding probably makes more sense.
Comment 8 Tim Horton 2012-06-04 15:22:55 PDT
Created attachment 145639 [details]
patch
Comment 9 Tim Horton 2012-06-04 15:25:48 PDT
Thanks!

http://trac.webkit.org/changeset/119431
Comment 10 Darin Adler 2012-06-05 11:28:48 PDT
Comment on attachment 145639 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145639&action=review

> Source/WebCore/platform/ScrollbarThemeComposite.cpp:245
> +    int length = round(proportion * trackLen);

Since we are rounding a float and want the result to be an int, the best function is lroundf.

By using round, we are converting from float to double, then rounding to another double, then converting to an int.

If we used lroundf, we would pass in a float, then get a long as a result, and then we would be good, since long and int are the same type on all the compilers that are used to compile WebKit.
Comment 11 Ryosuke Niwa 2012-06-06 14:17:48 PDT
(In reply to comment #10)
> (From update of attachment 145639 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145639&action=review
> 
> > Source/WebCore/platform/ScrollbarThemeComposite.cpp:245
> > +    int length = round(proportion * trackLen);
> 
> Since we are rounding a float and want the result to be an int, the best function is lroundf.
> 
> By using round, we are converting from float to double, then rounding to another double, then converting to an int.
> 
> If we used lroundf, we would pass in a float, then get a long as a result, and then we would be good, since long and int are the same type on all the compilers that are used to compile WebKit.

I wonder if this contributed to the large number of test failures in Chromium...
Comment 12 Tim Horton 2012-06-06 14:53:08 PDT
(In reply to comment #11)
> I wonder if this contributed to the large number of test failures in Chromium...

Seems unlikely, it is inefficient but shouldn't be wrong. The test failures are surely because this change actually changes the rounding... as it was intended to.
Comment 13 Ryosuke Niwa 2012-06-07 01:10:42 PDT
Anyway, I've reverted the work around and rebaselined all the tests. Took me forever to finish with all other test & build failures :(