Bug 63081 - [GTK] [WK2] Fix for getting editor client commands.
Summary: [GTK] [WK2] Fix for getting editor client commands.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 62802 64667 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-06-21 11:23 PDT by Lukasz Slachciak
Modified: 2011-07-17 22:33 PDT (History)
5 users (show)

See Also:


Attachments
Keyboard event patch 1 (1.70 KB, patch)
2011-06-21 11:27 PDT, Lukasz Slachciak
mrobinson: review-
Details | Formatted Diff | Diff
KeyboardEvent type patch (7.54 KB, patch)
2011-06-29 13:04 PDT, Lukasz Slachciak
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lukasz Slachciak 2011-06-21 11:23:58 PDT
In WebKit2 GTK keyboard event type was always set as KeyDown for command list. Additionally removed incorrect ASSERT causing build break on the WebKit2 Gtk debug build.
Please check attached patch.
Comment 1 Lukasz Slachciak 2011-06-21 11:27:48 PDT
Created attachment 98017 [details]
Keyboard event patch 1
Comment 2 Lukasz Slachciak 2011-06-21 11:30:41 PDT
Martin, I subscribed you because git says me that you are author of original code. Please take a look on this simple patch.
Comment 3 Martin Robinson 2011-06-21 11:41:18 PDT
Comment on attachment 98017 [details]
Keyboard event patch 1

Just removing the ASSERT is incorrect here. What type of events are passed to this function? If it's only KeyDown and KeyUp we need a comment explaining why a native  KeyUp event is equivalent to a DOM keypress event. The ASSERT should be updated, since the following line only handles two types of events.
Comment 4 Lukasz Slachciak 2011-06-21 11:52:34 PDT
As my checks show only key down are passed. Trying to find why key up are not passed too. I'll investigate it more and update ASSERT.
Comment 5 Lukasz Slachciak 2011-06-26 06:44:31 PDT
After some investigation I get why GTK port tries to interpret commands only for KeyDown event.
This is because whole command interpretation is done at UIProcess site. But e.g in Qt port it is done at WebProcess site.

But let\s start from the beginning:
exemplary execution log is following:

(I'm putting "d" with keyboard in google search box)


webkitWebViewBaseKeyPressEvent
 WebPageProxy::handleKeyboardEvent: KeyDown
   WebEditorClient::handleKeyboardEvent: keydown
     WebEditorClient::getEditorCommandsForKeyEvent: keydown (message sync)
       WebPageProxy::getEditorCommandsForKeyEvent - commandsList
         PageClientImpl::getEditorCommandsForKeyEvent: KeyDown
		
  WebEditorClient::handleKeyboardEvent: keypress
    WebEditorClient::getEditorCommandsForKeyEvent: keypress (message sync)
      WebPageProxy::getEditorCommandsForKeyEvent - commandsList
	PageClientImpl::getEditorCommandsForKeyEvent: KeyDown
	  WebEditorClient::handleKeyboardEvent inserting text d

  WebPageProxy::didReceiveEvent: KeyDown

webkitWebViewBaseKeyReleaseEvent
  WebPageProxy::handleKeyboardEvent: KeyUp
  WebPageProxy::didReceiveEvent: KeyUp


As you can see for original KeyPress event, two events are generated by  WebPageProxy::handleKeyboardEvent: KeyDown
In fact it is EventHandler::keyEvent who do this. For KeyDown event it generates two events: keydown and keypress which are handled by WebEditorClient::handleKeyboardEvent.
 
When WebEditorClient sends sync message to UI process (Messages::WebPageProxy::GetEditorCommandsForKeyEvent()) it expects that he will get those commands for current event. In the first communication it is not problem. WebPageProxy takes first event from m_keyEventQueue (KeyDown) and calls PageClientImpl::getEditorCommandsForKeyEvent.
But in the second case, after artificially genereated keypress event by EventHandler::keyEvent WebPageProxy doesn't know about it. He will take first event from the queue m_keyEventQueue (KeyDown).

In short words:
1. UIProcess gets KeyDown
2. WebProces handles calling EventHandler in WebCore
3. WebCore generates two event keydown and key press
4. Both events are handled by WebProcess
5. For each event WebProcess asks to interpret commands UIProcess.
6. WebProcess for command interpretation uses current event stored in m_keyEventQueue (which is KeyDown in both cases) AND THIS IS A PROBLEM
7. UIProcess gets KeyUp
8. For KeyUp event EventHandler::keyEvent says that he will not handle it.  


As for the patch I can add ASSERT checking if events are 
KeyDown or KeyPress
but as I described logic above KeyPress will never be passed to PageClientImpl::getEditorCommandsForKeyEvent.
Comment 6 Martin Robinson 2011-06-26 08:37:02 PDT
(In reply to comment #5)
> As for the patch I can add ASSERT checking if events are 
> KeyDown or KeyPress
> but as I described logic above KeyPress will never be passed to PageClientImpl::getEditorCommandsForKeyEvent.

So it looks like this code is buggy. I imagine that we need to pass the actual DOM event through to the UIProcess for it to work properly.
Comment 7 Lukasz Slachciak 2011-06-26 12:49:54 PDT
I agree.
I'm thinking about adding new parameter to WebPageProxy::getEditorCommandsForKeyEvent(Vector<WTF::String>& commandsList)

Let's say:
WebPageProxy::getEditorCommandsForKeyEvent(const KeyEvent& keyEvent, Vector<WTF::String>& commandsList)

 WebEditorClient::getEditorCommandsForKeyEvent will send sync message:

Messages::WebPageProxy::GetEditorCommandsForKeyEvent(event),                                              Messages::WebPageProxy::GetEditorCommandsForKeyEvent::Reply(pendingEditorCommands),
key event will be received by                                  
WebPageProxy::getEditorCommandsForKeyEvent
and it will call
PageClientImpl::getEditorCommandsForKeyEvent

I'll prepare patch for all of these.
Comment 8 Lukasz Slachciak 2011-06-29 08:02:12 PDT
I was thinking about it and tried some different implementations.

I ended up with conclusion:
Why we really need to push this KeyboardEvent from WebProcess to UIProcess (in sync, blocking way)?

What if we could interpret this event just in WebEditorClient::getEditorCommandsForKeyEvent.
As I see in code Qt port also make this interpretation at WebProcess site (see WebPage::handleEditingKeyboardEvent)

Advantages:
- correct KeyboardEvents generated at WebCore site will be processed (not KeyDown NativeWebKeyboardEvent stored in the m_keyEventQueue)
- no need to push KeyboardEvent between processes (and write KeyboardEvent coder/encoder for IPC)
- no communication overhead
Comment 9 Martin Robinson 2011-06-29 11:43:21 PDT
(In reply to comment #8)
> I ended up with conclusion:
> Why we really need to push this KeyboardEvent from WebProcess to UIProcess (in sync, blocking way)?

We want to avoid instantiating a GtkWidget in the WebProcess.
Comment 10 Lukasz Slachciak 2011-06-29 12:17:39 PDT
Thx Martin for comment.

I think that I have finally found optimal solution.
I'll pass from WebProcess to UIProcess eventType. In fact this is what we are missing. We still need to base on the NativeWebKeyboardEvent but for the type which is passed to KeyBindingTranslator we should use KeyboardEvent type obtained from WebProcess.
Comment 11 Lukasz Slachciak 2011-06-29 13:04:11 PDT
Created attachment 99132 [details]
KeyboardEvent type patch

This patch implements passing KeyboardEvent type from WebProcess to UIProcess
Comment 12 Carlos Garcia Campos 2011-07-05 23:56:55 PDT
*** Bug 62802 has been marked as a duplicate of this bug. ***
Comment 13 Martin Robinson 2011-07-17 10:31:32 PDT
*** Bug 64666 has been marked as a duplicate of this bug. ***
Comment 14 Martin Robinson 2011-07-17 17:50:09 PDT
Comment on attachment 99132 [details]
KeyboardEvent type patch

Looks good!
Comment 15 WebKit Review Bot 2011-07-17 18:47:41 PDT
Comment on attachment 99132 [details]
KeyboardEvent type patch

Clearing flags on attachment: 99132

Committed r91169: <http://trac.webkit.org/changeset/91169>
Comment 16 WebKit Review Bot 2011-07-17 18:47:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Martin Robinson 2011-07-17 22:33:31 PDT
*** Bug 64667 has been marked as a duplicate of this bug. ***