[webkit-reviews] review denied: [Bug 77105] Implement touch event emulation in the WebCore layer : [Attachment 124689] [PATCH] Removed the ENABLE(INSPECTOR) clause (conditional compilation) - rebased

Tue Jan 31 15:30:12 PST 2012

Ryosuke Niwa <rniwa at webkit.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 77105: Implement touch event emulation in the WebCore layer
https://bugs.webkit.org/show_bug.cgi?id=77105

Attachment 124689: [PATCH] Removed the ENABLE(INSPECTOR) clause (conditional
compilation) - rebased
https://bugs.webkit.org/attachment.cgi?id=124689&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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?