Bug 134506

Summary: [iOS][WK2] Sometimes the swipe snapshot stays up too long
Product: WebKit Reporter: Tim Horton <thorton>
Component: WebKit2Assignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, bunhere, cdumez, commit-queue, gyuyoung.kim, sam, sergio, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
weinig's change
none
actually different patch
simon.fraser: review-
patch simon.fraser: review+

Description Tim Horton 2014-07-01 12:33:33 PDT
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>
Comment 1 Tim Horton 2014-07-01 13:01:57 PDT
Created attachment 234192 [details]
patch
Comment 2 Sam Weinig 2014-07-01 14:31:35 PDT
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.
Comment 3 Tim Horton 2014-07-01 14:46:50 PDT
(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?
Comment 4 Tim Horton 2014-07-01 15:00:28 PDT
Created attachment 234201 [details]
weinig's change
Comment 5 Tim Horton 2014-07-01 15:01:08 PDT
Created attachment 234202 [details]
actually different patch
Comment 6 WebKit Commit Bot 2014-07-01 15:03:41 PDT
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.
Comment 7 Simon Fraser (smfr) 2014-07-02 11:33:59 PDT
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.
Comment 8 Tim Horton 2014-07-02 16:39:07 PDT
Created attachment 234293 [details]
patch

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

I did add TransactionCallbackID typedef, and renamed some things.
Comment 9 WebKit Commit Bot 2014-07-02 16:41:43 PDT
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.
Comment 10 Simon Fraser (smfr) 2014-07-03 12:08:10 PDT
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)) ?
Comment 11 Tim Horton 2014-07-03 12:31:04 PDT
http://trac.webkit.org/changeset/170761