Bug 183043 – [WinCairo] Add WebKit Shared/win event files for wincairo webkit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Yousuke Kimoto]

Add event files in Shared/Win for wincairo webkit.
In this ticket, the following files are implemented:
- WebKit/Shared/win/NativeWebKeyboardEventWin.cpp
- WebKit/Shared/win/NativeWebMouseEventWin.cpp
- WebKit/Shared/win/NativeWebTouchEventWin.cpp
- WebKit/Shared/win/NativeWebWheelEventWin.cpp
- WebKit/Shared/win/WebEventFactory.cpp
- WebKit/Shared/win/WebEventFactory.h

Also the following files are modified to add the above event files:
- WebKit/Shared/NativeWebKeyboardEvent.h
- WebKit/Shared/NativeWebMouseEvent.h
- WebKit/Shared/NativeWebTouchEvent.h
- WebKit/Shared/NativeWebWheelEvent.h

[Yousuke Kimoto]

Created attachment 334511 [details]
Patch

[Don Olmstead]

Comment on attachment 334511 [details]
Patch

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

This should be fine its just WPE and iOS are not compiling due to the #else being removed.

> Source/WebKit/Shared/NativeWebKeyboardEvent.h:90
> -#else
> -    const void* nativeEvent() const { return nullptr; }
> +#elif PLATFORM(WIN)
> +    const MSG* nativeEvent() const { return &m_nativeEvent; }
>  #endif

WPE and ios are failing because the #else was removed

> Source/WebKit/Shared/NativeWebMouseEvent.h:71
>      const GdkEvent* nativeEvent() const { return m_nativeEvent.get(); }
> -#else
> -    const void* nativeEvent() const { return nullptr; }
> +#elif PLATFORM(WIN)
> +    const MSG* nativeEvent() const { return &m_nativeEvent; }
>  #endif

Same problem here

> Source/WebKit/Shared/NativeWebWheelEvent.h:71
> -#else
> -    const void* nativeEvent() const { return nullptr; }
> +#elif PLATFORM(WIN)
> +    const MSG* nativeEvent() const { return &m_nativeEvent; }
>  #endif

Same problem

[Don Olmstead]

Created attachment 334862 [details]
Patch

Lets see if ios and wpe like this one

[youenn fablet]

Comment on attachment 334862 [details]
Patch

I see nothing wrong in this patch but I am not familiar enough with this code to fully review the patch.
Maybe Per Arne could take a look?
Since it is related to touch events, Ryosuke or Wenson might be good people to talk to as well.

[Brent Fulgham]

Comment on attachment 334862 [details]
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!

[Don Olmstead]

Comment on attachment 334862 [details]
Patch

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

>> 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, ...)"

There's actually something like that hanging out in Tools/DumpRenderTree/win/EventSender.cpp.

We could add something into WebEventFactory that would create the MSG.

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

I started poking around a bit more and this code is present Source/WebCore/platform/win/WheelEventWin.cpp as well as Tools/DumpRenderTree/win/EventSender.cpp.

Is there any reason the WebEvent things aren't using PlatformEvents? I see GTK bolts some stuff onto PlatformKeyboardEvent specifically for WebKit(2) use. I'm just wondering if we should maybe open a bug to refactor the PlatformEvents into something usable within WebKit.

[Ryosuke Niwa]

Comment on attachment 334862 [details]
Patch

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

r=me as an API owner as well.

> Source/WebKit/Shared/NativeWebWheelEvent.h:81
> +    MSG m_nativeEvent;

You probably want to initialize MSG here.

> Source/WebKit/Shared/win/WebEventFactory.cpp:110
> +    if (::GetKeyState(VK_MENU) & 0x8000)

Instead of keep having & 0x8000 everywhere,
can we add an inline helper function like IsKeyInDownState(VM_MENU) / IsKeyStateDown(::GetKeyState(VK_MENU))?

> Source/WebKit/Shared/win/WebEventFactory.cpp:123
> +    if (::GetKeyState(VK_CONTROL) & 0x8000)
> +        modifiers |= WebEvent::ControlKey;
> +    if (::GetKeyState(VK_SHIFT) & 0x8000)
> +        modifiers |= WebEvent::ShiftKey;
> +    if (::GetKeyState(VK_MENU) & 0x8000)
> +        modifiers |= WebEvent::AltKey;

Ditto about all these checks.

> Source/WebKit/Shared/win/WebEventFactory.cpp:456
> +    WebEvent::Type type             = keyboardEventTypeForEvent(message);
> +    String text                     = textFromEvent(wparam, type);
> +    String unmodifiedText           = unmodifiedTextFromEvent(wparam, type);
> +    String keyIdentifier            = keyIdentifierFromEvent(wparam, type);
> +    int windowsVirtualKeyCode       = static_cast<int>(wparam);
> +    int nativeVirtualKeyCode        = static_cast<int>(wparam);
> +    int macCharCode                 = 0;
> +    bool autoRepeat                 = HIWORD(lparam) & KF_REPEAT;
> +    bool isKeypad                   = isKeypadEvent(wparam, lparam, type);
> +    bool isSystemKey                = isSystemKeyEvent(message);
> +    WebEvent::Modifiers modifiers   = modifiersForCurrentKeyState();

Nit: Wrong indentation style. Never align assignment like this.

[Yousuke Kimoto]

Thank you for your review.

(In reply to Brent Fulgham from comment #5)
> Comment on attachment 334862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334862&action=review
> > 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?
At this point, I just expect that the member is just initialized but it is unnecessary.

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

I'll use this initialization style for NativeWeb*EventWin files.
 
> > 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.
will do.

> > 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.
As don said, PlatformEvent is a good place to define the value. I'll fix it.

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

Ah, it must be fixed.

[Yousuke Kimoto]

>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.
Also we never use those two parameters, then those must be initialized.

MSG is used to just keep 'hwnd', 'message', 'wParam' and 'lParam'. If MSG confuse other developers, I would like to define 4 members to keep them.

[Yousuke Kimoto]

Thank you for your review.

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 334862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334862&action=review
> 
> r=me as an API owner as well.
> 
> > Source/WebKit/Shared/NativeWebWheelEvent.h:81
> > +    MSG m_nativeEvent;
> 
> You probably want to initialize MSG here.
Four members of MSG are used, I think MSG confuse many developers. Since those are initialized at construction, I'll fix it as comment #8.

> > Source/WebKit/Shared/win/WebEventFactory.cpp:110
> > +    if (::GetKeyState(VK_MENU) & 0x8000)
> 
> Instead of keep having & 0x8000 everywhere,
> can we add an inline helper function like IsKeyInDownState(VM_MENU) /
> IsKeyStateDown(::GetKeyState(VK_MENU))?
It is a good idea. will do.

> > Source/WebKit/Shared/win/WebEventFactory.cpp:123
> > +    if (::GetKeyState(VK_CONTROL) & 0x8000)
> > +        modifiers |= WebEvent::ControlKey;
> > +    if (::GetKeyState(VK_SHIFT) & 0x8000)
> > +        modifiers |= WebEvent::ShiftKey;
> > +    if (::GetKeyState(VK_MENU) & 0x8000)
> > +        modifiers |= WebEvent::AltKey;
> 
> Ditto about all these checks.
will do

> > Source/WebKit/Shared/win/WebEventFactory.cpp:456
> > +    WebEvent::Type type             = keyboardEventTypeForEvent(message);
> > +    String text                     = textFromEvent(wparam, type);
> > +    String unmodifiedText           = unmodifiedTextFromEvent(wparam, type);
> > +    String keyIdentifier            = keyIdentifierFromEvent(wparam, type);
> > +    int windowsVirtualKeyCode       = static_cast<int>(wparam);
> > +    int nativeVirtualKeyCode        = static_cast<int>(wparam);
> > +    int macCharCode                 = 0;
> > +    bool autoRepeat                 = HIWORD(lparam) & KF_REPEAT;
> > +    bool isKeypad                   = isKeypadEvent(wparam, lparam, type);
> > +    bool isSystemKey                = isSystemKeyEvent(message);
> > +    WebEvent::Modifiers modifiers   = modifiersForCurrentKeyState();
> 
> Nit: Wrong indentation style. Never align assignment like this.
I'll fix it.

[Fujii Hironori]

Created attachment 337484 [details]
Patch for land

[Fujii Hironori]

Committed r230461: <https://trac.webkit.org/changeset/230461>

[Radar WebKit Bug Importer]

<rdar://problem/39301770>