Bug 134506 – [iOS][WK2] Sometimes the swipe snapshot stays up too long

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

The approach in r170035 is still not quite correct; there is a window where endSwipeGesture will grab the wrong ID, and I'm not sure it's trivially reconcilable.

Instead, let's implement transaction callbacks (one of my alternate solutions to that problem); the UI process will register a callback in endSwipeGesture, just after goToBackForwardItem; it will send the callback ID to the Web process, which will stuff it in the next transaction (and schedule one if needed). Then, upon receipt of the transaction, the UI process will perform the callbacks included in that transaction.

This ensures that a) we always get a commit some time soon after endSwipeGesture that contains our callback b) it's always in-order with the goToBackForwardItem: message.

This seems to work great for swiping between very stable pages (which don't commit often), very unstable pages (which commit very frequently), and a mixture of the two, and should be generally extendable to solve other similar synchronization problems as they come up.

<rdar://problem/17496803>

[Tim Horton]

Created attachment 234192 [details]
patch

[Sam Weinig]

Comment on attachment 234192 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234192&action=review

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:62
> +    virtual void installTransactionCallback(PassRefPtr<VoidCallback>) override;

This should take a std::function.

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 234192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234192&action=review
> 
> > Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:62
> > +    virtual void installTransactionCallback(PassRefPtr<VoidCallback>) override;
> 
> This should take a std::function.

And construct a VoidCallback? Or not use that mechanism at all?

[Tim Horton]

Created attachment 234201 [details]
weinig's change

[Tim Horton]

Created attachment 234202 [details]
actually different patch

[WebKit Commit Bot]

Attachment 234202 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:343:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:343:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/DrawingAreaProxy.h:91:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 234202 [details]
actually different patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234202&action=review

Too many confusing uint64_ts.

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:226
> +    Vector<uint64_t> m_callbackIDs;

Typedef for the uint64_t? Is this a pageID or a transactionID or something else? If a pageID, this member should say m_pageCallbacks or something.

> Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:70
> +    static NeverDestroyed<HashMap<uint64_t, WebKit::ViewGestureController*>> viewGestureControllers;

Would be nice if we had a pageID typedef.

[Tim Horton]

Created attachment 234293 [details]
patch

I'm not going to typedef PageID in this patch!

I did add TransactionCallbackID typedef, and renamed some things.

[WebKit Commit Bot]

Attachment 234293 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:343:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.h:62:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/DrawingAreaProxy.h:91:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 234293 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234293&action=review

> Source/WebKit2/Shared/mac/RemoteLayerTreeTransaction.h:213
> +    typedef uint64_t TransactionCallbackID;

Very nice.

> Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:68
> +static HashMap<uint64_t, WebKit::ViewGestureController*>& allViewGestureControllers()

Please add a comment saying that the uint64_t is a pageID or rename this to viewGestureControllersForAllPages()

> Source/WebKit2/UIProcess/ios/ViewGestureControllerIOS.mm:263
> +        if (gestureControllerIter == allViewGestureControllers().end())
> +            return;
> +        gestureControllerIter->value->willCommitPostSwipeTransitionLayerTree(error == CallbackBase::Error::None);

Would slightly prefer this as 
if (gestureControllerIter != allViewGestureControllers().end())
  gestureControllerIter->value->willCommitPostSwipeTransitionLayerTree(error == CallbackBase::Error::None);

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:137
> +        if (callback)

if (auto callback = m_callbacks.take<VoidCallback>(callbackID)) ?

[Tim Horton]

http://trac.webkit.org/changeset/170761