[webkit-reviews] review granted: [Bug 183043] [WinCairo] Add WebKit Shared/win event files for wincairo webkit : [Attachment 334862] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Don Olmstead
<don.olmstead at sony.com>'s request for review:
Bug 183043: [WinCairo] Add WebKit Shared/win event files for wincairo webkit
https://bugs.webkit.org/show_bug.cgi?id=183043

Attachment 334862: Patch

https://bugs.webkit.org/attachment.cgi?id=334862&action=review




--- Comment #5 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 334862
  --> https://bugs.webkit.org/attachment.cgi?id=334862
Patch

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

Wow! Looks nice. Please consider my suggestions, but I think this is fine.

> Source/WebKit/Shared/win/NativeWebKeyboardEventWin.cpp:38
> +    , m_nativeEvent()

Does this auto-initialize the various MSG members to null values? Isn't this
uninitialized crap at this point? Or does WinAPI now have a default constructor
that does the right thing?

> Source/WebKit/Shared/win/NativeWebKeyboardEventWin.cpp:43
> +    m_nativeEvent.lParam = lParam;

What about 'time' and 'pt'? Aren't those uninitialized now? Maybe we never use
them, but it's still a potential problem if we have code that inspects that
data.

This might be cleaner if we had some kind of WebKit 'MSG' initializer function,
since you repeat this in a few places.

Then you could just do "m_nativeEvent(hwnd, message, wParam, lParam)" or
"m_nativeEvent(createNativeEvent(hwnd, ...)"

> Source/WebKit/Shared/win/NativeWebMouseEventWin.cpp:41
> +    m_nativeEvent.lParam = lParam;

Repeated code

> Source/WebKit/Shared/win/NativeWebWheelEventWin.cpp:41
> +    m_nativeEvent.lParam = lParam;

Ditto initializations.

> Source/WebKit/Shared/win/WebEventFactory.cpp:80
> +	   || ((timeStampSeconds - gLastClickTime) * 1000.0 >
::GetDoubleClickTime());

I'd suggest using 'std::abs" here, so the code will work without casting if we
ever build for 32-bit, or if Microsoft ever changes the definition of POINT.

> Source/WebKit/Shared/win/WebEventFactory.cpp:400
> +    static const float cScrollbarPixelsPerLine = 100.0f / 3.0f;

We should probably expose this from WebCore in some useful way, since we now
have this hard-coded in two places.

> Source/WebKit/Shared/win/WebEventFactory.cpp:406
> +    WebWheelEvent::Granularity granularity  =
WebWheelEvent::ScrollByPixelWheelEvent;

Looks like two spaces after granularity. I'm surprised the syntax checker
didn't complain!