Adding New WebInputEvent type for IME events
Created attachment 177889 [details] Patch
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
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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.
Created attachment 177901 [details] Patch
Comment on attachment 177901 [details] Patch Attachment 177901 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15157614
Comment on attachment 177901 [details] Patch Attachment 177901 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15157629
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.
Are IME events ACK'd? I think this is a problem we need to solve on the chromium side.
(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.
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?
(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.
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.
(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.