Bug 46660

Summary: Need API to tell a WebKit2 client application that a key event was not handled
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: WebKit2Assignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, enrica, kenneth, sam, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Tell the UI client when a key event is not handled
none
Tell the UI client when a key event is not handled
none
Tell the UI client when a key event is not handled
none
Tell the UI client when a key event is not handled
none
Tell the UI client when a key event is not handled sam: review+

Description Adam Roben (:aroben) 2010-09-27 15:04:08 PDT
WebKit2 clients need a way to tell when a web page does not handle a key event. This is needed, e.g., to implement keyboard shortcuts that can be cancelled by a web page via event.preventDefault().
Comment 1 Adam Roben (:aroben) 2010-09-27 15:04:47 PDT
<rdar://problem/8483465>
Comment 2 Adam Roben (:aroben) 2010-09-28 08:03:27 PDT
Created attachment 69051 [details]
Tell the UI client when a key event is not handled
Comment 3 WebKit Review Bot 2010-09-28 08:09:36 PDT
Attachment 69051 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/win/NativeWebKeyboardEventWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/API/C/WKPage.h:166:  Extra space between WKPageDidNotHandleKeyEventCallback and didNotHandleKeyEvent  [whitespace/declaration] [3]
WebKit2/Shared/qt/NativeWebKeyboardEventQt.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/win/WebView.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Early Warning System Bot 2010-09-28 08:11:16 PDT
Attachment 69051 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4132026
Comment 5 Adam Roben (:aroben) 2010-09-28 08:25:38 PDT
Created attachment 69054 [details]
Tell the UI client when a key event is not handled
Comment 6 WebKit Review Bot 2010-09-28 08:27:13 PDT
Attachment 69054 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/win/NativeWebKeyboardEventWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/API/C/WKPage.h:166:  Extra space between WKPageDidNotHandleKeyEventCallback and didNotHandleKeyEvent  [whitespace/declaration] [3]
WebKit2/Shared/qt/NativeWebKeyboardEventQt.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/win/WebView.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Early Warning System Bot 2010-09-28 08:33:30 PDT
Attachment 69054 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4035204
Comment 8 Kenneth Rohde Christiansen 2010-09-28 08:39:15 PDT
A QKeyEvent is a QInputEvent which is a QEvent.
Comment 9 Adam Roben (:aroben) 2010-09-28 08:42:02 PDT
Created attachment 69056 [details]
Tell the UI client when a key event is not handled
Comment 10 Adam Roben (:aroben) 2010-09-28 08:42:24 PDT
EWS++
Comment 11 WebKit Review Bot 2010-09-28 08:45:22 PDT
Attachment 69056 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/win/NativeWebKeyboardEventWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/API/C/WKPage.h:166:  Extra space between WKPageDidNotHandleKeyEventCallback and didNotHandleKeyEvent  [whitespace/declaration] [3]
WebKit2/Shared/qt/NativeWebKeyboardEventQt.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/win/WebView.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Early Warning System Bot 2010-09-28 08:51:31 PDT
Attachment 69056 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4042148
Comment 13 Adam Roben (:aroben) 2010-09-28 08:54:58 PDT
Created attachment 69058 [details]
Tell the UI client when a key event is not handled
Comment 14 WebKit Review Bot 2010-09-28 08:57:07 PDT
Attachment 69058 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/Shared/win/NativeWebKeyboardEventWin.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/API/C/WKPage.h:166:  Extra space between WKPageDidNotHandleKeyEventCallback and didNotHandleKeyEvent  [whitespace/declaration] [3]
WebKit2/Shared/qt/NativeWebKeyboardEventQt.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKit2/UIProcess/win/WebView.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Kenneth Rohde Christiansen 2010-09-28 09:13:26 PDT
Comment on attachment 69058 [details]
Tell the UI client when a key event is not handled

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

> WebKit2/Shared/NativeWebKeyboardEvent.h:46
> +    NativeWebKeyboardEvent(NSEvent *, NSView *);

Why are you not left aligning the *'s? ObjC coding style?

> WebKit2/Shared/NativeWebKeyboardEvent.h:58
> +    const QEvent* nativeEvent() const { return &m_nativeEvent; }

OK, seems fine.

> WebKit2/UIProcess/API/C/WKPage.h:139
> +typedef const void* WKNativeEventPtr;

OK, so in Qt we will just have to supply a QKeyEvent*

> WebKit2/WebProcess/WebPage/WebPage.cpp:557
> +    WebProcess::shared().connection()->send(WebPageProxyMessage::DidReceiveEvent, m_pageID, CoreIPC::In(static_cast<uint32_t>(touchEvent.type()), handled));

Didn't you make it easier to send these messages with your generator script?
Comment 16 Adam Roben (:aroben) 2010-09-28 09:30:52 PDT
Comment on attachment 69058 [details]
Tell the UI client when a key event is not handled

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

>> WebKit2/Shared/NativeWebKeyboardEvent.h:46
>> +    NativeWebKeyboardEvent(NSEvent *, NSView *);
> 
> Why are you not left aligning the *'s? ObjC coding style?

Yes.

>> WebKit2/Shared/NativeWebKeyboardEvent.h:58
>> +    const QEvent* nativeEvent() const { return &m_nativeEvent; }
> 
> OK, seems fine.

Whoops, I meant to make that return a const QKeyEvent*.

>> WebKit2/UIProcess/API/C/WKPage.h:139
>> +typedef const void* WKNativeEventPtr;
> 
> OK, so in Qt we will just have to supply a QKeyEvent*

I did this for now until we find a better solution for Qt. Obviously it would be nice to have the following:

typedef const QKeyEvent* WKNativeEventPtr;

It seemed silly to hold up this patch while we figured out the details of how we're going to accomplish this.

>> WebKit2/WebProcess/WebPage/WebPage.cpp:557
>> +    WebProcess::shared().connection()->send(WebPageProxyMessage::DidReceiveEvent, m_pageID, CoreIPC::In(static_cast<uint32_t>(touchEvent.type()), handled));
> 
> Didn't you make it easier to send these messages with your generator script?

Yes, but we haven't switched over everything to using the new system yet.
Comment 17 Kenneth Rohde Christiansen 2010-09-28 09:35:28 PDT
Comment on attachment 69058 [details]
Tell the UI client when a key event is not handled

The patch seems fine to me, so r=me, but maybe you want Anders or Sam to have a look as well.
Comment 18 Adam Roben (:aroben) 2010-09-28 10:14:51 PDT
Created attachment 69065 [details]
Tell the UI client when a key event is not handled
Comment 19 Sam Weinig 2010-09-28 10:18:34 PDT
Comment on attachment 69065 [details]
Tell the UI client when a key event is not handled

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

> WebKit2/UIProcess/WebPageProxy.cpp:711
>              uint32_t type;
>              if (!arguments->decode(type))
>                  return;
> -            didReceiveEvent((WebEvent::Type)type);
> +            bool handled;
> +            if (!arguments->decode(handled))
> +                return;
> +            didReceiveEvent((WebEvent::Type)type, handled);
>              break;
>          }

We usually do this in one decode step using CoreIPC::Out().
Comment 20 Adam Roben (:aroben) 2010-09-28 10:36:07 PDT
(In reply to comment #19)
> (From update of attachment 69065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69065&action=review
> 
> > WebKit2/UIProcess/WebPageProxy.cpp:711
> >              uint32_t type;
> >              if (!arguments->decode(type))
> >                  return;
> > -            didReceiveEvent((WebEvent::Type)type);
> > +            bool handled;
> > +            if (!arguments->decode(handled))
> > +                return;
> > +            didReceiveEvent((WebEvent::Type)type, handled);
> >              break;
> >          }
> 
> We usually do this in one decode step using CoreIPC::Out().

Fixed. Thanks!
Comment 21 Adam Roben (:aroben) 2010-09-28 10:48:26 PDT
Committed r68538: <http://trac.webkit.org/changeset/68538>