Bug 194076 - Enable swipe tests on iOS
Summary: Enable swipe tests on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-30 21:01 PST by Ryosuke Niwa
Modified: 2019-01-31 17:09 PST (History)
7 users (show)

See Also:


Attachments
Enables the tests (4.83 KB, patch)
2019-01-30 21:10 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-01-30 21:01:32 PST
Tests in LayoutTests/swipe are currently skipped on iOS. Enable all six of them.
Comment 1 Ryosuke Niwa 2019-01-30 21:10:00 PST
Created attachment 360688 [details]
Enables the tests
Comment 2 Geoffrey Garen 2019-01-30 21:34:34 PST
Comment on attachment 360688 [details]
Enables the tests

r=me
Comment 3 Tim Horton 2019-01-30 21:48:02 PST
Comment on attachment 360688 [details]
Enables the tests

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

> LayoutTests/platform/ios/swipe/main-frame-pinning-requirement-expected.txt:2
> +Failure. Should never begin a swipe, because we were in the middle of a scrolling gesture that started when the main frame was not pinned to the left.

This test doesn't make sense on iOS because we don't require the main frame to be pinned to swipe... but maybe it's OK to turn it on with the expected failure with the understanding that it's truly an *expected* failure?
Comment 4 Ryosuke Niwa 2019-01-30 23:10:45 PST
(In reply to Tim Horton from comment #3)
> Comment on attachment 360688 [details]
> Enables the tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360688&action=review
> 
> > LayoutTests/platform/ios/swipe/main-frame-pinning-requirement-expected.txt:2
> > +Failure. Should never begin a swipe, because we were in the middle of a scrolling gesture that started when the main frame was not pinned to the left.
> 
> This test doesn't make sense on iOS because we don't require the main frame
> to be pinned to swipe... but maybe it's OK to turn it on with the expected
> failure with the understanding that it's truly an *expected* failure?

Yeah, I'm simply turning on the tests here. This is definitely not expected on iOS as you say. Having a failing test is better than no tests :)
Comment 5 Ryosuke Niwa 2019-01-30 23:12:41 PST
I'd mention that WebKitTestRunner doesn't seem to support back-forward swipe gesture even when we turn it on via testRunner.setNavigationGesturesEnabled(true). I can't manually trigger back-forward navigation via swiping for example.
Comment 6 Ryosuke Niwa 2019-01-30 23:31:23 PST
Comment on attachment 360688 [details]
Enables the tests

Clearing flags on attachment: 360688

Committed r240765: <https://trac.webkit.org/changeset/240765>
Comment 7 Ryosuke Niwa 2019-01-30 23:31:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-01-30 23:32:50 PST
<rdar://problem/47694943>
Comment 9 Simon Fraser (smfr) 2019-01-31 10:45:24 PST
Comment on attachment 360688 [details]
Enables the tests

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

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:-827
>      TestRunnerWKWebView *webView = TestController::singleton().mainWebView()->platformView();
>      [webView _beginBackSwipeForTesting];
> -
> -    unsigned callbackID = m_context->prepareForAsyncTask(callback, CallbackTypeNonPersistent);
> -    m_context->asyncTaskComplete(callbackID);

Is beginBackSwipe async or not? Who calls the callback?

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:827
>  void UIScriptController::completeBackSwipe(JSValueRef callback)

Odd that the API is not just "doBackswipe" with an async completion handler.
Comment 10 Ryosuke Niwa 2019-01-31 16:55:58 PST
(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 360688 [details]
> Enables the tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360688&action=review
> 
> > Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:-827
> >      TestRunnerWKWebView *webView = TestController::singleton().mainWebView()->platformView();
> >      [webView _beginBackSwipeForTesting];
> > -
> > -    unsigned callbackID = m_context->prepareForAsyncTask(callback, CallbackTypeNonPersistent);
> > -    m_context->asyncTaskComplete(callbackID);
> 
> Is beginBackSwipe async or not? Who calls the callback?

It's a sync test API.

> > Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:827
> >  void UIScriptController::completeBackSwipe(JSValueRef callback)
> 
> Odd that the API is not just "doBackswipe" with an async completion handler.

Yeah.
Comment 11 Tim Horton 2019-01-31 16:58:56 PST
(In reply to Ryosuke Niwa from comment #10)
> (In reply to Simon Fraser (smfr) from comment #9)
> > Comment on attachment 360688 [details]
> > Enables the tests
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=360688&action=review
> > 
> > > Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:-827
> > >      TestRunnerWKWebView *webView = TestController::singleton().mainWebView()->platformView();
> > >      [webView _beginBackSwipeForTesting];
> > > -
> > > -    unsigned callbackID = m_context->prepareForAsyncTask(callback, CallbackTypeNonPersistent);
> > > -    m_context->asyncTaskComplete(callbackID);
> > 
> > Is beginBackSwipe async or not? Who calls the callback?
> 
> It's a sync test API.
> 
> > > Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:827
> > >  void UIScriptController::completeBackSwipe(JSValueRef callback)
> > 
> > Odd that the API is not just "doBackswipe" with an async completion handler.
> 
> Yeah.

Don't some of the tests depend on being able to do things in the middle?
Comment 12 Ryosuke Niwa 2019-01-31 17:05:50 PST
(In reply to Tim Horton from comment #11)
> (In reply to Ryosuke Niwa from comment #10)
> > (In reply to Simon Fraser (smfr) from comment #9)
> > > Comment on attachment 360688 [details]
> > > Enables the tests
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=360688&action=review
> > > 
> > > > Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:-827
> > > >      TestRunnerWKWebView *webView = TestController::singleton().mainWebView()->platformView();
> > > >      [webView _beginBackSwipeForTesting];
> > > > -
> > > > -    unsigned callbackID = m_context->prepareForAsyncTask(callback, CallbackTypeNonPersistent);
> > > > -    m_context->asyncTaskComplete(callbackID);
> > > 
> > > Is beginBackSwipe async or not? Who calls the callback?
> > 
> > It's a sync test API.
> > 
> > > > Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:827
> > > >  void UIScriptController::completeBackSwipe(JSValueRef callback)
> > > 
> > > Odd that the API is not just "doBackswipe" with an async completion handler.
> > 
> > Yeah.
> 
> Don't some of the tests depend on being able to do things in the middle?

They do but you'd think we can have a higher level API which does the whole swipe too.
Comment 13 Tim Horton 2019-01-31 17:09:13 PST
> They do but you'd think we can have a higher level API which does the whole
> swipe too.

Ah, yes, sure.