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.
Created attachment 186573 [details] Patch
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
(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? :)
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?
Created attachment 186589 [details] Patch
(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.
> 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.
I'm ok with the patch as well.
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.
> 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?
(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.
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? :)
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.
Created attachment 186802 [details] Patch
(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.
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.
Created attachment 187769 [details] Patch
Comment on attachment 187769 [details] Patch Clearing flags on attachment: 187769 Committed r142579: <http://trac.webkit.org/changeset/142579>
All reviewed patches have been landed. Closing bug.