Bug 130028 - The scrolledContentOffset method should handle the hasOverflowClip check
Summary: The scrolledContentOffset method should handle the hasOverflowClip check
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords: AdobeTracked
Depends on:
Blocks:
 
Reported: 2014-03-10 10:35 PDT by Radu Stavila
Modified: 2014-03-13 09:33 PDT (History)
3 users (show)

See Also:


Attachments
Patch (14.02 KB, patch)
2014-03-11 08:58 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Radu Stavila 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.
Comment 1 Radu Stavila 2014-03-11 08:58:27 PDT
Created attachment 226429 [details]
Patch
Comment 2 zalan 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)
Comment 3 Radu Stavila 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.
Comment 4 zalan 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?
Comment 5 Radu Stavila 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
Comment 6 Csaba Osztrogonác 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>
Comment 7 Csaba Osztrogonác 2014-03-13 09:33:33 PDT
All reviewed patches have been landed.  Closing bug.