Bug 181681 – Unify use of uiController.dragFromPointToPoint

• Component: Tools / Tests
• Status: NEW

[Frédéric Wang (:fredw)]

.

[Frédéric Wang (:fredw)]

Created attachment 331385 [details]
Patch

[EWS Watchlist]

Comment on attachment 331385 [details]
Patch

Attachment 331385 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6093535

New failing tests:
imported/w3c/web-platform-tests/media-source/mediasource-config-change-mp4-a-bitrate.html

[EWS Watchlist]

Created attachment 331388 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Megan Gardner]

Comment on attachment 331385 [details]
Patch

I think there could be some more interesting integrations with basic-gestures.js as well.

[Frédéric Wang (:fredw)]

(In reply to Megan Gardner from comment #4)
> Comment on attachment 331385 [details]
> Patch
> 
> I think there could be some more interesting integrations with
> basic-gestures.js as well.

OK, I think most of these tests could use touchAndDragFromPointToPoint and tapAtPoint (some of them currently seems to assume that the script completes with "Done", so they might need some adjustment). However, it is a shame that:

- We duplicate some code in ui-helper.js and basic-gesture.js (e.g. UIHelper::tapAt and tapAtPoint).
- basic-gestures.js does not return promises running the scripts. This makes the usage a bit more verbose and requires more indentation than with "await".

[Wenson Hsieh]

(In reply to Frédéric Wang (:fredw) from comment #5)
> (In reply to Megan Gardner from comment #4)
> > Comment on attachment 331385 [details]
> > Patch
> > 
> > I think there could be some more interesting integrations with
> > basic-gestures.js as well.
> 
> OK, I think most of these tests could use touchAndDragFromPointToPoint and
> tapAtPoint (some of them currently seems to assume that the script completes
> with "Done", so they might need some adjustment). However, it is a shame
> that:
> 
> - We duplicate some code in ui-helper.js and basic-gesture.js (e.g.
> UIHelper::tapAt and tapAtPoint).
> - basic-gestures.js does not return promises running the scripts. This makes
> the usage a bit more verbose and requires more indentation than with "await".

This is true. What do you think about building UIHelper functionality off of basic-gestures.js? (i.e. use the helpers in basic-gestures.js to generate UI-side scripts that we call runUIScript with in ui-helper.js?)

[Frédéric Wang (:fredw)]

(In reply to Wenson Hsieh from comment #6)
> This is true. What do you think about building UIHelper functionality off of
> basic-gestures.js? (i.e. use the helpers in basic-gestures.js to generate
> UI-side scripts that we call runUIScript with in ui-helper.js?)

Yes, I was thinking about it too and I believe that should work. @Megan: What do you suggest?

[Megan Gardner]

Yes, looking at these a bit more, I think that is the thing to do. Use the strings generated by basic-gestures.js to run the encapsulated async functions in ui-helper. There is probably more streamlining and cleanup to be done here, but it would take a longer look.

[Frédéric Wang (:fredw)]

I was thinking maybe I should upload a preliminary patch to move the files in LayoutTests/fast/events/touch/ios/resources/ into LayoutTests/resources/ so that they are in the same location as ui-helper.js and other helper scripts?

[Wenson Hsieh]

(In reply to Frédéric Wang (:fredw) from comment #9)
> I was thinking maybe I should upload a preliminary patch to move the files
> in LayoutTests/fast/events/touch/ios/resources/ into LayoutTests/resources/
> so that they are in the same location as ui-helper.js and other helper
> scripts?

That sounds like a good idea to me!

[Frédéric Wang (:fredw)]

I opened bug 181798. However, I'm wondering if we should just merge basic-gestures.js into ui-helper.js (ui-debugging.js does not seem to be used). There are not too many tests using this file, so I guess I can convert them to the promise/await syntax.

[Ryosuke Niwa]

Note that the intent of ui-helper.js to abstract away the difference between WK1 and WK2 so whatever we do here should work in both WK1 and WK2, and shouldn't create two different solutions for them.

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #12)
> Note that the intent of ui-helper.js to abstract away the difference between
> WK1 and WK2 so whatever we do here should work in both WK1 and WK2, and
> shouldn't create two different solutions for them.

Also the difference between macOS and iOS. i.e. tap vs click, etc...

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #13)
> (In reply to Ryosuke Niwa from comment #12)
> > Note that the intent of ui-helper.js to abstract away the difference between
> > WK1 and WK2 so whatever we do here should work in both WK1 and WK2, and
> > shouldn't create two different solutions for them.
> 
> Also the difference between macOS and iOS. i.e. tap vs click, etc...

In the long term, I think we also want to abstract out cross-platform differences where possible, which is why we use some generic names like "activateAt", which simulates a mouseDown/mouseUp on macOS but sends a tap on iOS.

[Frédéric Wang (:fredw)]

(In reply to Ryosuke Niwa from comment #12)
> Note that the intent of ui-helper.js to abstract away the difference between
> WK1 and WK2 so whatever we do here should work in both WK1 and WK2, and
> shouldn't create two different solutions for them.

Per the changes made in attachment 331385 [details], uiController.dragFromPointToPoint is already used for tests on iOS and macOS. I am not sure whether it works on WK1 and how to emulate the behavior if it does not. I'm currently implemeting iOS frame scrolling in 173833 and I will only need iOS/WK2. So if it's too complicated I guess I can just use user-gestures.js for now. I uploaded bug 181841 to move it to the more concise syntax with promise/await. Or maybe I can do some asserts as other functions do to ensure they are only used on WK2 for now.

[Ryosuke Niwa]

(In reply to Frédéric Wang (:fredw) from comment #15)
> (In reply to Ryosuke Niwa from comment #12)
> > Note that the intent of ui-helper.js to abstract away the difference between
> > WK1 and WK2 so whatever we do here should work in both WK1 and WK2, and
> > shouldn't create two different solutions for them.
> 
> Per the changes made in attachment 331385 [details],
> uiController.dragFromPointToPoint is already used for tests on iOS and
> macOS. I am not sure whether it works on WK1 and how to emulate the behavior
> if it does not. I'm currently implemeting iOS frame scrolling in 173833 and
> I will only need iOS/WK2. So if it's too complicated I guess I can just use
> user-gestures.js for now. I uploaded bug 181841 to move it to the more
> concise syntax with promise/await. Or maybe I can do some asserts as other
> functions do to ensure they are only used on WK2 for now.

It's okay as long as we don't have to write a different test code for WK1/WK2/iOS/macOS.

[Ryosuke Niwa]

Comment on attachment 331385 [details]
Patch

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

> LayoutTests/resources/ui-helper.js:16
> +            testRunner.runUIScript(`

This assumes WebKitTestRunner. We should make this compatible with DumpRenderTree as well.

[Frédéric Wang (:fredw)]

I'm resetting the bug assignment as I don't plan to work on this in the short term. I started making basic-gestures.js more consistent with UIHelper (use of promises) in bug 181798 and bug 181841. I also modified one scrolling test I wrote in the past (bug 182041) to use functions from basic-gestures.js ; and the same for new tests I'm introducing in bug 173833. If someone is interested in unifying / generalize the use of uiController.dragFromPointToPoint via the UIHelper class, feel free to take that bug.