Bug 134150 – [iOS][WK2] Make the state restore from HistoryItem more precise and reliable

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Benjamin Poulain]

[iOS][WK2] Make the state restore from HistoryItem more precise and reliable

[Benjamin Poulain]

Created attachment 233506 [details]
Patch

[Benjamin Poulain]

<rdar://problem/16031704>

[Tim Horton]

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

[Anders Carlsson]

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?

[Benjamin Poulain]

(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?

[Benjamin Poulain]

Created attachment 233626 [details]
Patch

[Tim Horton]

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*

[Benjamin Poulain]

Committed r170325: <http://trac.webkit.org/changeset/170325>