[webkit-reviews] review denied: [Bug 136999] [EFL] Implement restorePageState() for backward navigation : [Attachment 238713] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Gyuyoung Kim
<gyuyoung.kim at webkit.org>'s request for review:
Bug 136999: [EFL] Implement restorePageState() for backward navigation
https://bugs.webkit.org/show_bug.cgi?id=136999

Attachment 238713: Patch
https://bugs.webkit.org/attachment.cgi?id=238713&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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/ScrollingCoordinatorCoordinat
edGraphics.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?