Created attachment 66021 [details] patch describing the point of the crash Web process crashes when pressing modifiers in input field Null pointer reference in PlatformKeyboardEvent.
Created attachment 66029 [details] Patch fixing the crash
Comment on attachment 66029 [details] Patch fixing the crash > +#if defined(XP_UNIX) > +uint32_t PlatformKeyboardEvent::x11NativeModifiers() const > +{ > + ASSERT(m_qtEvent); > + return m_qtEvent->nativeModifiers(); > +} > + > +uint32_t PlatformKeyboardEvent::x11NativeScanCode() const > +{ > + ASSERT(m_qtEvent); > + return m_qtEvent->nativeScanCode(); > +} > +#endif This is confusing. Can you explain what is the reason behind introducing these two Unix-only functions?
(In reply to comment #2) > (From update of attachment 66029 [details]) > > +#if defined(XP_UNIX) > > +uint32_t PlatformKeyboardEvent::x11NativeModifiers() const > > +{ > > + ASSERT(m_qtEvent); > > + return m_qtEvent->nativeModifiers(); > > +} > > + > > +uint32_t PlatformKeyboardEvent::x11NativeScanCode() const > > +{ > > + ASSERT(m_qtEvent); > > + return m_qtEvent->nativeScanCode(); > > +} > > +#endif > > This is confusing. Can you explain what is the reason behind introducing these two Unix-only functions? The idea is that X11 plugins can be supported even though PlatformKeyboardEvent may not be instantiated from QKeyEvent In other words: the implementation tries to remove the notion that PlatformKeyboardEvent is QKeyEvent. Otherwise we must synthetize QKeyEvent from WebKeyboardEvent. This is a bit redundant. On top of redundancy, this is not even possible, because there's no such a constructor for QKeyEvent (X11 Qt uses QKeyEventEx internally)
> In other words: the implementation tries to remove the notion that PlatformKeyboardEvent is QKeyEvent. I did not ask what the patch is doing, which is quite clear, but rather why it is implemented that way. Here is what I had in mind when thinking about this problem. There will be nativeModifiers() and nativeScanCode() for PlatformKeyboardEvent under #if PLATFORM(QT), i.e. no need to specialize for XP_UNIX. Can this work?
(In reply to comment #4) > > In other words: the implementation tries to remove the notion that PlatformKeyboardEvent is QKeyEvent. > > I did not ask what the patch is doing, which is quite clear, but rather why it is implemented that way. > > Here is what I had in mind when thinking about this problem. There will be nativeModifiers() and nativeScanCode() for PlatformKeyboardEvent under #if PLATFORM(QT), i.e. no need to specialize for XP_UNIX. Can this work? You mean that remove the #if XP_UNIX and remove the x11 prefix? Should work, though I don't see any point where the other qt platforms use the methods. It's ok for me, if it's ok for you.
> You mean that remove the #if XP_UNIX and remove the x11 prefix? Should work, though I don't see any point where the other qt platforms use the methods. It's ok for me, if it's ok for you. IMHO it makes the code more readable and less confusing.
Created attachment 69773 [details] Patch try #2 Patch addressing the comments.
Comment on attachment 69773 [details] Patch try #2 Clearing flags on attachment: 69773 Committed r69105: <http://trac.webkit.org/changeset/69105>
All reviewed patches have been landed. Closing bug.
(In reply to comment #8) > (From update of attachment 69773 [details]) > Clearing flags on attachment: 69773 > > Committed r69105: <http://trac.webkit.org/changeset/69105> It broke plugins/keyboard-events.html on Qt bot: http://build.webkit.org/results/Qt%20Linux%20Release/r69105%20%2821509%29/plugins/keyboard-events-pretty-diff.html
reopening due to a regression.
Created attachment 69785 [details] Fix for the test regression
Comment on attachment 69773 [details] Patch try #2 View in context: https://bugs.webkit.org/attachment.cgi?id=69773&action=review > WebCore/platform/qt/PlatformKeyboardEventQt.cpp:579 > +} We should reorganize the members in the enum to do this test it in O(1). Do you agree?
(In reply to comment #13) > (From update of attachment 69773 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69773&action=review > > > WebCore/platform/qt/PlatformKeyboardEventQt.cpp:579 > > +} > > We should reorganize the members in the enum to do this test it in O(1). Do you agree? It isn't an enum, but defines. We don't need to reorganize it, because GCC (and other compilers) generates a jump table for this case construction to execute the binary in O(1). ;)
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 69773 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=69773&action=review > > > > > WebCore/platform/qt/PlatformKeyboardEventQt.cpp:579 > > > +} > > > > We should reorganize the members in the enum to do this test it in O(1). Do you agree? > > It isn't an enum, but defines. Doesn't matter. > We don't need to reorganize it, > because GCC (and other compilers) generates a jump table > for this case construction to execute the binary in O(1). ;) Hm, are you sure? It could do that but I am afraid that the compiler is not so clever (or at least not all the compilers). Do you have an experience about that?
Comment on attachment 69785 [details] Fix for the test regression Clearing flags on attachment: 69785 Committed r69134: <http://trac.webkit.org/changeset/69134>
(In reply to comment #13) > (From update of attachment 69773 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69773&action=review > > > WebCore/platform/qt/PlatformKeyboardEventQt.cpp:579 > > +} > > We should reorganize the members in the enum to do this test it in O(1). Do you agree? We should, especially if it shows up in your profiles as a hotspot. Did it? It really doesn't show up for me, though, so it seems a bit of a wasted effort, effort which I would like to spend in some other place. I don't know the optimization technique. I cannot modify the enum as it is implemented in Qt. Do you mean that the order of the case statements should match the order of the enum values? I'll attach an example output from my compiler. I'm no expert in this, so I cannot say for sure, but something seems to be rearranged by the looks of it. If the output doesn't match your expectation, feel free to rearrange switch.
Created attachment 69891 [details] keyIdentifierForQtKeyCode compiled by probably not exceptionally "smart" compiler
Comment on attachment 69891 [details] keyIdentifierForQtKeyCode compiled by probably not exceptionally "smart" compiler oops, I noticed we were talking about different function.
Created attachment 69894 [details] disambiguateKeyDownEvent compiled Again, I'm no expert, but i think the switch is compiled to following sequence: .loc 1 505 0 cmp r1, #111 bgt .L399 cmp r1, #96 bge .L398 cmp r1, #57 bgt .L400 cmp r1, #48 bge .L398 cmp r1, #32 beq .L398
Seems to be optimized enough. Thanks for dealing with that.