Bug 136999 – [EFL] Restore previous scroll position using restoreViewState()

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

EFL port begins to support FrameLoaderClient::restoreViewState() to be aligned with iOS port. Scroll position update behavior has been optimized for iOS and Mac port. That's why EFL port tries to follow iOS implementation.

In addition, EFL port has supported to use ScrollingCoordinatorCoordinatedGraphics::requestScrollPositionUpdate() since r173785. Thus, EFL port doesn't need to have own code path to pass scroll position to WK2, because ScrollView::setScrollPosition() can support EFL port since r173785.

[Gyuyoung Kim]

Created attachment 238477 [details]
Patch

[Gyuyoung Kim]

Created attachment 238478 [details]
Patch

[Gyuyoung Kim]

CC'ing Darin and Benjamin

[Gyuyoung Kim]

Created attachment 238482 [details]
Patch

[Grzegorz Czajkowski]

Comment on attachment 238482 [details]
Patch

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

I got the positive impression while I looking at the code. However, I'd suggest reusing the iOS implementation instead of duplicating WebFrameLoaderClient' saveViewStateToItem() and restoreViewState() in EFL.

> Source/WebCore/loader/HistoryController.cpp:146
>      // through to the client.
>      m_frame.loader().client().restoreViewState();

Due to EFL is another port that is going to use this functionality, it might be good opportunity to follow FIXME suggestion and move that part to WebCore.

[Gyuyoung Kim]

Comment on attachment 238482 [details]
Patch

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

>> Source/WebCore/loader/HistoryController.cpp:146
>>      m_frame.loader().client().restoreViewState();
> 
> Due to EFL is another port that is going to use this functionality, it might be good opportunity to follow FIXME suggestion and move that part to WebCore.

yes, right. However, I thought it would be good if new bug handles it.

[Gyuyoung Kim]

(In reply to comment #5)
> (From update of attachment 238482 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238482&action=review
> 
> I got the positive impression while I looking at the code. However, I'd suggest reusing the iOS implementation instead of duplicating WebFrameLoaderClient' saveViewStateToItem() and restoreViewState() in EFL.

I already thought about that. However there is already other implementation on those function. If we share iOS implementation, we need to consider how to handle existing implementation.


#if !PLATFORM(IOS)
void WebFrameLoaderClient::saveViewStateToItem(HistoryItem*)
{
    notImplemented();
}

void WebFrameLoaderClient::restoreViewState()
{
    // Inform the UI process of the scale factor.
    double scaleFactor = m_frame->coreFrame()->loader().history().currentItem()->pageScaleFactor();

    // A scale factor of 0 means the history item has the default scale factor, thus we do not need to update it.
    if (scaleFactor)
        m_frame->page()->send(Messages::WebPageProxy::PageScaleFactorDidChange(scaleFactor));

    // FIXME: This should not be necessary. WebCore should be correctly invalidating
    // the view on restores from the back/forward cache.
    if (m_frame == m_frame->page()->mainWebFrame())
        m_frame->page()->drawingArea()->setNeedsDisplay();
}
#endif

[Gyuyoung Kim]

Created attachment 238713 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #7)
> (In reply to comment #5)
> > (From update of attachment 238482 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=238482&action=review
> > 
> > I got the positive impression while I looking at the code. However, I'd suggest reusing the iOS implementation instead of duplicating WebFrameLoaderClient' saveViewStateToItem() and restoreViewState() in EFL.
> 
> I already thought about that. However there is already other implementation on those function. If we share iOS implementation, we need to consider how to handle existing implementation.
> 
> 
> #if !PLATFORM(IOS)
> void WebFrameLoaderClient::saveViewStateToItem(HistoryItem*)
> {
>     notImplemented();
> }
> 
> void WebFrameLoaderClient::restoreViewState()
> {
>     // Inform the UI process of the scale factor.
>     double scaleFactor = m_frame->coreFrame()->loader().history().currentItem()->pageScaleFactor();
> 
>     // A scale factor of 0 means the history item has the default scale factor, thus we do not need to update it.
>     if (scaleFactor)
>         m_frame->page()->send(Messages::WebPageProxy::PageScaleFactorDidChange(scaleFactor));
> 
>     // FIXME: This should not be necessary. WebCore should be correctly invalidating
>     // the view on restores from the back/forward cache.
>     if (m_frame == m_frame->page()->mainWebFrame())
>         m_frame->page()->drawingArea()->setNeedsDisplay();
> }
> #endif

As I said in comment #7, Mac and GTK port use restoreViewState() in WebFrameLoaderClient.cpp. Thus it looks this patch can't move iOS implementation to WebFrameLoaderClient.cpp yet. Besides restoreViewState() can be changed by each port in near future. I think it would be good if each port has own implementation at the moment. I upload same patch again.

[Benjamin Poulain]

Comment on attachment 238713 [details]
Patch

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

I am quite confused by this patch... If there are good reasons for the concerns I raised, you should improve the changelog to give the rationale.

> Source/WebCore/page/Page.cpp:-742
> -            if (!view->delegatesScrolling())
> -                view->setScrollPosition(origin);

The check for delegatesScrolling() is necessary, see r165652 and r165913.

You cannot have the delegate changing the position asynchronously followed by a message telling it to go back to the old position.

> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:118
> -    if (!frameView->delegatesScrolling())
> +    if (!frameView->delegatesScrolling() || frameView->wasScrolledByUser())

This change is not explained in your changelog.

> Source/WebKit2/WebProcess/WebCoreSupport/efl/WebFrameLoaderClientEfl.cpp:2
> + * Copyright (C) 2014 Samsung Electronics. All rights reserved.

Gosh. You can't just copy Apple's code and put your copyright on top.

> Source/WebKit2/WebProcess/WebCoreSupport/efl/WebFrameLoaderClientEfl.cpp:57
> +void WebFrameLoaderClient::saveViewStateToItem(HistoryItem* historyItem)
> +{
> +    if (m_frame->isMainFrame())
> +        m_frame->page()->savePageState(*historyItem);
> +}
> +
> +void WebFrameLoaderClient::restoreViewState()
> +{
> +    Frame& frame = *m_frame->coreFrame();
> +    HistoryItem* currentItem = frame.loader().history().currentItem();
> +    if (FrameView* view = frame.view()) {
> +        if (m_frame->isMainFrame())
> +            m_frame->page()->restorePageState(*currentItem);
> +        else if (!view->wasScrolledByUser())
> +            view->setScrollPosition(currentItem->scrollPoint());
> +    }
> +}

Since this code is now shared, you can just move it to WebFrameLoaderClient.

> Source/WebKit2/WebProcess/WebPage/efl/WebPageEfl.cpp:236
> +    // When a HistoryItem is cleared, its scale factor and scroll point are set to zero. We should not try to restore the other
> +    // parameters in those conditions.
> +    float pageScaleFactor = historyItem.pageScaleFactor();
> +    if (!pageScaleFactor)
> +        return;
> +
> +    scalePage(pageScaleFactor, historyItem.scrollPoint());

I don't get this.

The reason state restoration is going up to the WebKit layer in iOS is the asynchronous model with delegate scrolling.

Here you are going back to WebCore through scalePage(), why go to the WebKit layer at all? Wouldn't HistoryController::restoreScrollPositionAndViewState() do everything you need?

[Gyuyoung Kim]

Comment on attachment 238713 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:-742
>> -                view->setScrollPosition(origin);
> 
> The check for delegatesScrolling() is necessary, see r165652 and r165913.
> 
> You cannot have the delegate changing the position asynchronously followed by a message telling it to go back to the old position.

ok, I didn't know iOS needed to use "!view->delegatesScrolling()" condition. In next patch, I will keep this code.

>> Source/WebCore/page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.cpp:118
>> +    if (!frameView->delegatesScrolling() || frameView->wasScrolledByUser())
> 
> This change is not explained in your changelog.

This is a try to fix regression by r173785. I think this should be fixed by new bug. A bug is filed - https://bugs.webkit.org/show_bug.cgi?id=137336.

>> Source/WebKit2/WebProcess/WebCoreSupport/efl/WebFrameLoaderClientEfl.cpp:2
>> + * Copyright (C) 2014 Samsung Electronics. All rights reserved.
> 
> Gosh. You can't just copy Apple's code and put your copyright on top.

Oops. Sorry about that. This file is going to be removed in next patch.

>> Source/WebKit2/WebProcess/WebCoreSupport/efl/WebFrameLoaderClientEfl.cpp:57
>> +}
> 
> Since this code is now shared, you can just move it to WebFrameLoaderClient.

Fixed.

>> Source/WebKit2/WebProcess/WebPage/efl/WebPageEfl.cpp:236
>> +    scalePage(pageScaleFactor, historyItem.scrollPoint());
> 
> I don't get this.
> 
> The reason state restoration is going up to the WebKit layer in iOS is the asynchronous model with delegate scrolling.
> 
> Here you are going back to WebCore through scalePage(), why go to the WebKit layer at all? Wouldn't HistoryController::restoreScrollPositionAndViewState() do everything you need?

Thank you for your nice catch ! I missed that scalePage() calls m_page->setPageScaleFactor() of WebCore. Fixed.

[Gyuyoung Kim]

Created attachment 239095 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #10)
> (From update of attachment 238713 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238713&action=review
> 
> I am quite confused by this patch... If there are good reasons for the concerns I raised, you should improve the changelog to give the rationale.

Benjamin, I update my patch title and description. Please take a look again !

[Benjamin Poulain]

Comment on attachment 239095 [details]
Patch

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

Those are really two fixes in one patch.

The WebCore part seems to be a step in the right direction. My only question has to do with bypassing ScrollView, if there is a good rationale, it should be explained in the changelog.

The WebKit2 part still don't make any sense as it is implemented. The code looks correct but the semantic does not fit with anything I know in the architecture. Again, this may be correct but it would need an improved changelog.

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

I find it odd that you are calling requestScrollPositionUpdate() directly instead of going through setScrollPosition(). What's the reason for that?

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

ditto.

> Source/WebKit2/WebProcess/WebPage/efl/WebPageEfl.cpp:237
> +    send(Messages::WebPageProxy::PageScaleFactorDidChange(pageScaleFactor));

This does not really make sense. PageScaleFactorDidChange should only happen after the Page's pageScaleFactor has been updated.

Where is it updated here?

[Benjamin Poulain]

By the way, do not hesitate to poke me on IRC. I am swamped in review emails so I frequently miss updates on bugzilla.

[Gyuyoung Kim]

Comment on attachment 239095 [details]
Patch

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

I try to improve my ChangeLog. If it is still poor, please let me know. And let me try to ping you on irc, not bugzilla from now on.

>> Source/WebCore/page/Page.cpp:745
>> +                view->requestScrollPositionUpdate(origin);
> 
> I find it odd that you are calling requestScrollPositionUpdate() directly instead of going through setScrollPosition(). What's the reason for that?

In my first patch, I called setScrollPosition(origin) instead of requestScallPositionUpdate() directly. But, as you said, mac port and iOS port needs to call setScrollPosition() only when delegatesScrolling() is disabled. However, EFL port based on USE(TILED_BACKING_STORE) supports setScrollPosition() through requestScrollPositionUpdate() only when delegatesScrolling() is enabled. That's why I call requestScrollPositionUpdate() directly.

>> Source/WebKit2/WebProcess/WebPage/efl/WebPageEfl.cpp:237
>> +    send(Messages::WebPageProxy::PageScaleFactorDidChange(pageScaleFactor));
> 
> This does not really make sense. PageScaleFactorDidChange should only happen after the Page's pageScaleFactor has been updated.
> 
> Where is it updated here?

I'm sorry. I think here needs to call scalePage() again. Because this is to restore previous pageScaleFactor and scrollPosition through WebCore's HistoryController. So we need to update the pageScalFactor of historyItem both WebProcess and UIProcess. Existing EFL code path has updated the pageScaleFactor of historyItem to WebCore and WebKit2 through below path. However, this patch tries to restore it using restorePageState() only, thus I think pageScaleFactor|scrollPosition of historyItem needs to apply both WebCore and UIProcess through scalePage().

#if !PLATFORM(IOS)
    // Don't restore scroll point on iOS as FrameLoaderClient::restoreViewState() does that.
    if (view && !view->wasScrolledByUser()) {
        Page* page = m_frame.page();
        if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor())
            page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint());
        else
            view->setScrollPosition(m_currentItem->scrollPoint());
    }
#endif

[Gyuyoung Kim]

Created attachment 239340 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 239340 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/efl/WebPageEfl.cpp:237
> +        scalePage(restorePageScaleFactor, restoreScrollPoint);

FYI, WebPageIOS::restorePageState() also calls scalePage().

http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm#L223
http://trac.webkit.org/browser/trunk/Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm#L251

However, this is one of layer violation. Thus, as mentioned in http://trac.webkit.org/browser/trunk/Source/WebCore/loader/HistoryController.cpp#L144, restoreViewState() needs to be moved from WebKit2 to WebCore. Then, IMHO, we can fix this issue more efficient.

[WebKit Commit Bot]

Comment on attachment 239340 [details]
Patch

Clearing flags on attachment: 239340

Committed r174380: <http://trac.webkit.org/changeset/174380>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.