[Webkit-unassigned] [Bug 66800] Modify WebTouchEvent structure to match WebCore::TouchEvent

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66800





--- Comment #6 from Steve Block <steveblock at google.com>  2011-08-24 02:44:22 PST ---
(From update of attachment 104914)
View in context: https://bugs.webkit.org/attachment.cgi?id=104914&action=review

Mostly minor points.

Once these are fixed up, it would be good to get a Chromium person to take a look before I r+.

> Source/WebKit/chromium/ChangeLog:3
> +        Modify WebTouchEvent structure to match WebCore::TouchEvent

You should modify the bug title to include 'Chromium', as this touches only Chromium's WebKit layer. I think the pattern is to prefix the title with '[Chromium]'.

> Source/WebKit/chromium/public/WebInputEvent.h:347
> +    static const int touchPointsLengthCap = 8;

Should this be 'touchesLengthCap' to match the other variable names?

> Source/WebKit/chromium/public/WebTouchPoint.h:-45
> -    };

Can you explain the reasons for and the consequences of removing this enum?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:46
> +#endif // ENABLE(TOUCH_EVENTS)

It looks like the contents of these files are already guarded with ENABLE(TOUCH_EVENTS), so there's no need for a guard here. WebKit policy is generally to use as few guards as possible.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:270
> +        m_touchPoints.append(PlatformTouchPointBuilder(widget, event.touches[i]));

Should you rename m_touchPoints to m_touches for consistency?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:394
> +namespace {

It looks like other helpers in this file are static, rather than in an anonymous namespace. I think an anonymous namespace is now the preferred style, but I'm not sure whether you should fix all other occurrences here, stick to 'static' for consistency, or leave it as it is.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:397
> +    for (unsigned int i = 0; i < touches->length(); i++) {

What happens if touches->length() exceeds touchPointsLengthCap? Is touches->length() limited by WebCore? If so, is there a compile or run-time check to make sure that touchPointsLengthCap is at least this value?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:398
> +        WebTouchPoint pt;

Use complete words for variable names.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:410
> +        (*touchPointsLength)++;

Is there any reason to increment this, rather than just setting it to touches->length()? It seems strange to rely on the fact that it's zero-initialized. If it's not zero-initialized, the value will be wrong, as we always set touchPoints from index 0 onwards.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:426
> +        type = Undefined;

Does WebCore use touch event types other than start/move/end/cancel? If so, mention in the header comment which ones we honour and the fallback case. If not, perhaps there should be an assert here?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:431
> +    timeStampSeconds = event.timeStamp() * 1.0e-3;

Eeek! Surely there's an existing kMillisecondsToSeconds you could use? Or at least use a static constant in this file.

> Source/WebKit/chromium/src/WebInputEventConversion.h:48
> +#endif // ENABLE(TOUCH_EVENTS)

Again, avoid guards unless required to build.

> Source/WebKit/chromium/src/WebInputEventConversion.h:127
> +// Converts a WebCore::TouchEvent to a corresponding WebTouchEvent.

Might be nice to add a corresponding comment to PlatformTouchPointBuilder/PlatformTouchEventBuilder to make clear that these convert in the other direction.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.