Bug 129981 - Change scrollOffsetForFixedPosition() to do LayoutUnit math
Summary: Change scrollOffsetForFixedPosition() to do LayoutUnit math
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-08 14:44 PST by Simon Fraser (smfr)
Modified: 2014-03-12 11:15 PDT (History)
16 users (show)

See Also:

Attachments
Patch (17.42 KB, patch)
2014-03-08 14:46 PST, Simon Fraser (smfr) 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (982.11 KB, application/zip)
2014-03-08 15:56 PST, Build Bot 		no flags Details
Patch (519.07 KB, patch)
2014-03-12 10:16 PDT, Simon Fraser (smfr) 		no flags Details | Formatted Diff | Diff
Patch (519.63 KB, patch)
2014-03-12 10:42 PDT, Simon Fraser (smfr) 		bdakin: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2014-03-08 14:44:26 PST 
Change scrollOffsetForFixedPosition() to do LayoutUnit math
Comment 1 Simon Fraser (smfr) 2014-03-08 14:46:18 PST 
Created attachment 226237 [details]
Patch
Comment 2 zalan 2014-03-08 15:44:58 PST 
Comment on attachment 226237 [details]
Patch

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

Why do we round/floor to layout unit?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1121
> +            LayoutPoint absoluteContainerOffset = flooredLayoutPoint(paintContainer->localToAbsolute(FloatPoint()));

round vs floor?
Comment 3 zalan 2014-03-08 15:45:00 PST 
Comment on attachment 226237 [details]
Patch

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

Why do we round/floor to layout unit?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1121
> +            LayoutPoint absoluteContainerOffset = flooredLayoutPoint(paintContainer->localToAbsolute(FloatPoint()));

round vs floor?
Comment 4 Build Bot 2014-03-08 15:56:20 PST 
Comment on attachment 226237 [details]
Patch

Attachment 226237 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6309407117803520

New failing tests:
platform/mac-wk2/tiled-drawing/fixed-background/fixed-body-background-zoomed.html
Comment 5 Build Bot 2014-03-08 15:56:24 PST 
Created attachment 226239 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 6 Simon Fraser (smfr) 2014-03-12 10:16:58 PDT 
Created attachment 226523 [details]
Patch
Comment 7 Simon Fraser (smfr) 2014-03-12 10:42:18 PDT 
Created attachment 226526 [details]
Patch
Comment 8 zalan 2014-03-12 11:00:55 PDT 
Looks good.
Comment 9 Beth Dakin 2014-03-12 11:01:47 PDT 
Comment on attachment 226526 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:1542
> +    viewportRect.setLocation(LayoutPoint() + scrollOffsetForFixedPosition());

Just out of curiosity, why is this better than toLayoutPoint() ?
Comment 10 Simon Fraser (smfr) 2014-03-12 11:15:57 PDT 
https://trac.webkit.org/r165484