Implement touch event emulation in the WebCore layer

• Component: UI Events
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

Web developers for the mobile want to be able to debug their applications using their desktop hardware. This necessitates the emulation of certain aspects of mobile devices not found in desktop computers. One example is the emulation of "touch" events using the mouse pointer.

The proposed implementation is based on http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html.
Specifically, http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html#mouse-events states that if preventDefault() has been called on a touch event, the associated mouse event(s) should not be dispatched.

[Alexander Pavlov (apavlov)]

Created attachment 124128 [details]
Patch

[Alexey Proskuryakov]

CC'ing some folks who will likely have comments.

[Adam Barth]

Comment on attachment 124128 [details]
Patch

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

> Source/WebCore/dom/Document.h:685
> +#if ENABLE(TOUCH_EVENTS)
> +    void setTouchEventEmulationEnabled(bool enabled) { m_touchEventEmulationEnabled = enabled; }
> +    bool isTouchEventEmulationEnabled() const { return m_touchEventEmulationEnabled; }
> +#endif

Should these be on WebCore::Settings rather than on Document itself?

[Adam Barth]

Comment on attachment 124128 [details]
Patch

Can we test this change?  Perhaps using WebCore::Internals?

[Ryosuke Niwa]

Comment on attachment 124128 [details]
Patch

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

>> Source/WebCore/dom/Document.h:685
>> +#endif
> 
> Should these be on WebCore::Settings rather than on Document itself?

Yes.

[Alexander Pavlov (apavlov)]

Created attachment 124283 [details]
Patch

[Alexander Pavlov (apavlov)]

Thanks for your comments, everyone.

I've moved the property from Document into Settings and come up with a test using InternalSettings.

[Ryosuke Niwa]

Comment on attachment 124283 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:146
> +#if ENABLE(TOUCH_EVENTS) && ENABLE(INSPECTOR)

This condition doesn't seem right. Why is enabling inspector required for synthetic touch events?

[Alexander Pavlov (apavlov)]

(In reply to comment #8)
> (From update of attachment 124283 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124283&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:146
> > +#if ENABLE(TOUCH_EVENTS) && ENABLE(INSPECTOR)
> 
> This condition doesn't seem right. Why is enabling inspector required for synthetic touch events?

Originally, this was not required, but then I noticed that Inspector was going to be the only client of this feature (at least for the time being) and added the second clause. If you think this will make perfect sense for non-inspector-enabled builds, I'll be happy to revert this to the original ENABLE(TOUCH_EVENTS) condition.

[Ryosuke Niwa]

(In reply to comment #9)
> Originally, this was not required, but then I noticed that Inspector was going to be the only client of this feature (at least for the time being) and added the second clause. If you think this will make perfect sense for non-inspector-enabled builds, I'll be happy to revert this to the original ENABLE(TOUCH_EVENTS) condition.

As long as symbols are not exported, linkers should strip this code away so I'd rather have it for all builds with TOUCH enabled and hard-coding INSPECTOR dependency. Alternatively, you can add SYNTHETIC_TOUCH_EVENTS flag but that seems like an overkill.

[Alexander Pavlov (apavlov)]

(In reply to comment #10)
> (In reply to comment #9)
> > Originally, this was not required, but then I noticed that Inspector was going to be the only client of this feature (at least for the time being) and added the second clause. If you think this will make perfect sense for non-inspector-enabled builds, I'll be happy to revert this to the original ENABLE(TOUCH_EVENTS) condition.
> 
> As long as symbols are not exported, linkers should strip this code away so I'd rather have it for all builds with TOUCH enabled and hard-coding INSPECTOR dependency. Alternatively, you can add SYNTHETIC_TOUCH_EVENTS flag but that seems like an overkill.

Fine with reverting to just ENABLE(TOUCH_EVENTS). And what do you mean by "hard-coding INSPECTOR dependency"?

[Alexander Pavlov (apavlov)]

Created attachment 124687 [details]
[PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   9504f8b..3b4a9fa  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 106347 = 9504f8bc63da3ef4bc09c7a9fb94caaf217cb96a
r106348 = 3b4a9fa133678770a77bc6d602361e3cd4cb2dd1
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


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

[Alexander Pavlov (apavlov)]

Created attachment 124689 [details]
[PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation) - rebased

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


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

[Alexander Pavlov (apavlov)]

@reviewers: reverted to using only ENABLE(TOUCH_EVENTS). Any other concerns about the patch?

[Ryosuke Niwa]

Comment on attachment 124689 [details]
[PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation) - rebased

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

r- due to various nits but this patch looks much better than previous ones.

> Source/WebCore/ChangeLog:7
> +

Please provide an overview of your change.

> Source/WebCore/page/EventHandler.cpp:159
> +        m_radiusY = 1; // Default value.
> +        m_radiusX = 1; // Default value.
> +        m_rotationAngle = 0.0f; // Default value.
> +        m_force = 1.0f; // Default value.

Instead of commenting that these are default values, please add const int with appropriate variable names e.g. radiusXDefaultValue.

> Source/WebCore/page/EventHandler.cpp:175
> +        case PlatformEvent::MouseScroll:
> +        default:

Why do we need to enumerate PlatformEvent::MouseScroll if we have default?

> Source/WebCore/page/EventHandler.cpp:199
> +        switch (event.type()) {
> +        case PlatformEvent::MouseMoved:
> +            m_type = TouchMove;
> +            break;
> +        case PlatformEvent::MousePressed:
> +            m_type = TouchStart;
> +            break;
> +        case PlatformEvent::MouseReleased:
> +            m_type = TouchEnd;
> +            break;
> +        case PlatformEvent::MouseScroll:
> +        default:
> +            ASSERT_NOT_REACHED();
> +            m_type = NoType;
> +        }

This switch statement duplicates the code. Please share code with SyntheticTouchPoint::SyntheticTouchPoint.

> Source/WebCore/page/EventHandler.cpp:3443
> +bool EventHandler::maybeDispatchSyntheticTouchEvent(const PlatformMouseEvent& event)

"maybe" isn't descriptive adverb here. How about dispatchSyntheticTouchEventIfEnabled?

> LayoutTests/fast/events/touch/emulate-touch-events.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Please use HTML5 style DOCTYPE: <!DOCTYPE html>

> LayoutTests/fast/events/touch/emulate-touch-events.html:4
> +<head>
> +<script src="../../js/resources/js-test-pre.js"></script>

Why do we need head element at all?

> LayoutTests/fast/events/touch/emulate-touch-events.html:9
> +<!--
> +  Touch tests that involve the ontouchstart, ontouchmove, ontouchend or ontouchcancel callbacks
> +  should be written in an asynchronous fashion so they can be run on mobile platforms like Android.
> +  You will need to invoke isSuccessfullyParsed() in your test script when the test completes.
> +-->

Once you move the script here, you wouldn't need this description. If you think this comment is still useful, then please put in the description.

> LayoutTests/fast/events/touch/emulate-touch-events.html:14
> +<script src="script-tests/emulate-touch-events.js"></script>

New style is to include the script in the html file itself.

> LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:21
> +        // If we've got here, we can safely say we were successfully parsed :) We need to
> +        // call the isSucccessfullyParsed function to output the correct TEST COMPLETE
> +        // footer message.

This comment doesn't provide us of any new information. Please remove.

> LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:22
> +        isSuccessfullyParsed();

You should be declaring successfullyParsed variable at the end of the time instead.

> LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:53
> +    switch (which) {
> +        case 0:

Don't indent cases.

> LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:99
> +    debug("This test requires DumpRenderTree. Tap on the blue rect to log.");

Is there anyway to manually test this feature?

[Alexander Pavlov (apavlov)]

Attaching a fixed patch shortly.

(In reply to comment #17)
> (From update of attachment 124689 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124689&action=review
> 
> r- due to various nits but this patch looks much better than previous ones.
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> Please provide an overview of your change.

Done.

> > Source/WebCore/page/EventHandler.cpp:159
> > +        m_radiusY = 1; // Default value.
> > +        m_radiusX = 1; // Default value.
> > +        m_rotationAngle = 0.0f; // Default value.
> > +        m_force = 1.0f; // Default value.
> 
> Instead of commenting that these are default values, please add const int with appropriate variable names e.g. radiusXDefaultValue.

Done.

> > Source/WebCore/page/EventHandler.cpp:175
> > +        case PlatformEvent::MouseScroll:
> > +        default:
> 
> Why do we need to enumerate PlatformEvent::MouseScroll if we have default?

I felt like enumerating all mouse events in case we might want to make use of them for some kind of additional emulation (that is, we have some "spare" mouse event to use.)

> > Source/WebCore/page/EventHandler.cpp:199
> > +        switch (event.type()) {
> > +        case PlatformEvent::MouseMoved:
> > +            m_type = TouchMove;
> > +            break;
> > +        case PlatformEvent::MousePressed:
> > +            m_type = TouchStart;
> > +            break;
> > +        case PlatformEvent::MouseReleased:
> > +            m_type = TouchEnd;
> > +            break;
> > +        case PlatformEvent::MouseScroll:
> > +        default:
> > +            ASSERT_NOT_REACHED();
> > +            m_type = NoType;
> > +        }
> 
> This switch statement duplicates the code. Please share code with SyntheticTouchPoint::SyntheticTouchPoint.

Not really. This switch chooses an unsigned value describing one of the three event types, the other one (in SyntheticTouchPoint ctor) chooses PlatformTouchPoint::State, which is quite different (describes a touch point state,) and in the general case the State-related mapping won't be as simple (e.g. it may have stationary touchpoints.) Consider this a V1 of the emulation protocol.

> > Source/WebCore/page/EventHandler.cpp:3443
> > +bool EventHandler::maybeDispatchSyntheticTouchEvent(const PlatformMouseEvent& event)
> 
> "maybe" isn't descriptive adverb here. How about dispatchSyntheticTouchEventIfEnabled?

Good point, done.

> > LayoutTests/fast/events/touch/emulate-touch-events.html:1
> > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> 
> Please use HTML5 style DOCTYPE: <!DOCTYPE html>

I based my test off of basic-single-touch-events.html, guess it's a bit out-of-date. Fixed.

> > LayoutTests/fast/events/touch/emulate-touch-events.html:4
> > +<head>
> > +<script src="../../js/resources/js-test-pre.js"></script>
> 
> Why do we need head element at all?

Copied from basic-single-touch-events.html. Fixed.

> > LayoutTests/fast/events/touch/emulate-touch-events.html:9
> > +<!--
> > +  Touch tests that involve the ontouchstart, ontouchmove, ontouchend or ontouchcancel callbacks
> > +  should be written in an asynchronous fashion so they can be run on mobile platforms like Android.
> > +  You will need to invoke isSuccessfullyParsed() in your test script when the test completes.
> > +-->
> 
> Once you move the script here, you wouldn't need this description. If you think this comment is still useful, then please put in the description.

Removed.

> > LayoutTests/fast/events/touch/emulate-touch-events.html:14
> > +<script src="script-tests/emulate-touch-events.js"></script>
> 
> New style is to include the script in the html file itself.

Done.

> > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:21
> > +        // If we've got here, we can safely say we were successfully parsed :) We need to
> > +        // call the isSucccessfullyParsed function to output the correct TEST COMPLETE
> > +        // footer message.
> 
> This comment doesn't provide us of any new information. Please remove.

Done.

> > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:22
> > +        isSuccessfullyParsed();
> 
> You should be declaring successfullyParsed variable at the end of the time instead.

Removed this call, added debug output denoting the test completion.

> > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:53
> > +    switch (which) {
> > +        case 0:
> 
> Don't indent cases.

Fixed.

> > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:99
> > +    debug("This test requires DumpRenderTree. Tap on the blue rect to log.");
> 
> Is there anyway to manually test this feature?

This feature will be exposed to the users in the Web Inspector UI (a checkbox in the Settings) shortly after this core change has been landed (patch making use of this change in bug 77096). E.g. you will be able to go to http://paulirish.com/demo/multi, enable the emulation mode and draw in the canvas with your mouse just as you would draw there on your touch-enabled device (with a single touch point.) There are no other clients (barring this test) that toggle the emulation mode at the moment.

[Alexander Pavlov (apavlov)]

Created attachment 124919 [details]
[PATCH] rniwa's comments addressed

[Ryosuke Niwa]

(In reply to comment #18)
> > > Source/WebCore/page/EventHandler.cpp:175
> > > +        case PlatformEvent::MouseScroll:
> > > +        default:
> > 
> > Why do we need to enumerate PlatformEvent::MouseScroll if we have default?
> 
> I felt like enumerating all mouse events in case we might want to make use of them for some kind of additional emulation (that is, we have some "spare" mouse event to use.)

Please don't add unnecessary code on speculation.

> > > Source/WebCore/page/EventHandler.cpp:199
...
> > This switch statement duplicates the code. Please share code with SyntheticTouchPoint::SyntheticTouchPoint.
> 
> Not really. This switch chooses an unsigned value describing one of the three event types, the other one (in SyntheticTouchPoint ctor) chooses PlatformTouchPoint::State, which is quite different (describes a touch point state,) and in the general case the State-related mapping won't be as simple (e.g. it may have stationary touchpoints.) Consider this a V1 of the emulation protocol.

Okay.

> > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:22
> > > +        isSuccessfullyParsed();
> > 
> > You should be declaring successfullyParsed variable at the end of the time instead.
> 
> Removed this call, added debug output denoting the test completion.

All you need to do is:
var successfullyParsed = true;
at the end of script.

> > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:99
> > > +    debug("This test requires DumpRenderTree. Tap on the blue rect to log.");
> > 
> > Is there anyway to manually test this feature?
> 
> This feature will be exposed to the users in the Web Inspector UI (a checkbox in the Settings) shortly after this core change has been landed (patch making use of this change in bug 77096). E.g. you will be able to go to http://paulirish.com/demo/multi, enable the emulation mode and draw in the canvas with your mouse just as you would draw there on your touch-enabled device (with a single touch point.) There are no other clients (barring this test) that toggle the emulation mode at the moment.

Okay.

[Ryosuke Niwa]

Comment on attachment 124919 [details]
[PATCH] rniwa's comments addressed

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

> Source/WebCore/page/EventHandler.cpp:181
> +            ASSERT_NOT_REACHED();

I think you need a break after ASSERT_NOT_REACHED on some ports.

> Source/WebCore/page/EventHandler.cpp:202
> +            m_type = NoType;

Ditto about break.

> Source/WebCore/page/EventHandler.cpp:3446
> +// Returns whether the original mouse event should be handled.

We don't normally add a comment like this; instead we follow the same convention.

> Source/WebCore/page/EventHandler.cpp:3447
> +bool EventHandler::dispatchSyntheticTouchEventIfEnabled(const PlatformMouseEvent& event)

We normally return defaultPrevented.

> LayoutTests/fast/events/touch/emulate-touch-events.html:9
> +    var div = document.createElement("div");
> +    div.id = "touchtarget";

It seems unnecessary to indent the entire script content like this.

> LayoutTests/fast/events/touch/emulate-touch-events.html:12
> +    div.style.width = "100px";
> +    div.style.height = "100px";
> +    div.style.backgroundColor = "blue";

Why can't we just put this as a markup and obtain the div?
i.e. <div id="touchtarget" style="width: 100px; height: 100px; background-color: blue;"></div>
var touchTarget = document.getElementById('touchtarget');

> LayoutTests/fast/events/touch/emulate-touch-events.html:26
> +            debug('<br /><span class="pass">TEST COMPLETE</span>');

I don't think you need this debug given that the time is going to timeout when notifyDone is never called.

> LayoutTests/fast/events/touch/emulate-touch-events.html:105
> +</script>

Do:
var successfullyParsed = true;
here.

[Alexander Pavlov (apavlov)]

(In reply to comment #20)
> (In reply to comment #18)
> > > > Source/WebCore/page/EventHandler.cpp:175
> > > > +        case PlatformEvent::MouseScroll:
> > > > +        default:
> > > 
> > > > LayoutTests/fast/events/touch/script-tests/emulate-touch-events.js:22
> > > > +        isSuccessfullyParsed();
> > > 
> > > You should be declaring successfullyParsed variable at the end of the time instead.
> > 
> > Removed this call, added debug output denoting the test completion.
> 
> All you need to do is:
> var successfullyParsed = true;
> at the end of script.

My guess was that successfullyParsed denoted the correctness of all the JavaScript executed so far, right? (Because all the cases using successfullyParsed I have seen are synchronous, so parsing errors and execution errors appear at the same time. OTOH, placing "var a; a.foo();" in a touch event handler and placing "var successfullyParsed = true;" at the end of script would yield no errors, but the test would never work correctly.) Since our test is asynchronous, this makes no sense until the point when we notify testController about the test termination. Do you think "var successfullyParsed = true;" at the end of script is the right solution?

[Ryosuke Niwa]

(In reply to comment #22)
> My guess was that successfullyParsed denoted the correctness of all the JavaScript executed so far, right?

No, successfullyParsed is there to say that "the test script parsed without syntax errors".

> Since our test is asynchronous, this makes no sense until the point when we notify testController about the test termination.

That's not what successfullyParsed is used for.

[Alexander Pavlov (apavlov)]

(In reply to comment #23)
> (In reply to comment #22)
> > My guess was that successfullyParsed denoted the correctness of all the JavaScript executed so far, right?
> 
> No, successfullyParsed is there to say that "the test script parsed without syntax errors".

I got confused because in the suggested setup it was not used in any way.

> > Since our test is asynchronous, this makes no sense until the point when we notify testController about the test termination.
> 
> That's not what successfullyParsed is used for.

successfullyParsed is used only in isSuccessfullyParsed() (called from finishJSTest()). That said, I guess I've figured it out. I need to call finishJSTest() and include js-test-post.js at the end of the file.

[Alexander Pavlov (apavlov)]

(In reply to comment #21)
> (From update of attachment 124919 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124919&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:181
> > +            ASSERT_NOT_REACHED();
> 
> I think you need a break after ASSERT_NOT_REACHED on some ports.

Fixed.

> > Source/WebCore/page/EventHandler.cpp:202
> > +            m_type = NoType;
> 
> Ditto about break.

Fixed.

> > Source/WebCore/page/EventHandler.cpp:3446
> > +// Returns whether the original mouse event should be handled.
> 
> We don't normally add a comment like this; instead we follow the same convention.

Removed.

> > Source/WebCore/page/EventHandler.cpp:3447
> > +bool EventHandler::dispatchSyntheticTouchEventIfEnabled(const PlatformMouseEvent& event)
> 
> We normally return defaultPrevented.

The point was that this does not correspond to defaultPrevented in the case when no touch event dispatching takes place, but looks like this can be included into the abstraction of "conditional event dispatching" :)

> > LayoutTests/fast/events/touch/emulate-touch-events.html:9
> > +    var div = document.createElement("div");
> > +    div.id = "touchtarget";
> 
> It seems unnecessary to indent the entire script content like this.

Fixed.

> > LayoutTests/fast/events/touch/emulate-touch-events.html:12
> > +    div.style.width = "100px";
> > +    div.style.height = "100px";
> > +    div.style.backgroundColor = "blue";
> 
> Why can't we just put this as a markup and obtain the div?
> i.e. <div id="touchtarget" style="width: 100px; height: 100px; background-color: blue;"></div>
> var touchTarget = document.getElementById('touchtarget');

Done. I'm not sure, but when fixing this, I had some issues with the mouse event handler invocations (things looked like the mouse events were not dispatched.) I solved this by random code motion.

> > LayoutTests/fast/events/touch/emulate-touch-events.html:26
> > +            debug('<br /><span class="pass">TEST COMPLETE</span>');
> 
> I don't think you need this debug given that the time is going to timeout when notifyDone is never called.

Right, I ended up invoking finishJSTest() in this line.

> > LayoutTests/fast/events/touch/emulate-touch-events.html:105
> > +</script>
> 
> Do:
> var successfullyParsed = true;
> here.

This is not necessary with finishJSTest() in place (it has some special handling for asynchronous tests.)

[Alexander Pavlov (apavlov)]

Created attachment 124928 [details]
[PATCH] more comments addressed

[Ryosuke Niwa]

Comment on attachment 124928 [details]
[PATCH] more comments addressed

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

Seems okay. We can always improve it iteratively since it's not a Web-facing API. But please make sure adding new methods to internals.settings doesn't break any builds when you land it.

> Source/WebCore/testing/InternalSettings.cpp:274
> +void InternalSettings::setTouchEventEmulationEnabled(bool enabled, ExceptionCode& ec)

Don't you need to add symbols for this?

[Alexander Pavlov (apavlov)]

Committed r106642: <http://trac.webkit.org/changeset/106642>

[Zoltan Arvai]

It seems that the newly added layout test in r106642
LayoutTests/fast/events/touch/emulate-touch-events.html
not running as expected on Qt Linux Release bot:

--- /ramdisk/qt-linux-release/build/layout-test-results/fast/events/touch/emulate-touch-events-expected.txt 
+++ /ramdisk/qt-linux-release/build/layout-test-results/fast/events/touch/emulate-touch-events-actual.txt 
@@ -52,8 +52,8 @@
 PASS lastEvent.changedTouches[0].clientY is 30
 PASS lastEvent.changedTouches[0].identifier is 0
 PASS lastEvent.shiftKey is false
-PASS lastEvent.altKey is true
-PASS lastEvent.ctrlKey is true
+FAIL lastEvent.altKey should be true. Was false.
+FAIL lastEvent.ctrlKey should be true. Was false.
 PASS lastEvent.metaKey is false
 PASS successfullyParsed is true

http://build.webkit.org/results/Qt%20Linux%20Release/r106642%20(42848)/results.html

[Csaba Osztrogonác]

Zoltán file a new bug report about it, and skipped this test on Qt until fix - http://trac.webkit.org/changeset/106652

[Allan Sandfeld Jensen]

Maybe I missed it, but was there any particular reason to do this in WebCore?

For Qt the touch emulation is done on application level. It is really just a simply transformation of what events we send to WebKit.

[Kenneth Rohde Christiansen]

Why is this not done in the application layer, like we do for Qt? We support pinch emulation etc. Is this due to inspector integration or ?

[Pavel Feldman]

(In reply to comment #32)
> Why is this not done in the application layer, like we do for Qt? We support pinch emulation etc. Is this due to inspector integration or ?

"Emulate touch events" is a part of the inspector functionality, we want it to be available to all ports.

[Kenneth Rohde Christiansen]

(In reply to comment #33)
> (In reply to comment #32)
> > Why is this not done in the application layer, like we do for Qt? We support pinch emulation etc. Is this due to inspector integration or ?
> 
> "Emulate touch events" is a part of the inspector functionality, we want it to be available to all ports.

Ok, great :-) Did you think about adding support for emulating pinch zoom etc?