Bug 46660 – Need API to tell a WebKit2 client application that a key event was not handled

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

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

<rdar://problem/8483465>

[Adam Roben (:aroben)]

Created attachment 69051 [details]
Tell the UI client when a key event is not handled

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 69051 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4132026

[Adam Roben (:aroben)]

Created attachment 69054 [details]
Tell the UI client when a key event is not handled

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 69054 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4035204

[Kenneth Rohde Christiansen]

A QKeyEvent is a QInputEvent which is a QEvent.

[Adam Roben (:aroben)]

Created attachment 69056 [details]
Tell the UI client when a key event is not handled

[Adam Roben (:aroben)]

EWS++

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 69056 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4042148

[Adam Roben (:aroben)]

Created attachment 69058 [details]
Tell the UI client when a key event is not handled

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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?

[Adam Roben (:aroben)]

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.

[Kenneth Rohde Christiansen]

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.

[Adam Roben (:aroben)]

Created attachment 69065 [details]
Tell the UI client when a key event is not handled

[Sam Weinig]

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

[Adam Roben (:aroben)]

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

[Adam Roben (:aroben)]

Committed r68538: <http://trac.webkit.org/changeset/68538>