[Webkit-unassigned] [Bug 183043] [WinCairo] Add WebKit Shared/win event files for wincairo webkit

[bugzilla-daemon at webkit.org]

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

Brent Fulgham <bfulgham at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #334862|review?                     |review+, commit-queue-
              Flags|                            |

--- 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!

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180302/7f14c486/attachment.html>