Bug 64790

Summary: [GTK][WK2] Handle doneWithKeyEvent in GTK port
Product: WebKit Reporter: Ravi Phaneendra Kasibhatla <ravi.kasibhatla>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, amruthraj, cgarcia, mrobinson, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
Fix the tab switch issue
mrobinson: review-
Implementation for doneWithKeyEvent for WK2 GTK port
none
Implementation for doneWithKeyEvent for WK2 GTK port
mrobinson: review-
Implementation for doneWithKeyEvent for WK2 GTK port
none
Implementation for doneWithKeyEvent for WK2 GTK port - revision 4
mrobinson: review-, mrobinson: commit-queue-
Implementation for doneWithKeyEvent for WK2 GTK port - revision 5 none

Description Ravi Phaneendra Kasibhatla 2011-07-19 03:51:42 PDT
WebKitWebViewBase widget should forward key_press event to parent widget only if it is not handled by Web Process. Currently all key_press events are passed to parent widget.

For ex:
Press tab key in www.gmail.com username field. Instead of focusing password field, focus moves to toolbar.
Comment 1 Carlos Garcia Campos 2011-07-19 09:48:28 PDT
Renaming, since didNotHandleKeyEvent was renamed to doneWithKeyEvent in r77694. It seems we added the new method but we didn't remove the old one.
Comment 2 Carlos Garcia Campos 2011-07-19 09:50:13 PDT
I guess we should always return TRUE from key_press_event and key_release_event, and propagate the event again in doneWithKeyEvent() when not handled by the web process.
Comment 3 Ravi Phaneendra Kasibhatla 2011-07-19 09:59:32 PDT
Created attachment 101337 [details]
Fix the tab switch issue
Comment 4 Martin Robinson 2011-07-19 10:09:56 PDT
Comment on attachment 101337 [details]
Fix the tab switch issue

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

Looks good, but I have a few nits.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:216
> +                                             reinterpret_cast<const GdkEventKey*>(event.nativeEvent()));

The indentation is off here and you should just do: &event.nativeEvent()->key or similar.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:330
> +                GTK_WIDGET(webViewBase),
> +                const_cast<GdkEventKey*>(keyEvent));

No need to break this line. It isn't right to simply cast away the constness here. The NativeWebKeyboardEvent is not expecting the GdkEventKey to change. Probably it's better to make a copy of the event and send that.
Comment 5 Ravi Phaneendra Kasibhatla 2011-07-19 23:17:40 PDT
Created attachment 101429 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

Addressing review comments of mrobinson & cgarcia.
Comment 6 Ravi Phaneendra Kasibhatla 2011-07-19 23:19:38 PDT
(In reply to comment #4)
> (From update of attachment 101337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101337&action=review
> 
> Looks good, but I have a few nits.
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:216
> > +                                             reinterpret_cast<const GdkEventKey*>(event.nativeEvent()));
> 
> The indentation is off here and you should just do: &event.nativeEvent()->key or similar.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:330
> > +                GTK_WIDGET(webViewBase),
> > +                const_cast<GdkEventKey*>(keyEvent));
> 
> No need to break this line. It isn't right to simply cast away the constness here. The NativeWebKeyboardEvent is not expecting the GdkEventKey to change. Probably it's better to make a copy of the event and send that.
When invoking GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event, we need to do const_cast<GdkEventKey*> since the API expects a non-const GdkEventKey* and we have a const GdkEventKey*.
Addressed the other points in the comment.
Comment 7 Carlos Garcia Campos 2011-07-20 00:10:51 PDT
Comment on attachment 101429 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

With the patch the focus never moves to the toolbar, I'm not sure what the right behaviour is, but with GtkLauncher the toolbar is focused at some point after pressing tab several times.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:329
> +void webkitWebViewBaseDidNotHandleKeyEvent(WebKitWebViewBase* webViewBase, const GdkEventKey* keyEvent)
> +{
> +    GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(GTK_WIDGET(webViewBase), const_cast<GdkEventKey*>(keyEvent));
> +}
> +

I'm not sure about this, maybe it would be better to use gtk_main_do_event() so that the event is properly handled again by GTK and added to the stack. Event callbacks expect functions gtk_get_current_event* to work. We would need to add a boolean flag to tell the view we are forwarding an event not handled by the web process and simply call gtk_main_do_event(). In key_press and key_release, we check that flag and if set, we simply unset it and call key_press_event on parent widget.
Comment 8 Ravi Phaneendra Kasibhatla 2011-07-20 09:43:00 PDT
Created attachment 101478 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

Addresses the comments given by Carlos for using gtk_main_do_event()
Comment 9 Ravi Phaneendra Kasibhatla 2011-07-20 09:43:15 PDT
(In reply to comment #7)
> (From update of attachment 101429 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101429&action=review
> 
> With the patch the focus never moves to the toolbar, I'm not sure what the right behaviour is, but with GtkLauncher the toolbar is focused at some point after pressing tab several times.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:329
> > +void webkitWebViewBaseDidNotHandleKeyEvent(WebKitWebViewBase* webViewBase, const GdkEventKey* keyEvent)
> > +{
> > +    GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(GTK_WIDGET(webViewBase), const_cast<GdkEventKey*>(keyEvent));
> > +}
> > +
> 
> I'm not sure about this, maybe it would be better to use gtk_main_do_event() so that the event is properly handled again by GTK and added to the stack. Event callbacks expect functions gtk_get_current_event* to work. We would need to add a boolean flag to tell the view we are forwarding an event not handled by the web process and simply call gtk_main_do_event(). In key_press and key_release, we check that flag and if set, we simply unset it and call key_press_event on parent widget.
Done.
Comment 10 Carlos Garcia Campos 2011-07-20 09:54:15 PDT
Comment on attachment 101478 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:213
> +        webkitWebViewBaseDidNotHandleKeyEvent(webkitWebViewBase, event.nativeEvent());

We don't need this method, we can just call gtk_main_do_event here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:339
> +    WebKitWebViewBasePrivate* priv = webkitWebViewBase->priv;
> +    priv->shouldForwardEvent = TRUE;

This could be a single line webkitWebViewBase->priv->shouldForwardEvent = TRUE;
Comment 11 Martin Robinson 2011-07-22 23:41:02 PDT
Comment on attachment 101478 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

I have a few concerns below. Please also take into account Carlos's suggestions.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:56
> +    gboolean shouldForwardEvent;

I think that shouldForwardKeyboardEvent would be a better name here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:208
> +    // Since WebProcess key event handling is not synchronous, make it 2-pass handling.
> +    // First, always return TRUE in event handler.
> +    // Second, PageClientImpl::doneWithKeyEvent return status of event handling in WebProcess.
> +    // Third, using status, determine whether to pass the event to parent or not using gtk_main_do_event().

I think this would be easier to read as a paragraph. Something like:

// Since WebProcess key event handling is not synchronous, we must handle this event in two passes. When
// the WebProcess is done handling the event it will call PageClientImpl::doneWithKeyEvent, where we will
// be able to determine whether to pass the event to our parent or not using gtk_main_do_event().

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:212
> +    if (priv->shouldForwardEvent) {
> +        priv->shouldForwardEvent = FALSE;
> +        return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> +    }

I'm not sure how I understand how priv->shouldForwardEvent can be true before you call PageProxy::handleKeyboardEvent below.
Comment 12 Martin Robinson 2011-07-22 23:42:45 PDT
Comment on attachment 101478 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:345
> +    gtk_main_do_event(const_cast<GdkEvent*>(event));

I still believe that it is incorrect to simply cast away the constness of this GdkEvent. It's probably more correct to make a copy of the event and send that.
Comment 13 Ravi Phaneendra Kasibhatla 2011-07-25 00:58:31 PDT
Created attachment 101854 [details]
Implementation for doneWithKeyEvent for WK2 GTK port
Comment 14 Ravi Phaneendra Kasibhatla 2011-07-25 00:58:57 PDT
(In reply to comment #10)
> (From update of attachment 101478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101478&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:213
> > +        webkitWebViewBaseDidNotHandleKeyEvent(webkitWebViewBase, event.nativeEvent());
> 
> We don't need this method, we can just call gtk_main_do_event here.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:339
> > +    WebKitWebViewBasePrivate* priv = webkitWebViewBase->priv;
> > +    priv->shouldForwardEvent = TRUE;
> 
> This could be a single line webkitWebViewBase->priv->shouldForwardEvent = TRUE;
Done.
Comment 15 Ravi Phaneendra Kasibhatla 2011-07-25 01:03:24 PDT
(In reply to comment #11)
> (From update of attachment 101478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101478&action=review
> 
> I have a few concerns below. Please also take into account Carlos's suggestions.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:56
> > +    gboolean shouldForwardEvent;
> 
> I think that shouldForwardKeyboardEvent would be a better name here.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:208
> > +    // Since WebProcess key event handling is not synchronous, make it 2-pass handling.
> > +    // First, always return TRUE in event handler.
> > +    // Second, PageClientImpl::doneWithKeyEvent return status of event handling in WebProcess.
> > +    // Third, using status, determine whether to pass the event to parent or not using gtk_main_do_event().
> 
> I think this would be easier to read as a paragraph. Something like:
> 
> // Since WebProcess key event handling is not synchronous, we must handle this event in two passes. When
> // the WebProcess is done handling the event it will call PageClientImpl::doneWithKeyEvent, where we will
> // be able to determine whether to pass the event to our parent or not using gtk_main_do_event().
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:212
> > +    if (priv->shouldForwardEvent) {
> > +        priv->shouldForwardEvent = FALSE;
> > +        return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> > +    }
> 
> I'm not sure how I understand how priv->shouldForwardEvent can be true before you call PageProxy::handleKeyboardEvent below.
shouldForwardKeyboardEvent will not be true when we are calling PageProxy::handleKeyboardEvent. It would be FALSE and hence event would be sent to PageProxy for handling. On handling the input event, when we get the call back to PageClientImpl::doneWithKeyEvent, we check for handled status. If it is false, then we call webkitWebViewBaseSetForwardEvent which set shouldForwardKeyboardEvent to TRUE. Then we call gtk_main_do_event which triggers key_press_event where we pass the event to parent widget.
Comment 16 Ravi Phaneendra Kasibhatla 2011-07-25 01:03:42 PDT
(In reply to comment #12)
> (From update of attachment 101478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101478&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:345
> > +    gtk_main_do_event(const_cast<GdkEvent*>(event));
> 
> I still believe that it is incorrect to simply cast away the constness of this GdkEvent. It's probably more correct to make a copy of the event and send that.
Done.
Comment 17 Martin Robinson 2011-07-25 11:37:05 PDT
Comment on attachment 101854 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

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

I'm a bit concerned about the interaction of key release and following key press events.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:210
> +    if (!wasEventHandled) {

Please just use an early return here.

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:212
> +        webkitWebViewBaseSetForwardEvent(webkitWebViewBase);

With the way the code is written now, what happens when this is called on a key release event? Won't this setting then effect the behavior of the next key press?

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:213
> +        GdkEvent inputEvent = *event.nativeEvent();

It's probably better to use: GOwnPtr<GdkEvent> inputEvent(gdk_event_copy(event.nativeEvent())); here.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:214
> +    // Since WebProcess key event handling is not synchronous, handle the event in two passes.
> +    // When WebProcess processes the input event, it will call PageClientImpl::doneWithKeyEvent 
> +    // with event handled status which determines whether to pass the input event to parent or not 
> +    // using gtk_main_do_event().
> +    if (priv->shouldForwardKeyboardEvent) {
> +        priv->shouldForwardKeyboardEvent = FALSE;
> +        return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> +    }
>      priv->pageProxy->handleKeyboardEvent(NativeWebKeyboardEvent(reinterpret_cast<GdkEvent*>(event)));
> -
> -    return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> +    return TRUE;

I should clarify my confusion here. I do not understand how priv->shouldForwardKeyboardEvent can be true when webkitWebViewBaseKeyPressEvent is called. The only way I can see this happening is if during a previous key release event, priv->shouldForwardKeyboardEvent was set to true. I do not think previous key release events should affect future key press events (see my comment above).

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:339
> +void webkitWebViewBaseSetForwardEvent(WebKitWebViewBase* webkitWebViewBase)
> +{
> +    webkitWebViewBase->priv->shouldForwardKeyboardEvent = TRUE;
> +}

I think you can just eliminate this method entirely and do this directly in PageClientImpl::doneWithKeyEvent.
Comment 18 Carlos Garcia Campos 2011-07-25 23:36:35 PDT
(In reply to comment #12)
> (From update of attachment 101478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101478&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:345
> > +    gtk_main_do_event(const_cast<GdkEvent*>(event));
> 
> I still believe that it is incorrect to simply cast away the constness of this GdkEvent. It's probably more correct to make a copy of the event and send that.

We don't need to copy the event, gtk_main_do_event() doesn't modify the event at all.
Comment 19 Carlos Garcia Campos 2011-07-25 23:43:35 PDT
(In reply to comment #17)
> (From update of attachment 101854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101854&action=review
> 
> I'm a bit concerned about the interaction of key release and following key press events.
> 
> 
> With the way the code is written now, what happens when this is called on a key release event? Won't this setting then effect the behavior of the next key press?

shouldForwardKeyboardEvent is set to false in both key press and release event handlers, so it won't affect any other events.

> > -
> > -    return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> > +    return TRUE;
> 
> I should clarify my confusion here. I do not understand how priv->shouldForwardKeyboardEvent can be true when webkitWebViewBaseKeyPressEvent is called. The only way I can see this happening is if during a previous key release event, priv->shouldForwardKeyboardEvent was set to true. I do not think previous key release events should affect future key press events (see my comment above).

shouldForwardKeyboardEvent is only true when a key press/release event is not handled by the web process, in this case we simply take the same event and send it to gtk to be processed again. Then key_press or key_release will be called, and shouldForwardKeyboardEvent will be true, so the handler will send the event to the parent widget and will set shouldForwardKeyboardEvent to false again so it won't affect future events. It's not a previous event, we are processing the same event again, so other key events are not affected.
Comment 20 Ravi Phaneendra Kasibhatla 2011-07-26 07:10:21 PDT
Created attachment 101997 [details]
Implementation for doneWithKeyEvent for WK2 GTK port - revision 4

Revision 4 - based on latest comments from Martin.
Comment 21 Ravi Phaneendra Kasibhatla 2011-07-26 07:11:09 PDT
Comment on attachment 101854 [details]
Implementation for doneWithKeyEvent for WK2 GTK port

Updated with revision 4 patch.
Comment 22 Ravi Phaneendra Kasibhatla 2011-07-26 07:20:56 PDT
(In reply to comment #17)
> (From update of attachment 101854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101854&action=review
> 
> I'm a bit concerned about the interaction of key release and following key press events.
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:210
> > +    if (!wasEventHandled) {
> 
> Please just use an early return here.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:212
> > +        webkitWebViewBaseSetForwardEvent(webkitWebViewBase);
> 
> With the way the code is written now, what happens when this is called on a key release event? Won't this setting then effect the behavior of the next key press?
Will be addressed in new enum type implementation.
> 
> > Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:213
> > +        GdkEvent inputEvent = *event.nativeEvent();
> 
> It's probably better to use: GOwnPtr<GdkEvent> inputEvent(gdk_event_copy(event.nativeEvent())); here.
Done.
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:214
> > +    // Since WebProcess key event handling is not synchronous, handle the event in two passes.
> > +    // When WebProcess processes the input event, it will call PageClientImpl::doneWithKeyEvent 
> > +    // with event handled status which determines whether to pass the input event to parent or not 
> > +    // using gtk_main_do_event().
> > +    if (priv->shouldForwardKeyboardEvent) {
> > +        priv->shouldForwardKeyboardEvent = FALSE;
> > +        return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> > +    }
> >      priv->pageProxy->handleKeyboardEvent(NativeWebKeyboardEvent(reinterpret_cast<GdkEvent*>(event)));
> > -
> > -    return GTK_WIDGET_CLASS(webkit_web_view_base_parent_class)->key_press_event(widget, event);
> > +    return TRUE;
> 
> I should clarify my confusion here. I do not understand how priv->shouldForwardKeyboardEvent can be true when webkitWebViewBaseKeyPressEvent is called. The only way I can see this happening is if during a previous key release event, priv->shouldForwardKeyboardEvent was set to true. I do not think previous key release events should affect future key press events (see my comment above).
Carlos has explained in the below comment about the this design. We reset the flag in each key_press_event or key_release_event handler to prevent any effect on future events. Like we discussed, I have made it an enum rather than bool, to pass on specifically the event not handled to parent.

I still though have one concern in calling gtk_main_do_event(). Consider a usecase, where key_press gets handled by web process but not key_release. Since we are calling gtk_main_do_event(), we again get key_press & key_release callbacks to WebKitWebViewBase widget. With our flag for handled status, we will pass key_release to parent widget, but key_press gets propagated again to web process. Wouldn't that be an issue?
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:339
> > +void webkitWebViewBaseSetForwardEvent(WebKitWebViewBase* webkitWebViewBase)
> > +{
> > +    webkitWebViewBase->priv->shouldForwardKeyboardEvent = TRUE;
> > +}
> 
> I think you can just eliminate this method entirely and do this directly in PageClientImpl::doneWithKeyEvent.
Done.
Comment 23 Martin Robinson 2011-07-26 07:29:53 PDT
(In reply to comment #22)

> I still though have one concern in calling gtk_main_do_event(). Consider a usecase, where key_press gets handled by web process but not key_release. Since we are calling gtk_main_do_event(), we again get key_press & key_release callbacks to WebKitWebViewBase widget. With our flag for handled status, we will pass key_release to parent widget, but key_press gets propagated again to web process. Wouldn't that be an issue?

Yes, that's an issue. We need to throw the event away in that case.
Comment 24 Carlos Garcia Campos 2011-07-26 08:14:24 PDT
Comment on attachment 101997 [details]
Implementation for doneWithKeyEvent for WK2 GTK port - revision 4

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

> Source/WebKit2/UIProcess/API/gtk/PageClientImpl.cpp:220
> +    GOwnPtr<GdkEvent> inputEvent(gdk_event_copy(event.nativeEvent()));
> +    gtk_main_do_event(inputEvent.get());

I think we shouldn't copy the event, gtk_main_do_event() doesn't modify the event.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:-60
> -struct _WebKitWebViewBasePrivate {
> -    OwnPtr<PageClientImpl> pageClient;
> -    RefPtr<WebPageProxy> pageProxy;
> -    gboolean isPageActive;
> -    GtkIMContext* imContext;
> -    GtkClickCounter clickCounter;
> -    CString tooltipText;
> -};
> -

I prefer to keep private structures as private and add specific private API when needed, it makes easier to know who and why is using the private members outside the object.
Comment 25 Martin Robinson 2011-07-26 08:15:51 PDT
(In reply to comment #24)

> I prefer to keep private structures as private and add specific private API when needed, it makes easier to know who and why is using the private members outside the object.

I suggested this because it's a pattern we commonly use in WebKit1.
Comment 26 Carlos Garcia Campos 2011-07-26 08:24:12 PDT
(In reply to comment #22)
> I still though have one concern in calling gtk_main_do_event(). Consider a usecase, where key_press gets handled by web process but not key_release. Since we are calling gtk_main_do_event(), we again get key_press & key_release callbacks to WebKitWebViewBase widget. With our flag for handled status, we will pass key_release to parent widget, but key_press gets propagated again to web process. Wouldn't that be an issue?

No, with gtk_main_do_event() we only process the event that was not handled by the we process, if key press is handled by not key release, doneWithKeyEvent will be called with handled=true only for the key press event, so we will call gtk_main_do_event() only for the key release. I don't think we need the enum, since doneWithKeyEvent will be called for every key press/release event, the boolean is enough, events are handled synchronously by gtk+.
Comment 27 Ravi Phaneendra Kasibhatla 2011-07-28 07:49:09 PDT
Created attachment 102258 [details]
Implementation for doneWithKeyEvent for WK2 GTK port - revision 5

Addressed the concerns & comments of carlos & martin.
As per explanation given by Carlos, since gtk_main_do_event is sync & gets invoked only for event which is being passed gtk_main_do_event, reverting to boolean flag. 
As per understanding with Martin & Carlos now keeping the WebKitWebViewPrivate in .cpp only and exposing a new API to set the flag from PageClientImpl.
As per discussion with Carlos & Martin, NativeWebKeyboardEvent::nativeEvent now returns a non-const GdkEvent. Removing the casting & event copy from PageClientImpl.
Comment 28 WebKit Review Bot 2011-07-28 10:25:00 PDT
Comment on attachment 102258 [details]
Implementation for doneWithKeyEvent for WK2 GTK port - revision 5

Clearing flags on attachment: 102258

Committed r91937: <http://trac.webkit.org/changeset/91937>
Comment 29 WebKit Review Bot 2011-07-28 10:25:06 PDT
All reviewed patches have been landed.  Closing bug.