Bug 130028

Summary: The scrolledContentOffset method should handle the hasOverflowClip check
Product: WebKit Reporter: Radu Stavila <stavila>
Component: CSSAssignee: Radu Stavila <stavila>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ossy, zalan
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Radu Stavila
Reported 2014-03-10 10:35:08 PDT
Currently, all call sites of scrolledContentOffset are performing the hasOverflowClip check. This check should be moved into the scrolledContentOffset itself.
Attachments
Patch (14.02 KB, patch)
2014-03-11 08:58 PDT, Radu Stavila 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radu Stavila
Comment 1 2014-03-11 08:58:27 PDT
Created attachment 226429 [details] Patch
alan
Comment 2 2014-03-11 09:15:03 PDT
Comment on attachment 226429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226429&action=review > Source/WebCore/rendering/RenderBlock.cpp:2512 > + LayoutPoint offsetFromRepaintContainer = roundedLayoutPoint(transformState.mappedPoint()) - scrolledContentOffset(); Is there an intention of actual rounding here? If not I'd rather use the explicit LayoutPoint(const FloatPoint&) c'tor instead. (roundedLayoutPoint() sadly doesnt do any rounding -I'll be cleaning that up at some point)
Radu Stavila
Comment 3 2014-03-11 09:44:27 PDT
Comment on attachment 226429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226429&action=review >> Source/WebCore/rendering/RenderBlock.cpp:2512 >> + LayoutPoint offsetFromRepaintContainer = roundedLayoutPoint(transformState.mappedPoint()) - scrolledContentOffset(); > > Is there an intention of actual rounding here? If not I'd rather use the explicit LayoutPoint(const FloatPoint&) c'tor instead. (roundedLayoutPoint() sadly doesnt do any rounding -I'll be cleaning that up at some point) That is part of the existing code, the change here is removing the hasOverflowClip call, which was moved to scrolledContentOffset.
alan
Comment 4 2014-03-11 20:40:13 PDT
(In reply to comment #3) > (From update of attachment 226429 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=226429&action=review > > >> Source/WebCore/rendering/RenderBlock.cpp:2512 > >> + LayoutPoint offsetFromRepaintContainer = roundedLayoutPoint(transformState.mappedPoint()) - scrolledContentOffset(); > > > > Is there an intention of actual rounding here? If not I'd rather use the explicit LayoutPoint(const FloatPoint&) c'tor instead. (roundedLayoutPoint() sadly doesnt do any rounding -I'll be cleaning that up at some point) > > That is part of the existing code, the change here is removing the hasOverflowClip call, which was moved to scrolledContentOffset. Are you planning to fix it at some point?
Radu Stavila
Comment 5 2014-03-12 07:18:30 PDT
Comment on attachment 226429 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226429&action=review >>>> Source/WebCore/rendering/RenderBlock.cpp:2512 >>>> + LayoutPoint offsetFromRepaintContainer = roundedLayoutPoint(transformState.mappedPoint()) - scrolledContentOffset(); >>> >>> Is there an intention of actual rounding here? If not I'd rather use the explicit LayoutPoint(const FloatPoint&) c'tor instead. (roundedLayoutPoint() sadly doesnt do any rounding -I'll be cleaning that up at some point) >> >> That is part of the existing code, the change here is removing the hasOverflowClip call, which was moved to scrolledContentOffset. > > Are you planning to fix it at some point? Yes - https://bugs.webkit.org/show_bug.cgi?id=130128
Csaba Osztrogonác
Comment 6 2014-03-13 09:33:26 PDT
Comment on attachment 226429 [details] Patch Clearing flags on attachment: 226429 Committed r165537: <http://trac.webkit.org/changeset/165537>
Csaba Osztrogonác
Comment 7 2014-03-13 09:33:33 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.