Summary: | [iOS][WK2] Make the state restore from HistoryItem more precise and reliable | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||
Component: | New Bugs | Assignee: | Benjamin Poulain <benjamin> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, esprehn+autocc, glenn, japhet, kondapallykalyan, sam, simon.fraser, thorton | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Benjamin Poulain
2014-06-20 20:51:01 PDT
Created attachment 233506 [details]
Patch
Comment on attachment 233506 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233506&action=review Too many things to take in right now, but some minor comments: > Source/WebKit2/Shared/VisibleContentRectUpdateInfo.h:77 > + uint64_t lastLayerTreeTransactionId() const { return m_lastLayerTreeTransactionId; } it's ID, not Id, everywhere else > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2361 > + // Skip any VisibleContentRectUpdate that have been queued before DidCommitLoad suppress the updates on the UIProcess. there's at least one word missing in this sentence > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2362 > + if (visibleContentRectUpdateInfo.lastLayerTreeTransactionId() <= m_lastLayerTreeTransactionBeforeDidCommitLoad) shouldn't m_lastLayerTreeTransactionBeforeDidCommitLoad have "ID" in it somewhere? > Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.h:56 > + uint64_t currentTransactionID() { return m_currentTransactionID; } const Comment on attachment 233506 [details]
Patch
Let's not add more things to HistoryItem; I'm working on a better way to do state restoration, and I'm planning to have something landed early next week. Maybe we can revisit this patch then?
(In reply to comment #4) > (From update of attachment 233506 [details]) > Let's not add more things to HistoryItem; I'm working on a better way to do state restoration, and I'm planning to have something landed early next week. Maybe we can revisit this patch then? Are you kidding? I have been debugging this crap for a week. Why didn't you mention it last monday? Created attachment 233626 [details]
Patch
Comment on attachment 233626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233626&action=review > Source/WebCore/history/CachedPage.cpp:93 > + bool hadProhibitsScrolling = false; maybe explain this in the changelog? > Source/WebCore/loader/HistoryController.cpp:82 > + FrameView& frameView = *m_frame.view(); might as well move this up a little further and use it in more places > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:176 > + uint64_t _firstTransactionIdAfterPageRestore; ID, not Id. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:785 > + if (_needsToRestoreExposedRect && layerTreeTransaction.transactionID() >= _firstTransactionIdAfterPageRestore) { can we factor out the layerTreeTransaction.transactionID() >= _firstTransactionIdAfterPageRestore from these two blocks > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:832 > +- (void)_restorePageSateToExposedRect:(WebCore::FloatRect)exposedRect scale:(double)scale typo "Sate" > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:847 > +- (void)_restorePageSateToUnobscuredCenter:(WebCore::FloatPoint)center scale:(double)scale typo "Sate" > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:857 > + _firstTransactionIdAfterPageRestore = toRemoteLayerTreeDrawingAreaProxy(_page->drawingArea())->nextLayerTreeTransactionID(); ID > Source/WebKit2/UIProcess/WebPageProxy.h:1201 > + void restorePageSate(const WebCore::FloatRect&, double scale); so many Sates > Source/WebKit2/WebProcess/WebCoreSupport/ios/WebFrameLoaderClientIOS.mm:94 > + if (m_frame->isMainFrame()) > + m_frame->page()->restorePageState(*currentItem); might as well use frame instead of m_frame > Source/WebKit2/WebProcess/WebPage/WebPage.h:1220 > + uint64_t m_lastLayerTreeTransactionIdBeforeDidCommitLoad; ID > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:171 > + // When the content size change, we keep the same relative horizontal content width in view, otherwise we would change*s* > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:172 > + // end up zoom to far in landscape->portrait, and too close in portrait->landscape. zoom*ed*, to*o* Committed r170325: <http://trac.webkit.org/changeset/170325> |