Bug 126022 - [CoordinatedGraphics][WK2] Scale factor and scroll position is not being restored properly in a back/forward load
Summary: [CoordinatedGraphics][WK2] Scale factor and scroll position is not being rest...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
: 119063 (view as bug list)
Depends on: 126243
Blocks: 131831 136999
  Show dependency treegraph
 
Reported: 2013-12-19 15:44 PST by Thiago de Barros Lacerda
Modified: 2014-09-22 04:43 PDT (History)
18 users (show)

See Also:


Attachments
Patch (19.47 KB, patch)
2013-12-19 16:02 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Patch (20.67 KB, patch)
2013-12-20 15:03 PST, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Work-in-progress (17.00 KB, patch)
2014-03-07 00:20 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (3.59 KB, patch)
2014-03-15 09:08 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (15.38 KB, patch)
2014-03-15 18:15 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (16.65 KB, patch)
2014-03-19 08:14 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (16.99 KB, patch)
2014-03-20 00:49 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (12.53 KB, patch)
2014-04-25 03:19 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (13.42 KB, patch)
2014-04-30 07:04 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2013-12-19 15:44:55 PST
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.
Comment 1 Thiago de Barros Lacerda 2013-12-19 16:02:36 PST
Created attachment 219697 [details]
Patch
Comment 2 Thiago de Barros Lacerda 2013-12-19 16:06:32 PST
*** Bug 119063 has been marked as a duplicate of this bug. ***
Comment 3 Noam Rosenthal 2013-12-20 05:47:06 PST
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.
Comment 4 Thiago de Barros Lacerda 2013-12-20 14:38:12 PST
(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
Comment 5 Thiago de Barros Lacerda 2013-12-20 15:03:53 PST
Created attachment 219798 [details]
Patch
Comment 6 Benjamin Poulain 2013-12-20 15:31:40 PST
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.
Comment 7 Thiago de Barros Lacerda 2013-12-22 14:42:31 PST
(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?
Comment 8 Gyuyoung Kim 2014-03-04 20:06:50 PST
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 ?
Comment 9 Thiago de Barros Lacerda 2014-03-05 18:30:42 PST
(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
Comment 10 Gyuyoung Kim 2014-03-05 19:01:49 PST
(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.
Comment 11 Gyuyoung Kim 2014-03-07 00:20:32 PST
Created attachment 226091 [details]
Work-in-progress
Comment 12 Gyuyoung Kim 2014-03-07 00:22:36 PST
(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.
Comment 13 Gyuyoung Kim 2014-03-15 09:08:18 PDT
Created attachment 226822 [details]
Patch
Comment 14 Early Warning System Bot 2014-03-15 09:10:40 PDT
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.
Comment 15 Csaba Osztrogonác 2014-03-15 10:39:51 PDT
Comment on attachment 226822 [details]
Patch

it's not a patch
Comment 16 Gyuyoung Kim 2014-03-15 18:15:18 PDT
Created attachment 226827 [details]
Patch
Comment 17 Gyuyoung Kim 2014-03-15 19:20:35 PDT
(In reply to comment #15)
> (From update of attachment 226822 [details])
> it's not a patch

Oops, something was weird. I upload patch again.
Comment 18 Gyuyoung Kim 2014-03-17 01:32:35 PDT
Benjamin, Noam, could you take a look latest patch ?
Comment 19 Thiago de Barros Lacerda 2014-03-17 07:19:06 PDT
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.
Comment 20 Gyuyoung Kim 2014-03-17 09:24:06 PDT
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.
Comment 21 Gyuyoung Kim 2014-03-19 08:14:26 PDT
Created attachment 227183 [details]
Patch
Comment 22 Gyuyoung Kim 2014-03-19 08:35:07 PDT
(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.
Comment 23 Thiago de Barros Lacerda 2014-03-19 10:52:00 PDT
(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
Comment 24 Gyuyoung Kim 2014-03-20 00:49:19 PDT
Created attachment 227269 [details]
Patch
Comment 25 Gyuyoung Kim 2014-03-20 00:54:18 PDT
(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 ?
Comment 26 Thiago de Barros Lacerda 2014-03-20 14:41:29 PDT
(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.
Comment 27 Gyuyoung Kim 2014-03-20 22:07:30 PDT
(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().
Comment 28 Gyuyoung Kim 2014-03-22 17:33:28 PDT
(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 ?
Comment 29 Gyuyoung Kim 2014-03-31 01:08:27 PDT
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 ?
Comment 30 Gyuyoung Kim 2014-04-08 22:46:29 PDT
(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 ?
Comment 31 Gyuyoung Kim 2014-04-08 22:47:33 PDT
(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 ?
Comment 32 Simon Fraser (smfr) 2014-04-22 23:25:09 PDT
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.
Comment 33 Gyuyoung Kim 2014-04-23 01:25:20 PDT
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.
Comment 34 Gyuyoung Kim 2014-04-25 03:19:10 PDT
Created attachment 230159 [details]
Patch
Comment 35 Gyuyoung Kim 2014-04-25 03:28:09 PDT
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
Comment 36 Benjamin Poulain 2014-04-26 17:54:39 PDT
(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.
Comment 37 Gyuyoung Kim 2014-04-27 18:12:04 PDT
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.
Comment 38 Gyuyoung Kim 2014-04-30 07:04:49 PDT
Created attachment 230477 [details]
Patch
Comment 39 Gyuyoung Kim 2014-04-30 07:11:42 PDT
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 ?
Comment 40 Benjamin Poulain 2014-05-07 15:55:53 PDT
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.
Comment 41 Gyuyoung Kim 2014-05-07 18:10:31 PDT
(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.
Comment 42 WebKit Commit Bot 2014-05-07 18:42:46 PDT
Comment on attachment 230477 [details]
Patch

Clearing flags on attachment: 230477

Committed r168458: <http://trac.webkit.org/changeset/168458>
Comment 43 WebKit Commit Bot 2014-05-07 18:42:57 PDT
All reviewed patches have been landed.  Closing bug.