Bug 183043 - [WinCairo] Add WebKit Shared/win event files for wincairo webkit
Summary: [WinCairo] Add WebKit Shared/win event files for wincairo webkit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on: 184296
Blocks: 174003
  Show dependency treegraph
 
Reported: 2018-02-22 09:39 PST by Yousuke Kimoto
Modified: 2018-04-09 18:34 PDT (History)
10 users (show)

See Also:


Attachments
Patch (32.95 KB, patch)
2018-02-23 01:09 PST, Yousuke Kimoto
no flags Details | Formatted Diff | Diff
Patch (33.14 KB, patch)
2018-03-01 15:42 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch for land (33.71 KB, patch)
2018-04-08 23:26 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yousuke Kimoto 2018-02-22 09:39:30 PST
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
Comment 1 Yousuke Kimoto 2018-02-23 01:09:15 PST
Created attachment 334511 [details]
Patch
Comment 2 Don Olmstead 2018-03-01 14:40:43 PST
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
Comment 3 Don Olmstead 2018-03-01 15:42:23 PST
Created attachment 334862 [details]
Patch

Lets see if ios and wpe like this one
Comment 4 youenn fablet 2018-03-02 08:31:59 PST
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.
Comment 5 Brent Fulgham 2018-03-02 09:02:00 PST
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!
Comment 6 Don Olmstead 2018-03-02 11:15:59 PST
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.
Comment 7 Ryosuke Niwa 2018-03-02 12:02:23 PST
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.
Comment 8 Yousuke Kimoto 2018-03-05 06:41:58 PST
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.
Comment 9 Yousuke Kimoto 2018-03-05 06:48:52 PST
>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.
Comment 10 Yousuke Kimoto 2018-03-05 06:54:54 PST
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.
Comment 11 Fujii Hironori 2018-04-08 23:26:51 PDT
Created attachment 337484 [details]
Patch for land
Comment 12 Fujii Hironori 2018-04-09 18:33:59 PDT
Committed r230461: <https://trac.webkit.org/changeset/230461>
Comment 13 Radar WebKit Bug Importer 2018-04-09 18:34:21 PDT
<rdar://problem/39301770>