Summary: | [EFL] Restore previous scroll position using restoreViewState() | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, darin, lucas.de.marchi | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 126022, 136917 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2014-09-22 04:24:56 PDT
Created attachment 238477 [details]
Patch
Created attachment 238478 [details]
Patch
CC'ing Darin and Benjamin Created attachment 238482 [details]
Patch
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. 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. (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 Created attachment 238713 [details]
Patch
(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. 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? 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. Created attachment 239095 [details]
Patch
(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 ! 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? By the way, do not hesitate to poke me on IRC. I am swamped in review emails so I frequently miss updates on bugzilla. 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 Created attachment 239340 [details]
Patch
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. Comment on attachment 239340 [details] Patch Clearing flags on attachment: 239340 Committed r174380: <http://trac.webkit.org/changeset/174380> All reviewed patches have been landed. Closing bug. |