[Webkit-unassigned] [Bug 64790] [GTK][WK2] Handle doneWithKeyEvent in GTK port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=64790





--- Comment #22 from Ravi Phaneendra Kasibhatla <ravi.kasibhatla at motorola.com>  2011-07-26 07:20:56 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.