Bug 108922 – Coordinated Graphics: Make CoordinatedGraphicsScene not know contents size.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

Currently, CoordinatedGraphicsScene has two methods to know contents size:
setContentsSize() and setVisibleContentsRect(). Contents size is used when
adjusting a scroll position, but adjustment is not needed because
PageViewportController already adjusts a scroll position. So this patch makes
CoordinatedGraphicsScene not know contents size.

In addition, now DrawingAreaProxy::coordinatedLayerTreeHostProxy() is only used
to get CoordinatedGraphicsScene.

[Dongseong Hwang]

Created attachment 186573 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186573 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Currently, CoordinatedGraphicsScene has two methods to know contents size:
> +        setContentsSize() and setVisibleContentsRect(). Contents size is used when
> +        adjusting a scroll position, but adjustment is not needed because
> +        PageViewportController already adjusts a scroll position. So this patch makes
> +        CoordinatedGraphicsScene not know contents size.

I guess this is not PageViewportController specific... but more about who has the responsibility.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:-63
> -static FloatPoint boundedScrollPosition(const FloatPoint& scrollPosition, const FloatRect& visibleContentRect, const FloatSize& contentSize)
> -{
> -    float scrollPositionX = std::max(scrollPosition.x(), 0.0f);
> -    scrollPositionX = std::min(scrollPositionX, contentSize.width() - visibleContentRect.width());
> -
> -    float scrollPositionY = std::max(scrollPosition.y(), 0.0f);
> -    scrollPositionY = std::min(scrollPositionY, contentSize.height() - visibleContentRect.height());
> -    return FloatPoint(scrollPositionX, scrollPositionY);
> -}
> -

This indeed seems like "view specific" code as as the view might implement bouncing etc, I agree that this might not belong here

[Dongseong Hwang]

(In reply to comment #2)

Thank you for review!

> (From update of attachment 186573 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186573&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        PageViewportController already adjusts a scroll position. So this patch makes
> > +        CoordinatedGraphicsScene not know contents size.
> 
> I guess this is not PageViewportController specific... but more about who has the responsibility.

I'll change the changelog.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:-63
> This indeed seems like "view specific" code as as the view might implement bouncing etc, I agree that this might not belong here

I think so :)

noam, kenneth agreed. could you review? :)

[Mikhail Pozdnyakov]

Comment on attachment 186573 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:192
> +    FloatSize delta = m_scrollPosition - m_renderedContentsScrollPosition;

this is used for fixed elements positioning while scrolling. Have you checked that your patch does not bring regressions there?

[Dongseong Hwang]

Created attachment 186589 [details]
Patch

[Dongseong Hwang]

(In reply to comment #4)
> (From update of attachment 186573 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186573&action=review
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:192
> > +    FloatSize delta = m_scrollPosition - m_renderedContentsScrollPosition;
> 
> this is used for fixed elements positioning while scrolling. Have you checked that your patch does not bring regressions there?

I tested ManualTests/fixed-position.html and ManualTests/nested-fixed-position.html using Qt and EFL Minibrowser.
and I feel the same between before and after.

In theory, it is impossible to make regression, because the major two behaviors are not changed: the way to get m_renderedContentsScrollPosition, and the way to get current scroll position.

[Mikhail Pozdnyakov]

> In theory, it is impossible to make regression
Well my experience say that it is always possible :) but since you've checked I'm OK with the patch.

[Noam Rosenthal]

I'm ok with the patch as well.

[Benjamin Poulain]

Comment on attachment 186589 [details]
Patch

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

I have no problem with this for WK2. Feel free to go ahead with a real review.

> Source/WebCore/ChangeLog:20
> +        Test: ManualTests/fixed-position.html
> +              ManualTests/nested-fixed-position.html

Is there really no ref-test covering this? :)
ManualTests are a very weak tool. If you feel the current ref-tests do not cover everything you need, please improve the test coverage.

[Noam Rosenthal]

> Is there really no ref-test covering this? :)
> ManualTests are a very weak tool. If you feel the current ref-tests do not cover everything you need, please improve the test coverage.

There are no current facilities to test things "in motion" in the UI process; E.g. how would you test that during a particular touch sequence there are no flickers/shakes?

[Benjamin Poulain]

(In reply to comment #10)
> There are no current facilities to test things "in motion" in the UI process; E.g. how would you test that during a particular touch sequence there are no flickers/shakes?

Yep, not much you can do for those.

Fortunately, such bugs are extremely rare.

[Dongseong Hwang]

I wish we can cover this patch automatically. Unfortunately, I couldn't think of a way to test this in an automated fashion.

btw, noam, could you review? :)

[Noam Rosenthal]

Comment on attachment 186589 [details]
Patch

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

>> Source/WebCore/ChangeLog:20
>> +              ManualTests/nested-fixed-position.html
> 
> Is there really no ref-test covering this? :)
> ManualTests are a very weak tool. If you feel the current ref-tests do not cover everything you need, please improve the test coverage.

Please state in the changelog that this can only be tested manually since there is no automated testing facilities for in-motion touch.

[Dongseong Hwang]

Created attachment 186802 [details]
Patch

[Dongseong Hwang]

(In reply to comment #13)
> (From update of attachment 186589 [details])
> Please state in the changelog that this can only be tested manually since there is no automated testing facilities for in-motion touch.

yes, Done. Thank you for improving changelog.

[Noam Rosenthal]

Comment on attachment 186802 [details]
Patch

LGTM. Also signed off by Benjamin for WK2. Please run through EWS before committing; currently patch doesn't apply.

[Dongseong Hwang]

Created attachment 187769 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187769 [details]
Patch

Clearing flags on attachment: 187769

Committed r142579: <http://trac.webkit.org/changeset/142579>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.