Bug 104196 – Adding New WebInputEvent type for IME events

• Component: New Bugs
• Status: RESOLVED INVALID

[Aurimas Liutikas]

Adding New WebInputEvent type for IME events

[Aurimas Liutikas]

Created attachment 177889 [details]
Patch

[Aurimas Liutikas]

Adding new WebInputEvent type for IME events. This will be used to
send IME events through the same path as WebKeyboardEvents, thus
solving out-of-order issues for Android using multi-thread compositing.
More bug info: crbug.com/164470

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 177889 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1
Source/WebKit/chromium/src/android/WebInputEventFactory.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/android/WebInputEventFactory.cpp:73:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/src/android/WebInputEventFactory.cpp:74:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/public/android/WebInputEventFactory.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/public/android/WebInputEventFactory.h:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit/chromium/public/android/WebInputEventFactory.h:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Aurimas Liutikas]

Created attachment 177901 [details]
Patch

[WebKit Review Bot]

Comment on attachment 177901 [details]
Patch

Attachment 177901 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15157614

[Peter Beverloo (cr-android ews)]

Comment on attachment 177901 [details]
Patch

Attachment 177901 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15157629

[James Robinson]

Comment on attachment 177901 [details]
Patch

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

> Source/WebKit/chromium/public/WebInputEvent.h:313
> +    // Caps on string lengths so we can make them static arrays and keep
> +    // them PODs.
> +    static const size_t textLengthCap = 400;
> +
> +    // |text| is the text that needs to be composed, committed, or replaced.
> +    WebUChar text[textLengthCap];

WebInputEvents are assumed to be POD and to be *cheap* to copy.  Making some be 800 bytes long seems contrary to this goal.

[James Robinson]

Are IME events ACK'd?

I think this is a problem we need to solve on the chromium side.

[Min Qin]

(In reply to comment #9)
> Are IME events ACK'd?
> 
> I think this is a problem we need to solve on the chromium side.


They are not ACKed currently.
Even for keyboard event, I believe chromium never actually rely on the ACK as chromium batch sends all the keyevents without waiting for the previous ACK.

[Darin Fisher (:fishd, Google)]

As part of this change, I'd like to understand what is happening to the old way to inputting IME events.  It looks like these events are intended to obsolete WebWidget::{setComposition,confirmComposition} and so on.  What is the grand plan?

[Min Qin]

(In reply to comment #8)
> (From update of attachment 177901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177901&action=review
> 
> > Source/WebKit/chromium/public/WebInputEvent.h:313
> > +    // Caps on string lengths so we can make them static arrays and keep
> > +    // them PODs.
> > +    static const size_t textLengthCap = 400;
> > +
> > +    // |text| is the text that needs to be composed, committed, or replaced.
> > +    WebUChar text[textLengthCap];
> 
> WebInputEvents are assumed to be POD and to be *cheap* to copy.  Making some be 800 bytes long seems contrary to this goal.


I think we can make WebImeEvents relatively small by adding the text to ViewMsg_HandleInputEvent. 
The ViewMsg_HandleInputEvent containing a WebImeEvent may not be passed to the compositor thread, but they will stay in the message queue in input_event_filter. And when the queue is empty or a previous event get acked, we will pass all the following WebImeEvent to the render thread.

[Darin Fisher (:fishd, Google)]

JamesR and I discussed this a bit more, and now I understand better.  You don't need to extend WebInputEvent.  You just need the IME related IPC messages to follow the same "stream" as WebInputEvents.  That can be done without modifying WebInputEvent.  Take a look at how ViewMsg_HandleInputEvent gets routed / queued / etc. on the Chromium side, and mimic that for IME related IPC messages.

[Min Qin]

(In reply to comment #13)
> JamesR and I discussed this a bit more, and now I understand better.  You don't need to extend WebInputEvent.  You just need the IME related IPC messages to follow the same "stream" as WebInputEvents.  That can be done without modifying WebInputEvent.  Take a look at how ViewMsg_HandleInputEvent gets routed / queued / etc. on the Chromium side, and mimic that for IME related IPC messages.

Makes sense, just add another ViewMsg_HandleImeEvent message type and queue it up in the input_event_filter before passing it to the render thread.