[webkit-reviews] review denied: [Bug 133885] [iOS][wk2] Swiping back briefly shows the previous page before loading the new one : [Attachment 233087] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Tim Horton
<thorton at apple.com>'s request for review:
Bug 133885: [iOS][wk2] Swiping back briefly shows the previous page before
loading the new one
https://bugs.webkit.org/show_bug.cgi?id=133885

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233087&action=review


> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:209
> +    void setTransactionID(uint64_t transactionID) { m_transactionID =
transactionID; }

Should this be provided to the constructor?

> Source/WebKit2/UIProcess/DrawingAreaProxy.h:89
> +    virtual uint64_t nextTransactionID() const { ASSERT_NOT_REACHED();
return 0; }

Weird to have this one.

> Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:231
> +    m_targetTransactionID =
m_webPageProxy.drawingArea()->nextTransactionID();

"target" doesn't communicate what you're going to use this transaction ID for.

> Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:253
> +    if (m_targetRenderTreeSize && renderTreeSize > m_targetRenderTreeSize &&
m_webPageProxy.drawingArea()->currentTransactionID() >= m_targetTransactionID)

I want to see a call here like "isCurrentTransaction()" or "isStaleTransaction"
or something.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:80
> +    virtual uint64_t nextTransactionID() const override { return
m_currentTransactionID + 1; }

Remove?

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:94
> +    uint64_t m_currentTransactionID;

I feel like this should be lastVisibleTransactionID or something.

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:312
> +    m_currentTransactionID++;

Rather than just increment this count, I feel like it should copy the ID
obtained from the last transaction somehow.

> Source/WebKit2/WebProcess/WebPage/mac/RemoteLayerTreeDrawingArea.mm:282
> +    layerTransaction.setTransactionID(m_currentTransactionID++);

getNextTransactionID()?