Bug 126022 – [CoordinatedGraphics][WK2] Scale factor and scroll position is not being restored properly in a back/forward load

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Thiago de Barros Lacerda]

When user is navigating back/forward to a page that has been scrolled and/or scaled, that page must be shown with its last scroll position and scale factor.
Also, when a user was using the WKView API to set the scale factor (WKViewSetContentScaleFactor) this scale was not being forwarded to WebCore.
Then, when going back/forward to a page HistoryController was not knowing about its real scale factor.
A WebKitAPI test is being added to validate this behaviour.

[Thiago de Barros Lacerda]

Created attachment 219697 [details]
Patch

[Thiago de Barros Lacerda]

*** Bug 119063 has been marked as a duplicate of this bug. ***

[Noam Rosenthal]

Comment on attachment 219697 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        * loader/HistoryController.cpp:
> +        (WebCore::HistoryController::restoreScrollPositionAndViewState):

Explain what you're actually doing to rectify this.

> Source/WebKit2/ChangeLog:13
> +        When user is navigating back/forward to a page that has been scrolled and/or scaled, that page must be shown
> +        with its last scroll position and scale factor.
> +        Also, when a user was using the WKView API to set the scale factor (WKViewSetContentScaleFactor) this scale was
> +        not being forwarded to WebCore. Then, when going back/forward to a page HistoryController was not knowing about
> +        its real scale factor.
> +        A WebKitAPI test is being added to validate this behaviour.

This is a description of the bug. Also describe the solution otherwise the code is hard to follow.

> Source/WebCore/loader/HistoryController.cpp:159
> +        double currentScaleFactor = m_currentItem->pageScaleFactor();
> +        IntPoint currentScrollPoint = m_currentItem->scrollPoint();
> +#if PLATFORM(EFL) || PLATFORM(NIX)
> +        double previousScaleFactor = m_previousItem ? m_previousItem->pageScaleFactor() : currentScaleFactor;
> +        IntPoint previousScrollPoint = m_previousItem ? m_previousItem->scrollPoint() : currentScrollPoint;
> +
> +        if (previousScaleFactor != currentScaleFactor || previousScrollPoint != currentScrollPoint) {
> +#else
>          if (!view->wasScrolledByUser()) {
> -            if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor())
> -                page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint());
> +#endif // PLATFORM(EFL) || PLATFORM(NIX)
> +            if (page && m_frame.isMainFrame() && currentScaleFactor)
> +                page->setPageScaleFactor(currentScaleFactor, currentScrollPoint);
>              else
> -                view->setScrollPosition(m_currentItem->scrollPoint());
> +                view->setScrollPosition(currentScrollPoint);
>          }
>      }

This snippet is a bit spaghettified... I would rather try to figure out how ports where this already works (Mac?) do this, and try to consolidate rather than have port #ifdefs in WebCore.

> Source/WebKit2/UIProcess/PageClient.h:127
> +#if PLATFORM(EFL) || PLATFORM(NIX)
> +    virtual void didChangePageScaleFactor(double scaleFactor) = 0;
> +#endif

I think this can be an empty implementation rather than some #ifdefed thing.

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1147
> +#if PLATFORM(EFL) || PLATFORM(NIX)
> +    if (m_frame == m_frame->page()->mainWebFrame())
> +        m_frame->page()->send(Messages::WebPageProxy::PageDidRequestScroll(m_frame->coreFrame()->loader().history().currentItem()->scrollPoint()));
> +#endif

How is this done in Mac?
If it's not, maybe it's better to make this into a Settings flag rather than an #ifdef block.

[Thiago de Barros Lacerda]

(In reply to comment #3)
> (From update of attachment 219697 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219697&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +        * loader/HistoryController.cpp:
> > +        (WebCore::HistoryController::restoreScrollPositionAndViewState):
> 
> Explain what you're actually doing to rectify this.

OK

> 
> > Source/WebKit2/ChangeLog:13
> > +        When user is navigating back/forward to a page that has been scrolled and/or scaled, that page must be shown
> > +        with its last scroll position and scale factor.
> > +        Also, when a user was using the WKView API to set the scale factor (WKViewSetContentScaleFactor) this scale was
> > +        not being forwarded to WebCore. Then, when going back/forward to a page HistoryController was not knowing about
> > +        its real scale factor.
> > +        A WebKitAPI test is being added to validate this behaviour.
> 
> This is a description of the bug. Also describe the solution otherwise the code is hard to follow.

OK

> 
> > Source/WebCore/loader/HistoryController.cpp:159
> > +        double currentScaleFactor = m_currentItem->pageScaleFactor();
> > +        IntPoint currentScrollPoint = m_currentItem->scrollPoint();
> > +#if PLATFORM(EFL) || PLATFORM(NIX)
> > +        double previousScaleFactor = m_previousItem ? m_previousItem->pageScaleFactor() : currentScaleFactor;
> > +        IntPoint previousScrollPoint = m_previousItem ? m_previousItem->scrollPoint() : currentScrollPoint;
> > +
> > +        if (previousScaleFactor != currentScaleFactor || previousScrollPoint != currentScrollPoint) {
> > +#else
> >          if (!view->wasScrolledByUser()) {
> > -            if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor())
> > -                page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint());
> > +#endif // PLATFORM(EFL) || PLATFORM(NIX)
> > +            if (page && m_frame.isMainFrame() && currentScaleFactor)
> > +                page->setPageScaleFactor(currentScaleFactor, currentScrollPoint);
> >              else
> > -                view->setScrollPosition(m_currentItem->scrollPoint());
> > +                view->setScrollPosition(currentScrollPoint);
> >          }
> >      }
> 
> This snippet is a bit spaghettified... I would rather try to figure out how ports where this already works (Mac?) do this, and try to consolidate rather than have port #ifdefs in WebCore.

I could only reproduce this on ports using CoordinatedGraphics, with TILED_BACKING_STORE and fixed layout enabled.
Then a very specific code path occurs, where FrameView::setWasScrolledByUser is set between Frame::reset and HistoryController::restoreViewStateAndScrollPosition, which makes the zoom and the scrollPosition to do not be restored (since they will not enter in the condition if (!view->wasScrolledByUser()).
I will remove the #if code to a separated method.

> 
> > Source/WebKit2/UIProcess/PageClient.h:127
> > +#if PLATFORM(EFL) || PLATFORM(NIX)
> > +    virtual void didChangePageScaleFactor(double scaleFactor) = 0;
> > +#endif
> 
> I think this can be an empty implementation rather than some #ifdefed thing.

OK

> 
> > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1147
> > +#if PLATFORM(EFL) || PLATFORM(NIX)
> > +    if (m_frame == m_frame->page()->mainWebFrame())
> > +        m_frame->page()->send(Messages::WebPageProxy::PageDidRequestScroll(m_frame->coreFrame()->loader().history().currentItem()->scrollPoint()));
> > +#endif
> 
> How is this done in Mac?
> If it's not, maybe it's better to make this into a Settings flag rather than an #ifdef block.

I figured out that this will not be needed

[Thiago de Barros Lacerda]

Created attachment 219798 [details]
Patch

[Benjamin Poulain]

Comment on attachment 219798 [details]
Patch

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

r- due to platform #ifdefs in HistoryController.

> Source/WebKit2/ChangeLog:3
> +        scalebackhistory

?

> Source/WebCore/loader/HistoryController.cpp:162
> +#if PLATFORM(EFL) || PLATFORM(NIX)
> +    double previousScaleFactor = m_previousItem ? m_previousItem->pageScaleFactor() : m_currentItem->pageScaleFactor();
> +    IntPoint previousScrollPoint = m_previousItem ? m_previousItem->scrollPoint() : m_currentItem->scrollPoint();
> +    return previousScaleFactor != m_currentItem->pageScaleFactor() || previousScrollPoint != m_currentItem->scrollPoint();
> +#else
> +    return !view->wasScrolledByUser();
> +#endif

This #ifdef is not okay. Why can't the code be made common exactly?

The changelog has a little explanation but not enough to determine what is correct.

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:88
> +    if (isSuspended())
> +        return;

This is odd. An explanation in the changelog would be welcome.

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:106
> +    m_contentScaleFactor = scaleFactor;

This is a bad idea. The view presents the information of WebPageProxy. The information about contentScaleFactor already exists on WebPageProxy, I don't think you should duplicate it.

[Thiago de Barros Lacerda]

(In reply to comment #6)
> (From update of attachment 219798 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219798&action=review
> 
> r- due to platform #ifdefs in HistoryController.
> 
> > Source/WebKit2/ChangeLog:3
> > +        scalebackhistory
> 
> ?
> 
> > Source/WebCore/loader/HistoryController.cpp:162
> > +#if PLATFORM(EFL) || PLATFORM(NIX)
> > +    double previousScaleFactor = m_previousItem ? m_previousItem->pageScaleFactor() : m_currentItem->pageScaleFactor();
> > +    IntPoint previousScrollPoint = m_previousItem ? m_previousItem->scrollPoint() : m_currentItem->scrollPoint();
> > +    return previousScaleFactor != m_currentItem->pageScaleFactor() || previousScrollPoint != m_currentItem->scrollPoint();
> > +#else
> > +    return !view->wasScrolledByUser();
> > +#endif
> 
> This #ifdef is not okay. Why can't the code be made common exactly?
> 
> The changelog has a little explanation but not enough to determine what is correct.
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:88
> > +    if (isSuspended())
> > +        return;
> 
> This is odd. An explanation in the changelog would be welcome.
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:106
> > +    m_contentScaleFactor = scaleFactor;
> 
> This is a bad idea. The view presents the information of WebPageProxy. The information about contentScaleFactor already exists on WebPageProxy, I don't think you should duplicate it.

I completely agree with you. This is legacy code, so I just kept it as is.
But having two "scale factors" (one in the WebView and another in the WebPageProxy) really annoys me. I would be more than happy to make a patch to unify those (WebView always getting this info from WebPageProxy). What do you think?

[Gyuyoung Kim]

Thiago de Barros Lacerda, any update ?

EFL port has same problem when using fixed layout. So, if you can't follow up this patch, can I finish this patch ?

[Thiago de Barros Lacerda]

(In reply to comment #8)
> Thiago de Barros Lacerda, any update ?
> 
> EFL port has same problem when using fixed layout. So, if you can't follow up this patch, can I finish this patch ?

Hi Gyuyoung, I have forgotten this bug. I will update the patch and let you know.
Thanks

[Gyuyoung Kim]

(In reply to comment #9)
> (In reply to comment #8)
> > Thiago de Barros Lacerda, any update ?
> > 
> > EFL port has same problem when using fixed layout. So, if you can't follow up this patch, can I finish this patch ?
> 
> Hi Gyuyoung, I have forgotten this bug. I will update the patch and let you know.
> Thanks

Nice, thank you.

[Gyuyoung Kim]

Created attachment 226091 [details]
Work-in-progress

[Gyuyoung Kim]

(In reply to comment #11)
> Created an attachment (id=226091) [details]
> Work-in-progress

Thiago said he doesn't have time to finish this bug. So, let me finish this patch. Thanks. Next patch is going to be uploaded.

[Gyuyoung Kim]

Created attachment 226822 [details]
Patch

[Early Warning System Bot]

Attachment 226822 [details] did not pass style-queue:


Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Csaba Osztrogonác]

Comment on attachment 226822 [details]
Patch

it's not a patch

[Gyuyoung Kim]

Created attachment 226827 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #15)
> (From update of attachment 226822 [details])
> it's not a patch

Oops, something was weird. I upload patch again.

[Gyuyoung Kim]

Benjamin, Noam, could you take a look latest patch ?

[Thiago de Barros Lacerda]

Comment on attachment 226827 [details]
Patch

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

The problem on EFL port seems to be on the PageViewportController, it is setting the scale to 1 every time the page is loaded in didRenderFrame method

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104
> +

I think we don't need this callback, due to http://trac.webkit.org/changeset/162382.
updateViewportSize() call can be moved to WebView::setContentScaleFactor.

[Gyuyoung Kim]

Comment on attachment 226827 [details]
Patch

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

> The problem on EFL port seems to be on the PageViewportController, it is setting the scale to 1 every time the page is loaded in didRenderFrame method

This problem is not fixed by this patch yet. Let me check it soon.

>> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104
>> +
> 
> I think we don't need this callback, due to http://trac.webkit.org/changeset/162382.
> updateViewportSize() call can be moved to WebView::setContentScaleFactor.

> I think we don't need this callback, due to http://trac.webkit.org/changeset/162382.
Looks like that. r162382 already updates m_pageScaleFactor with new scale factor. I will remove it.

[Gyuyoung Kim]

Created attachment 227183 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #19)
> (From update of attachment 226827 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226827&action=review
> 
> The problem on EFL port seems to be on the PageViewportController, it is setting the scale to 1 every time the page is loaded in didRenderFrame method

It looks it is a problem that didRenderFrame() resets existing pageScaleFactor which is not updated. It is really problem when we backward to previous page. The didRenderFrame() overwrites a pageScaleFactor of current scale factor though HistoryController::restoreScrollPositionAndViewState() restores the scale factor of previous page. So, the reset function calls are removed in latest patch. Please let me know if we need to keep those calls.


> > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104
> > +
> 
> I think we don't need this callback, due to http://trac.webkit.org/changeset/162382.
> updateViewportSize() call can be moved to WebView::setContentScaleFactor.

didChangePageScaleFactor() is needed to set an updated pageScaleFactor if we remove the function calls to set an outdated pageScaleFactor.

[Thiago de Barros Lacerda]

(In reply to comment #22)
> (In reply to comment #19)
> > (From update of attachment 226827 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=226827&action=review
> > 
> > The problem on EFL port seems to be on the PageViewportController, it is setting the scale to 1 every time the page is loaded in didRenderFrame method
> 
> It looks it is a problem that didRenderFrame() resets existing pageScaleFactor which is not updated. It is really problem when we backward to previous page. The didRenderFrame() overwrites a pageScaleFactor of current scale factor though HistoryController::restoreScrollPositionAndViewState() restores the scale factor of previous page. So, the reset function calls are removed in latest patch. Please let me know if we need to keep those calls.

I think there is no problem, only EFL uses PageViewportController :)

> 
> 
> > > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104
> > > +
> > 
> > I think we don't need this callback, due to http://trac.webkit.org/changeset/162382.
> > updateViewportSize() call can be moved to WebView::setContentScaleFactor.
> 
> didChangePageScaleFactor() is needed to set an updated pageScaleFactor if we remove the function calls to set an outdated pageScaleFactor.

There is no need for that, since there is no more duplicated scaleFactor in both WebPageProxy and WebView, like in the past. WebView always get the updated scaleFactor because it retrieves directly from WebPageProxy (see WebView::contentScaleFactor). So, didChangeScaleFactor does nothing

[Gyuyoung Kim]

Created attachment 227269 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #23)

> > > > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104
> > > > +
> > > 
> > > I think we don't need this callback, due to http://trac.webkit.org/changeset/162382.
> > > updateViewportSize() call can be moved to WebView::setContentScaleFactor.
> > 
> > didChangePageScaleFactor() is needed to set an updated pageScaleFactor if we remove the function calls to set an outdated pageScaleFactor.
> 
> There is no need for that, since there is no more duplicated scaleFactor in both WebPageProxy and WebView, like in the past. WebView always get the updated scaleFactor because it retrieves directly from WebPageProxy (see WebView::contentScaleFactor). So, didChangeScaleFactor does nothing

I see. I verified we don't need to add didChangeScaleFactor(). However, there is another problem. scroll position is not restored in backward load. According to my investigation, Page::setPageScaleFactor() has not updated scroll position when delegatesScrolling() is enabled. So, WK2 can't know previous scroll position in backward page load.

Page::setPageScaleFactor(float scale, const IntPoint& origin) {
    if (!view->delegatesScrolling())
        view->setScrollPosition(origin);
}

I would call "view->setScrollPosition(origin)" regardless of delegatesScrolling() in order to let WK2 know new scroll position. When I do that, is there any issue ?

[Thiago de Barros Lacerda]

(In reply to comment #25)
> (In reply to comment #23)
> 
> > > > > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104
> > > > > +
> > > > 
> > > > I think we don't need this callback, due to http://trac.webkit.org/changeset/162382.
> > > > updateViewportSize() call can be moved to WebView::setContentScaleFactor.
> > > 
> > > didChangePageScaleFactor() is needed to set an updated pageScaleFactor if we remove the function calls to set an outdated pageScaleFactor.
> > 
> > There is no need for that, since there is no more duplicated scaleFactor in both WebPageProxy and WebView, like in the past. WebView always get the updated scaleFactor because it retrieves directly from WebPageProxy (see WebView::contentScaleFactor). So, didChangeScaleFactor does nothing
> 
> I see. I verified we don't need to add didChangeScaleFactor(). However, there is another problem. scroll position is not restored in backward load. According to my investigation, Page::setPageScaleFactor() has not updated scroll position when delegatesScrolling() is enabled. So, WK2 can't know previous scroll position in backward page load.
> 
> Page::setPageScaleFactor(float scale, const IntPoint& origin) {
>     if (!view->delegatesScrolling())
>         view->setScrollPosition(origin);
> }
> 
> I would call "view->setScrollPosition(origin)" regardless of delegatesScrolling() in order to let WK2 know new scroll position. When I do that, is there any issue ?

Huum... I don't know, better ask benjaminp, he did this patch.

I remember that I faced that problem in Nix as well, what I did that fixed the problem was to change this part of code:

#if USE(TILED_BACKING_STORE)
    if (m_frame->coreFrame()->view())
        fixedVisibleContentRect = m_frame->coreFrame()->view()->fixedVisibleContentRect();
#endif

to something like this:

#if USE(TILED_BACKING_STORE)
#if PLATFORM(NIX)
    fixedVisibleContentRect = IntRect();
#else
    if (m_frame->coreFrame()->view())
        fixedVisibleContentRect = m_frame->coreFrame()->view()->fixedVisibleContentRect();
#endif
#endif

Thus, view->wasScrolledByUser (in HistoryController) was never set and the scroll position is restored properly.
See if that works.

[Gyuyoung Kim]

(In reply to comment #26)
> 
> Thus, view->wasScrolledByUser (in HistoryController) was never set and the scroll position is restored properly.
> See if that works.

Currently, m_wasScrolledByUser() is set as false by FrameView::reset() before calling HistoryController::restoreScrollPositionAndViewState().

  if (!view->wasScrolledByUser()) {
      if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor())
          page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint());
      else
          view->setScrollPosition(m_currentItem->scrollPoint());

Thus, though page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint()) is called, Page::setPageScaleFactor() can't pass the updated scrollPoint() value to WK2 side. That's why I wanna remove "if (!view->delegatesScrolling())" condition in the Page::setPageScaleFactor().

[Gyuyoung Kim]

(In reply to comment #27)
> (In reply to comment #26)
> > 
> > Thus, view->wasScrolledByUser (in HistoryController) was never set and the scroll position is restored properly.
> > See if that works.
> 
> Currently, m_wasScrolledByUser() is set as false by FrameView::reset() before calling HistoryController::restoreScrollPositionAndViewState().
> 
>   if (!view->wasScrolledByUser()) {
>       if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor())
>           page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint());
>       else
>           view->setScrollPosition(m_currentItem->scrollPoint());
> 
> Thus, though page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint()) is called, Page::setPageScaleFactor() can't pass the updated scrollPoint() value to WK2 side. That's why I wanna remove "if (!view->delegatesScrolling())" condition in the Page::setPageScaleFactor().

Benjamin, what do you think about above comments ?

[Gyuyoung Kim]

Comment on attachment 227269 [details]
Patch

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

> Source/WebCore/page/Page.cpp:-702
> -            if (!view->delegatesScrolling())

Tim, could you check if this change may influence on mac and iOS ports ?

[Gyuyoung Kim]

(In reply to comment #29)
> (From update of attachment 227269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227269&action=review
> 
> > Source/WebCore/page/Page.cpp:-702
> > -            if (!view->delegatesScrolling())
> 
> Tim, could you check if this change may influence on mac and iOS ports ?

Horton, any comment about my question ?

[Gyuyoung Kim]

(In reply to comment #29)
> (From update of attachment 227269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227269&action=review
> 
> > Source/WebCore/page/Page.cpp:-702
> > -            if (!view->delegatesScrolling())
> 
> Tim, could you check if this change may influence on mac and iOS ports ?

Horton, any comment about my question ?

[Simon Fraser (smfr)]

Comment on attachment 227269 [details]
Patch

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

> Source/WebCore/page/Page.cpp:702
> +            view->setScrollPosition(origin);

Did you look at the history of this code? I'm pretty sure we added the if (!view->delegatesScrolling()) test to fix iOS.

[Gyuyoung Kim]

Comment on attachment 227269 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:702
>> +            view->setScrollPosition(origin);
> 
> Did you look at the history of this code? I'm pretty sure we added the if (!view->delegatesScrolling()) test to fix iOS.

Simon, if so, let me try to find other way to fix this problem for EFL port. I'm going to update my opinion. Thanks.

[Gyuyoung Kim]

Created attachment 230159 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 230159 [details]
Patch

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

> Source/WebCore/page/Page.cpp:718
>              if (!view->delegatesScrolling())

Simon, if we don't call view->setScrollPosition(origin), we can't pass previous scroll position to WK2 side.

Below view->setScrollPosition(origin) calls ScrollView::setScrollPosition(scrollPoint) eventually. Then, it calls WebChromeClient::delegatedScrollRequested() in WK2. Then, WK2 UIProcess can handle updated scroll position and scale. However, r165652 and r165913 have prohibited to call it. It looks WK2 ports(which use tiled backing store) need to call "view->setScrollPosition(origin)" in order to pass stored scroll position to WK2 implementation.


    void ScrollView::setScrollPosition(const IntPoint& scrollPoint) {
    ...
    #if USE(TILED_BACKING_STORE)
        if (delegatesScrolling()) {
            hostWindow()->delegatedScrollRequested(scrollPoint);
            return;
        }
    #endif
    ...
    }

    #if USE(TILED_BACKING_STORE)
    void WebChromeClient::delegatedScrollRequested(const IntPoint& scrollOffset)
    {
        m_page->pageDidRequestScroll(scrollOffset);
    }
    #endif

[Benjamin Poulain]

(In reply to comment #35)
> (From update of attachment 230159 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230159&action=review
> 
> > Source/WebCore/page/Page.cpp:718
> >              if (!view->delegatesScrolling())
> 
> Simon, if we don't call view->setScrollPosition(origin), we can't pass previous scroll position to WK2 side.
> 
> Below view->setScrollPosition(origin) calls ScrollView::setScrollPosition(scrollPoint) eventually. Then, it calls WebChromeClient::delegatedScrollRequested() in WK2. Then, WK2 UIProcess can handle updated scroll position and scale. However, r165652 and r165913 have prohibited to call it. It looks WK2 ports(which use tiled backing store) need to call "view->setScrollPosition(origin)" in order to pass stored scroll position to WK2 implementation.

There is a general conflict about how the scroll position is set when using delegateScrolling. With delegateScrolling, there are two sources that can set the scroll position: the UI side or the engine side.

There are races between the two, there is a need for differentiating the source, and we need a way to solve conflicts. There is such things in WebKit today.

I do agree with Simon that changing the scroll position when changing the scale factor on page does not seem to make much sense with delegate scrolling.

I agree there is a bug here. I disagree the #ifdef USE(TILED_BACKING_STORE) of this patch is a solution.

I think we may need to split the paths between scrolling requested from the WebKit layers against scrolling requested from the WebCore layer. Scrolling from the WebKit layer is imperative. Scrolling from the WebCore layer needs to go through conflicts resolution.

I don't have the code in front of me to find a solution, but I can have a look next week.

[Gyuyoung Kim]

Comment on attachment 230159 [details]
Patch

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

>>> Source/WebCore/page/Page.cpp:718
>>>              if (!view->delegatesScrolling())
>> 
>> Simon, if we don't call view->setScrollPosition(origin), we can't pass previous scroll position to WK2 side.
>> 
>> Below view->setScrollPosition(origin) calls ScrollView::setScrollPosition(scrollPoint) eventually. Then, it calls WebChromeClient::delegatedScrollRequested() in WK2. Then, WK2 UIProcess can handle updated scroll position and scale. However, r165652 and r165913 have prohibited to call it. It looks WK2 ports(which use tiled backing store) need to call "view->setScrollPosition(origin)" in order to pass stored scroll position to WK2 implementation.
>> 
>> 
>>     void ScrollView::setScrollPosition(const IntPoint& scrollPoint) {
>>     ...
>>     #if USE(TILED_BACKING_STORE)
>>         if (delegatesScrolling()) {
>>             hostWindow()->delegatedScrollRequested(scrollPoint);
>>             return;
>>         }
>>     #endif
>>     ...
>>     }
>> 
>>     #if USE(TILED_BACKING_STORE)
>>     void WebChromeClient::delegatedScrollRequested(const IntPoint& scrollOffset)
>>     {
>>         m_page->pageDidRequestScroll(scrollOffset);
>>     }
>>     #endif
> 
> There is a general conflict about how the scroll position is set when using delegateScrolling. With delegateScrolling, there are two sources that can set the scroll position: the UI side or the engine side.
> 
> There are races between the two, there is a need for differentiating the source, and we need a way to solve conflicts. There is such things in WebKit today.
> 
> I do agree with Simon that changing the scroll position when changing the scale factor on page does not seem to make much sense with delegate scrolling.
> 
> I agree there is a bug here. I disagree the #ifdef USE(TILED_BACKING_STORE) of this patch is a solution.
> 
> I think we may need to split the paths between scrolling requested from the WebKit layers against scrolling requested from the WebCore layer. Scrolling from the WebKit layer is imperative. Scrolling from the WebCore layer needs to go through conflicts resolution.
> 
> I don't have the code in front of me to find a solution, but I can have a look next week.

Benjamin, will you upload your patch to this bug or file a new bug ? Anyway, I agree to use #if USE(TILED_BACKING_STORE) is not good solution. Please let me know your way to me. Thanks.

[Gyuyoung Kim]

Created attachment 230477 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 230477 [details]
Patch

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

> Source/WebCore/page/Page.cpp:755
> +            view->hostWindow()->delegatedScrollRequested(origin);

>> I think we may need to split the paths between scrolling requested from the WebKit layers against scrolling requested from the WebCore layer.

Benjamin, I'm not sure if this is one of the paths you think though, can't we split the path like this ?

I think we don't need to call the delegatedScrollRequested() in ScrollView::setScrollPosition(). If we call it here, we may make a path to split between scrolling requests from Webkit layer, isn't it ?

[Benjamin Poulain]

Comment on attachment 230477 [details]
Patch

We should really get rid of TILED_BACKING_STORE.
OS X and iOS also have tiled backing store, TILED_BACKING_STORE is mostly duplicated code at this point.

A quick grep for TILED_BACKING_STORE shows it is actually duplicating a lot of features that have been upstreamed when iOS was upstreamed. In some cases, the code behind TILED_BACKING_STORE looks very suspicious.
Now is not the right time though.

I have serious doubt this is patch correct. For example, the call to Page::setPageScaleFactor() could come from WebPage::scalePage(), in which case calling back into scroll requested does not make any sense.
I don't want to block you on this regression forever, the impact not any worse than the other TILED_BACKING_STORE stuff  -> r+ for now.

[Gyuyoung Kim]

(In reply to comment #40)
> (From update of attachment 230477 [details])
> We should really get rid of TILED_BACKING_STORE.
> OS X and iOS also have tiled backing store, TILED_BACKING_STORE is mostly duplicated code at this point.
> 
> A quick grep for TILED_BACKING_STORE shows it is actually duplicating a lot of features that have been upstreamed when iOS was upstreamed. In some cases, the code behind TILED_BACKING_STORE looks very suspicious.
> Now is not the right time though.

I also think we need to do refactoring for the TILED_BACKING_STORE align with iOS or mac port's implementation. I'm going to talk with my co-worker how to refactor the tiled backing store.
 
> I have serious doubt this is patch correct. For example, the call to Page::setPageScaleFactor() could come from WebPage::scalePage(), in which case calling back into scroll requested does not make any sense.
> I don't want to block you on this regression forever, the impact not any worse than the other TILED_BACKING_STORE stuff  -> r+ for now.

Thank you for helping to my development. When we modify it, let me try to consider your concern together.

[WebKit Commit Bot]

Comment on attachment 230477 [details]
Patch

Clearing flags on attachment: 230477

Committed r168458: <http://trac.webkit.org/changeset/168458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.