Bug 183043

Created attachment 334511 [details] Patch

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

Created attachment 334862 [details] Patch Lets see if ios and wpe like this one

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

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.

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

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.

Created attachment 337484 [details] Patch for land

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

<rdar://problem/39301770>