Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1267483003:
Combine the WM_CHAR with WM_KEY* for key event flow. (Closed)
|
Created:
5 years, 4 months ago by Shu Chen Modified:
5 years, 2 months ago CC:
chromium-reviews, nona+watch_chromium.org, sadrul, yusukes+watch_chromium.org, shuchen+watch_chromium.org, scottmg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCombine the WM_CHAR with WM_KEY* for key event flow.
The combination is required for Chrome IMEs, which can do IME related actions based on the single event.
Chrome IME extension may wants to consume certain key events based on the character information of WM_CHAR messages. Holding WM_KEY* messages until WM_CHAR is processed by the IME extension is not feasible because there is no way to know wether there will or not be a WM_CHAR following the WM_KEY*.
What's more, with the combination, ui::KeyEvent::is_char() can be removed, can reduce the complexity for key event handling.
This cl will change a little for the order of message handling. e.g.
Original:
- WM_KEYDOWN received in message pump.
- TranslateMessage is called from message pump.
- WM_KEYDOWN is dispatched to HWNDMessageHandler.
- WM_KEYDOWN is dispatched to Chrome by IMF.
With this cl:
- WM_KEYDOWN received in message pump.
- WM_KEYDOWN is dispatched to HWNDMessageHandler.
- TranslateMessage is called from IMF.
- WM_KEYDOWN is dispatched to Chrome by IMF.
Also, this cl makes WM_CHAR messages (generated by WM_KEY*) never hit message pump & HWNDMessageHandler.
Note: the behavior changes by this cl is behind the flag merge-key-char-events, and the flag default is "disabled".
TBR=isherman@chromium.org
BUG=517773
TEST=No regressions for text inputing (including dead keys & CJK IMEs) in Windows 7 and Windows 8 metro.
Committed: https://crrev.com/cb0b75575e1ebab71e76aaeb8784c18f95c0c145
Cr-Commit-Position: refs/heads/master@{#353106}
Patch Set 1 #
Total comments: 10
Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : addressed comments. #
Total comments: 6
Patch Set 5 : nits. #Patch Set 6 : fixed compiling errors. #Patch Set 7 : Added MetroViewerHostMsg_CharacterForNextKeyEvent, so that WM_UNICHAR can be supported. #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 2
Patch Set 12 : addressed suzhe@'s comment. #
Total comments: 4
Patch Set 13 : nit. #Patch Set 14 : addressed robliao@'s comments. #
Total comments: 8
Patch Set 15 : nits. #
Total comments: 6
Patch Set 16 : addressed comments. #Patch Set 17 : add --disable-merge-key-char-events flag. #
Total comments: 6
Patch Set 18 : addressed yukawa@'s comments. #
Total comments: 4
Patch Set 19 : put flag in ui_base_switches instead of base_switches. #Patch Set 20 : turn off the flag by default. #
Total comments: 4
Patch Set 21 : fixed an issue and use set_character(). #Patch Set 22 : reverted changes for win8 metro. #Patch Set 23 : #Patch Set 24 : #
Total comments: 2
Patch Set 25 : rebased. #Patch Set 26 : #Patch Set 27 : fixed the compiling failure on win_chromium_compile_dbg_ng. #Patch Set 28 : #Messages
Total messages: 110 (18 generated)
shuchen@chromium.org changed reviewers: + suzhe@chromium.org, yukawa@chromium.org
Guys, can you please take a look? Yohei, do you have any ideas of how to support win8 metro driver & RemoteInputMethodWin? Thanks
https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:147: if (details.dispatcher_destroyed || event->stopped_propagation()) is it necessary to check details.target_destroyed?
https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:110: while (::PeekMessage(&msg, native_key_event.hwnd, WM_CHAR, WM_SYSCHAR, is it necessary to handle WM_DEADCHAR and WM_SYSDEADCHAR?
https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:110: while (::PeekMessage(&msg, native_key_event.hwnd, WM_CHAR, WM_SYSCHAR, On 2015/08/03 07:31:18, James Su wrote: > is it necessary to handle WM_DEADCHAR and WM_SYSDEADCHAR? No, chrome nevers handles WM_DEADCHAR & WM_SYSDEADCHAR.
https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:110: while (::PeekMessage(&msg, native_key_event.hwnd, WM_CHAR, WM_SYSCHAR, On 2015/08/03 07:39:59, Shu Chen wrote: > On 2015/08/03 07:31:18, James Su wrote: > > is it necessary to handle WM_DEADCHAR and WM_SYSDEADCHAR? > > No, chrome nevers handles WM_DEADCHAR & WM_SYSDEADCHAR. The fact that Chrome has never handled WM_DEADCHAR & WM_SYSDEADCHAR does not prevent us from removing WM_DEADCHAR & WM_SYSDEADCHAR. To me removing them sounds a bit safer than not removing them.
https://codereview.chromium.org/1267483003/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (left): https://codereview.chromium.org/1267483003/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:404: TranslateMessage(&msg); better add comment to explain why we don't call TranslateMessage() here.
https://codereview.chromium.org/1267483003/diff/1/base/message_loop/message_p... File base/message_loop/message_pump_win.cc (left): https://codereview.chromium.org/1267483003/diff/1/base/message_loop/message_p... base/message_loop/message_pump_win.cc:404: TranslateMessage(&msg); On 2015/08/03 16:09:00, James Su wrote: > better add comment to explain why we don't call TranslateMessage() here. Done. https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:110: while (::PeekMessage(&msg, native_key_event.hwnd, WM_CHAR, WM_SYSCHAR, On 2015/08/03 08:09:46, yukawa wrote: > On 2015/08/03 07:39:59, Shu Chen wrote: > > On 2015/08/03 07:31:18, James Su wrote: > > > is it necessary to handle WM_DEADCHAR and WM_SYSDEADCHAR? > > > > No, chrome nevers handles WM_DEADCHAR & WM_SYSDEADCHAR. > > The fact that Chrome has never handled WM_DEADCHAR & WM_SYSDEADCHAR does not > prevent us from removing WM_DEADCHAR & WM_SYSDEADCHAR. To me removing them > sounds a bit safer than not removing them. What do you mean by removing WM_DEADCHAR? If you meant to remove WM_DEADCHAR in the message queue, the current change does already do so. https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:147: if (details.dispatcher_destroyed || event->stopped_propagation()) On 2015/08/03 07:07:11, James Su wrote: > is it necessary to check details.target_destroyed? Done.
https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:110: while (::PeekMessage(&msg, native_key_event.hwnd, WM_CHAR, WM_SYSCHAR, On 2015/08/04 08:18:42, Shu Chen wrote: > On 2015/08/03 08:09:46, yukawa wrote: > > On 2015/08/03 07:39:59, Shu Chen wrote: > > > On 2015/08/03 07:31:18, James Su wrote: > > > > is it necessary to handle WM_DEADCHAR and WM_SYSDEADCHAR? > > > > > > No, chrome nevers handles WM_DEADCHAR & WM_SYSDEADCHAR. > > > > The fact that Chrome has never handled WM_DEADCHAR & WM_SYSDEADCHAR does not > > prevent us from removing WM_DEADCHAR & WM_SYSDEADCHAR. To me removing them > > sounds a bit safer than not removing them. > > What do you mean by removing WM_DEADCHAR? > If you meant to remove WM_DEADCHAR in the message queue, the current change does > already do so. Ah, sorry I misunderstood this code. Can you leave a comment about what messages will be removed in practice? IIUC, they are: - WM_CHAR (0x0102) - WM_DEADCHAR (0x0103) - WM_SYSKEYDOWN (0x0104) - WM_SYSKEYUP (0x0105) - WM_SYSCHAR (0x0106) BTW, is it intentional that we do not remove WM_SYSDEADCHAR (0x0107)?
https://codereview.chromium.org/1267483003/diff/40001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1267483003/diff/40001/base/message_loop/messa... base/message_loop/message_pump_win.cc:404: // InputMethodWin, so that the WM_KEYDOWN & WM_CHAR can be combined for key WM_KEYDOWN/WM_KEYUP https://codereview.chromium.org/1267483003/diff/40001/ui/base/ime/input_metho... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/40001/ui/base/ime/input_metho... ui/base/ime/input_method_win.cc:111: PM_REMOVE)) { There are WM_SYSKEYDOWN and WM_SYSKEYUP between WM_CHAR and WM_SYSCHAR. And how about MW_SYSDEADCHAR? I think you should better filter WM_CHAR (102) / WM_DEADCHAR (103) and WM_SYSCHAR (106) / WM_SYSDEADCHAR (107) separately. And you probably can simply discard WM_DEADCHAR and WM_SYSDEADCHAR events for now. https://codereview.chromium.org/1267483003/diff/40001/ui/base/ime/remote_inpu... File ui/base/ime/remote_input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/40001/ui/base/ime/remote_inpu... ui/base/ime/remote_input_method_win.cc:210: pending_char_events_.clear(); this cache should always be cleared for WM_KEYDOWN/WM_KEYUP https://codereview.chromium.org/1267483003/diff/40001/win8/metro_driver/metro... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/40001/win8/metro_driver/metro... win8/metro_driver/metro_driver_win7.cc:917: while (::PeekMessage(&char_msg, msg.hwnd, WM_CHAR, WM_SYSCHAR, ditto And WM_KEYDOWN/WM_KEYUP won't generate WM_SYSCHAR/WM_SYSDEADCHAR.
https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/1/ui/base/ime/input_method_wi... ui/base/ime/input_method_win.cc:110: while (::PeekMessage(&msg, native_key_event.hwnd, WM_CHAR, WM_SYSCHAR, On 2015/08/04 08:35:07, yukawa wrote: > On 2015/08/04 08:18:42, Shu Chen wrote: > > On 2015/08/03 08:09:46, yukawa wrote: > > > On 2015/08/03 07:39:59, Shu Chen wrote: > > > > On 2015/08/03 07:31:18, James Su wrote: > > > > > is it necessary to handle WM_DEADCHAR and WM_SYSDEADCHAR? > > > > > > > > No, chrome nevers handles WM_DEADCHAR & WM_SYSDEADCHAR. > > > > > > The fact that Chrome has never handled WM_DEADCHAR & WM_SYSDEADCHAR does not > > > prevent us from removing WM_DEADCHAR & WM_SYSDEADCHAR. To me removing them > > > sounds a bit safer than not removing them. > > > > What do you mean by removing WM_DEADCHAR? > > If you meant to remove WM_DEADCHAR in the message queue, the current change > does > > already do so. > > Ah, sorry I misunderstood this code. > Can you leave a comment about what messages will be removed in practice? IIUC, > they are: > - WM_CHAR (0x0102) > - WM_DEADCHAR (0x0103) > - WM_SYSKEYDOWN (0x0104) > - WM_SYSKEYUP (0x0105) > - WM_SYSCHAR (0x0106) > BTW, is it intentional that we do not remove WM_SYSDEADCHAR (0x0107)? Done. https://codereview.chromium.org/1267483003/diff/40001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1267483003/diff/40001/base/message_loop/messa... base/message_loop/message_pump_win.cc:404: // InputMethodWin, so that the WM_KEYDOWN & WM_CHAR can be combined for key On 2015/08/04 17:06:44, James Su wrote: > WM_KEYDOWN/WM_KEYUP Done. https://codereview.chromium.org/1267483003/diff/40001/ui/base/ime/input_metho... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/40001/ui/base/ime/input_metho... ui/base/ime/input_method_win.cc:111: PM_REMOVE)) { On 2015/08/04 17:06:44, James Su wrote: > There are WM_SYSKEYDOWN and WM_SYSKEYUP between WM_CHAR and WM_SYSCHAR. > And how about MW_SYSDEADCHAR? > > I think you should better filter WM_CHAR (102) / WM_DEADCHAR (103) and > WM_SYSCHAR (106) / WM_SYSDEADCHAR (107) separately. > > And you probably can simply discard WM_DEADCHAR and WM_SYSDEADCHAR events for > now. Done. https://codereview.chromium.org/1267483003/diff/40001/ui/base/ime/remote_inpu... File ui/base/ime/remote_input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/40001/ui/base/ime/remote_inpu... ui/base/ime/remote_input_method_win.cc:210: pending_char_events_.clear(); On 2015/08/04 17:06:44, James Su wrote: > this cache should always be cleared for WM_KEYDOWN/WM_KEYUP Done. https://codereview.chromium.org/1267483003/diff/40001/win8/metro_driver/metro... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/40001/win8/metro_driver/metro... win8/metro_driver/metro_driver_win7.cc:917: while (::PeekMessage(&char_msg, msg.hwnd, WM_CHAR, WM_SYSCHAR, On 2015/08/04 17:06:44, James Su wrote: > ditto > > And WM_KEYDOWN/WM_KEYUP won't generate WM_SYSCHAR/WM_SYSDEADCHAR. Done.
Guys, can you please take another look at the latest patchset? Btw, I've verified it on win7 & win8 desktop mode. But I failed to verify it on win8 metro mode, because the latest ToT Chrome fails to relaunch into metro mode. I believe my cl doesn't cause the issue because my cl only affects key event handling, but fail-to-relaunch-into-metro issue doesn't involve key events at all.
Probably this CL would be adequate for the goal of this CL, but I'm still worried about compatibility issues that this CL may cause. https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... File win8/metro_driver/metro_driver_win7.cc (left): https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... win8/metro_driver/metro_driver_win7.cc:930: case WM_UNICHAR: { I supposed removing WM_UNICHAR support does not affect the goal of this CL, but may still have some non-trivial effect. If we intentionally do this, it should be commented in the CL description. https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... win8/metro_driver/metro_driver_win7.cc:466: ::DispatchMessage(&msg); Can you leave a comment about why ::TranslateMessage isn't called here.
I've verified on win8 successfully. No regressions found for metro mode with dead keys & virtual keyboard. https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... File win8/metro_driver/metro_driver_win7.cc (left): https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... win8/metro_driver/metro_driver_win7.cc:930: case WM_UNICHAR: { On 2015/08/05 21:23:36, yukawa wrote: > I supposed removing WM_UNICHAR support does not affect the goal of this CL, but > may still have some non-trivial effect. If we intentionally do this, it should > be commented in the CL description. Removing WM_UNICHAR here doesn't change the existing behavior. Please note that RemoteInputMethodWin::DispatchKeyEvent() doesn't support WM_UNICHAR. https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ime/remote... So I think the change here is pretty safe. https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... win8/metro_driver/metro_driver_win7.cc:466: ::DispatchMessage(&msg); On 2015/08/05 21:23:36, yukawa wrote: > Can you leave a comment about why ::TranslateMessage isn't called here. Done.
https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... File win8/metro_driver/metro_driver_win7.cc (left): https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... win8/metro_driver/metro_driver_win7.cc:930: case WM_UNICHAR: { On 2015/08/06 01:35:51, Shu Chen wrote: > On 2015/08/05 21:23:36, yukawa wrote: > > I supposed removing WM_UNICHAR support does not affect the goal of this CL, > but > > may still have some non-trivial effect. If we intentionally do this, it > should > > be commented in the CL description. > > Removing WM_UNICHAR here doesn't change the existing behavior. > Please note that RemoteInputMethodWin::DispatchKeyEvent() doesn't support > WM_UNICHAR. Is there any chance that unhandled WM_UNICHAR was translated into multiple WM_CHAR in DefWindowProc?
On 2015/08/06 01:45:51, yukawa wrote: > https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... > File win8/metro_driver/metro_driver_win7.cc (left): > > https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... > win8/metro_driver/metro_driver_win7.cc:930: case WM_UNICHAR: { > On 2015/08/06 01:35:51, Shu Chen wrote: > > On 2015/08/05 21:23:36, yukawa wrote: > > > I supposed removing WM_UNICHAR support does not affect the goal of this CL, > > but > > > may still have some non-trivial effect. If we intentionally do this, it > > should > > > be commented in the CL description. > > > > Removing WM_UNICHAR here doesn't change the existing behavior. > > Please note that RemoteInputMethodWin::DispatchKeyEvent() doesn't support > > WM_UNICHAR. > > Is there any chance that unhandled WM_UNICHAR was translated into multiple > WM_CHAR in DefWindowProc? Ah, sorry I didn't realize WM_UNICHAR can be translated into WM_CHAR. Thanks for catching it. Looks like I have to distinguish WM_CHAR with and without WM_KEYDOWN. Therefore, introducing MetroViewerHostMsg_CharacterForNextKeyEvent is necessary. I am working on the change.
Yohei, my latest patchset includes the newly added MetroViewerHostMsg_CharacterForNextKeyEvent IPC and considered the case of WM_UNICHAR. Can you please take another look? Thanks
The CQ bit was checked by shuchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267483003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267483003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by shuchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267483003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267483003/200001
https://codereview.chromium.org/1267483003/diff/200001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/200001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:929: while (::PeekMessage(&char_msg, msg.hwnd, WM_UNICHAR, WM_UNICHAR, did you try any unichar use case? As far as I understand, a WM_UNICHAR may be generated by a character palette, thus there may be no corresponding key down/up event.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1267483003/diff/200001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/200001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:929: while (::PeekMessage(&char_msg, msg.hwnd, WM_UNICHAR, WM_UNICHAR, On 2015/08/07 04:55:55, James Su wrote: > did you try any unichar use case? As far as I understand, a WM_UNICHAR may be > generated by a character palette, thus there may be no corresponding key down/up > event. The change here is just for double insurance to make it safer. I found this: https://social.msdn.microsoft.com/forums/windowsdesktop/en-US/d455e846-d18b-4... Looks like WM_UNICHAR will never be generated by TranslateMessage(), so I've removed this change. Thanks
On 2015/08/07 06:02:50, Shu Chen wrote: > https://codereview.chromium.org/1267483003/diff/200001/win8/metro_driver/metr... > File win8/metro_driver/metro_driver_win7.cc (right): > > https://codereview.chromium.org/1267483003/diff/200001/win8/metro_driver/metr... > win8/metro_driver/metro_driver_win7.cc:929: while (::PeekMessage(&char_msg, > msg.hwnd, WM_UNICHAR, WM_UNICHAR, > On 2015/08/07 04:55:55, James Su wrote: > > did you try any unichar use case? As far as I understand, a WM_UNICHAR may be > > generated by a character palette, thus there may be no corresponding key > down/up > > event. > > The change here is just for double insurance to make it safer. > I found this: > https://social.msdn.microsoft.com/forums/windowsdesktop/en-US/d455e846-d18b-4... > Looks like WM_UNICHAR will never be generated by TranslateMessage(), so I've > removed this change. > Thanks LGTM.
https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... File win8/metro_driver/metro_driver_win7.cc (left): https://codereview.chromium.org/1267483003/diff/60001/win8/metro_driver/metro... win8/metro_driver/metro_driver_win7.cc:930: case WM_UNICHAR: { On 2015/08/06 01:45:51, yukawa wrote: > On 2015/08/06 01:35:51, Shu Chen wrote: > > On 2015/08/05 21:23:36, yukawa wrote: > > > I supposed removing WM_UNICHAR support does not affect the goal of this CL, > > but > > > may still have some non-trivial effect. If we intentionally do this, it > > should > > > be commented in the CL description. > > > > Removing WM_UNICHAR here doesn't change the existing behavior. > > Please note that RemoteInputMethodWin::DispatchKeyEvent() doesn't support > > WM_UNICHAR. > > Is there any chance that unhandled WM_UNICHAR was translated into multiple > WM_CHAR in DefWindowProc? I've removed the WM_DEADCHAR & WM_UNICHAR here, and only handle WM_CHAR. Because it is useless to do Invoke() for WM_DEADCHAR & WM_UNICHAR. As for WM_UNICHAR case, accordingly to MSDN doc: https://msdn.microsoft.com/en-us/library/windows/desktop/ms646288(v=vs.85).aspx > If wParam is not UNICODE_NOCHAR, return FALSE. The Unicode DefWindowProc posts a WM_CHAR message with the same parameters and the ANSI DefWindowProc function posts either one or two WM_CHAR messages with the corresponding ANSI character(s). So, application doesn't need to handle WM_UNICHAR, because DefWindowProc() can take care of things nicely.
Pinging... Yohei, mind taking another look? Thanks
On 2015/08/10 23:22:51, Shu Chen wrote:
> Pinging...
>
> Yohei, mind taking another look? Thanks
I think we can go ahead with this CL for prototyping. Do you want me to review
this for production?
My remaining concerns are:
1. We are going to heavily rely on PeekMessage to tweak message pump, which I
think is still non-trivial.
2. This CL may introduce non-zero performance drawback regardless of the
existence of Chrome-IME, which may make performance folks unhappy.
3. IIRC, TranslateMessage may internally call IME's ImeToAsciiEx, which may also
trigger some nested tasks before returning from it. Imagine the following
scenario.
[A: IME is deactivated. w/o this CL]
I. WM_KEYDOWN is picked up from the message queue.
II. TranslateMessage is called from the message pump.
III. WM_KEYDOWN is dispatched to Chrome.
IV. Return from WM_KEYDOWN handler.
V. WM_CHAR is picked up from the message queue.
VI. WM_CHAR is dispatched to Chrome.
VII. Return from WM_CHAR handler.
[B: IME is deactivated. w/ this CL]
I. WM_KEYDOWN is picked up from the message queue.
II. WM_KEYDOWN is dispatched to Chrome.
III. TranslateMessage is called from the message pump.
IV. Return from WM_KEYDOWN handler.
[C: IME is activated. w/o this CL]
I. WM_KEYDOWN is picked up from the message queue.
II. TranslateMessage is called from the message pump, which allows IMEs to
do some non-trivial nested tasks before returning from it and to enqueue some
additional WM_IME_*** messages to the message queue.
III. WM_KEYDOWN is dispatched to Chrome.
IV. Return from WM_KEYDOWN handler.
V. WM_IME_*** is picked up from the message queue.
VI. WM_IME_*** is dispatched to Chrome.
VII. Return from WM_IME_*** handler.
[D: IME is activated. w/ this CL]
I. WM_KEYDOWN is picked up from the message queue.
II. WM_KEYDOWN is dispatched to Chrome.
III. TranslateMessage is called from the message pump, which allows IMEs to
do some non-trivial nested tasks before returning from it and to enqueue some
additional WM_IME_*** messages to the message queue.
IV. Return from WM_KEYDOWN handler.
V. WM_IME_*** is picked up from the message queue.
VI. WM_IME_*** is dispatched to Chrome.
VII. Return from WM_IME_*** handler.
Note that things that have been done between C:III and C:IV will be mapped into
D:II and D:IV, which can slightly change the order of existing event handlings.
I don't say this CL violates some basic rules in Windows, but there still be a
chance that such a change introduce some unexpected behavior change.
For production I think we should have a feature flag so that we can revert the
new behavior back to the previous one when something unexpected is found.
On 2015/08/11 00:01:43, yukawa wrote: > On 2015/08/10 23:22:51, Shu Chen wrote: > > Pinging... > > > > Yohei, mind taking another look? Thanks > > I think we can go ahead with this CL for prototyping. Do you want me to review > this for production? Yes. I'm now targeting to commit this for production. > > My remaining concerns are: > 1. We are going to heavily rely on PeekMessage to tweak message pump, which I > think is still non-trivial. > 2. This CL may introduce non-zero performance drawback regardless of the > existence of Chrome-IME, which may make performance folks unhappy. I thought this cl won't cause perf regression. Can you please elaborate the potential perf issues? > 3. IIRC, TranslateMessage may internally call IME's ImeToAsciiEx, which may also > trigger some nested tasks before returning from it. Imagine the following > scenario. > > [A: IME is deactivated. w/o this CL] > I. WM_KEYDOWN is picked up from the message queue. > II. TranslateMessage is called from the message pump. > III. WM_KEYDOWN is dispatched to Chrome. > IV. Return from WM_KEYDOWN handler. > V. WM_CHAR is picked up from the message queue. > VI. WM_CHAR is dispatched to Chrome. > VII. Return from WM_CHAR handler. > > [B: IME is deactivated. w/ this CL] > I. WM_KEYDOWN is picked up from the message queue. > II. WM_KEYDOWN is dispatched to Chrome. > III. TranslateMessage is called from the message pump. > IV. Return from WM_KEYDOWN handler. > > [C: IME is activated. w/o this CL] > I. WM_KEYDOWN is picked up from the message queue. > II. TranslateMessage is called from the message pump, which allows IMEs to > do some non-trivial nested tasks before returning from it and to enqueue some > additional WM_IME_*** messages to the message queue. > III. WM_KEYDOWN is dispatched to Chrome. > IV. Return from WM_KEYDOWN handler. > V. WM_IME_*** is picked up from the message queue. > VI. WM_IME_*** is dispatched to Chrome. > VII. Return from WM_IME_*** handler. > > [D: IME is activated. w/ this CL] > I. WM_KEYDOWN is picked up from the message queue. > II. WM_KEYDOWN is dispatched to Chrome. > III. TranslateMessage is called from the message pump, which allows IMEs to > do some non-trivial nested tasks before returning from it and to enqueue some > additional WM_IME_*** messages to the message queue. > IV. Return from WM_KEYDOWN handler. > V. WM_IME_*** is picked up from the message queue. > VI. WM_IME_*** is dispatched to Chrome. > VII. Return from WM_IME_*** handler. > > Note that things that have been done between C:III and C:IV will be mapped into > D:II and D:IV, which can slightly change the order of existing event handlings. > I don't say this CL violates some basic rules in Windows, but there still be a > chance that such a change introduce some unexpected behavior change. For desktop, InputMethod is the first guy to handle key event in chrome. So "WM_KEYDOWN is dispatched to Chrome", can be divided into: - "WM_KEYDOWN is dispatched to HWNDMessageHandler" - "WM_KEYDOWN is dispatched to Chrome by IMF" For C:II - C:IV: - TranslateMessage is called from message pump... - WM_KEYDOWN is dispatched to HWNDMessageHandler. - WM_KEYDOWN is dispatched to Chrome by IMF. - Return from WM_KEYDOWN handler. For D:II - D:IV: - WM_KEYDOWN is dispatched to HWNDMessageHandler. - TranslateMessage is called from IMF... - WM_KEYDOWN is dispatched to Chrome by IMF. - Return from WM_KEYDOWN handler. So the only behavior that is changed by this cl is that the HWNDMessageHandler now handles key event before TranslateMessage(). I don't see much risk from that, as there isn't heavy logic of key event handling in HWNDMessageHandler. > > For production I think we should have a feature flag so that we can revert the > new behavior back to the previous one when something unexpected is found. Good idea. I will add a flag --enable-chrome-ime. WDYT? Or do you want it be a separated flag like --merge-key-char-events?
shuchen@chromium.org changed reviewers: + ananta@chromium.org, jln@chromium.org, thakis@chromium.org
+thakis@ for owner of: - base/message_loop/message_pump_win.cc - ui/aura/remote_window_tree_host_win.* +ananta@ for owner of: - win8/metro_driver/... +jln@ for owner of: - ui/metro_viewer/metro_viewer_messages.h Thanks!
On 2015/08/11 00:57:18, Shu Chen wrote: > > My remaining concerns are: > > 1. We are going to heavily rely on PeekMessage to tweak message pump, which I > > think is still non-trivial. > > 2. This CL may introduce non-zero performance drawback regardless of the > > existence of Chrome-IME, which may make performance folks unhappy. > I thought this cl won't cause perf regression. Can you please elaborate the > potential perf issues? I'm not sure if this is still valid, but I saw some commit messages that claimed PeekMessage is one of sources of jank. See http://crbug.com/440919, comment #8 and #9 in particular. > > 3. IIRC, TranslateMessage may internally call IME's ImeToAsciiEx, which may > also > > trigger some nested tasks before returning from it. Imagine the following > > scenario. > > > > [A: IME is deactivated. w/o this CL] > > I. WM_KEYDOWN is picked up from the message queue. > > II. TranslateMessage is called from the message pump. > > III. WM_KEYDOWN is dispatched to Chrome. > > IV. Return from WM_KEYDOWN handler. > > V. WM_CHAR is picked up from the message queue. > > VI. WM_CHAR is dispatched to Chrome. > > VII. Return from WM_CHAR handler. > > > > [B: IME is deactivated. w/ this CL] > > I. WM_KEYDOWN is picked up from the message queue. > > II. WM_KEYDOWN is dispatched to Chrome. > > III. TranslateMessage is called from the message pump. > > IV. Return from WM_KEYDOWN handler. > > > > [C: IME is activated. w/o this CL] > > I. WM_KEYDOWN is picked up from the message queue. > > II. TranslateMessage is called from the message pump, which allows IMEs > to > > do some non-trivial nested tasks before returning from it and to enqueue some > > additional WM_IME_*** messages to the message queue. > > III. WM_KEYDOWN is dispatched to Chrome. > > IV. Return from WM_KEYDOWN handler. > > V. WM_IME_*** is picked up from the message queue. > > VI. WM_IME_*** is dispatched to Chrome. > > VII. Return from WM_IME_*** handler. > > > > [D: IME is activated. w/ this CL] > > I. WM_KEYDOWN is picked up from the message queue. > > II. WM_KEYDOWN is dispatched to Chrome. > > III. TranslateMessage is called from the message pump, which allows IMEs > to > > do some non-trivial nested tasks before returning from it and to enqueue some > > additional WM_IME_*** messages to the message queue. > > IV. Return from WM_KEYDOWN handler. > > V. WM_IME_*** is picked up from the message queue. > > VI. WM_IME_*** is dispatched to Chrome. > > VII. Return from WM_IME_*** handler. > > > > Note that things that have been done between C:III and C:IV will be mapped > into > > D:II and D:IV, which can slightly change the order of existing event > handlings. > > I don't say this CL violates some basic rules in Windows, but there still be a > > chance that such a change introduce some unexpected behavior change. > For desktop, InputMethod is the first guy to handle key event in chrome. > So "WM_KEYDOWN is dispatched to Chrome", can be divided into: > - "WM_KEYDOWN is dispatched to HWNDMessageHandler" > - "WM_KEYDOWN is dispatched to Chrome by IMF" > For C:II - C:IV: > - TranslateMessage is called from message pump... > - WM_KEYDOWN is dispatched to HWNDMessageHandler. > - WM_KEYDOWN is dispatched to Chrome by IMF. > - Return from WM_KEYDOWN handler. > For D:II - D:IV: > - WM_KEYDOWN is dispatched to HWNDMessageHandler. > - TranslateMessage is called from IMF... > - WM_KEYDOWN is dispatched to Chrome by IMF. > - Return from WM_KEYDOWN handler. > So the only behavior that is changed by this cl is that the HWNDMessageHandler > now handles key event before TranslateMessage(). > I don't see much risk from that, as there isn't heavy logic of key event > handling in HWNDMessageHandler. Can we mention the actual behavior change in the commit message for future readers? > > For production I think we should have a feature flag so that we can revert the > > new behavior back to the previous one when something unexpected is found. > Good idea. I will add a flag --enable-chrome-ime. WDYT? > Or do you want it be a separated flag like --merge-key-char-events? To me --merge-key-char-events sounds more appropriate since it describes what we are going to change more precisely.
yukawa@chromium.org changed reviewers: + robliao@chromium.org
+robliao@ in case he might want to say something about PeekMessage.
https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... base/message_loop/message_pump_win.cc:403: // Don't call TranslateMessage() here but call TranslateMessage() in TranslateMessage+DispatchMessage is generally idiomatic Windows message pump code. Separating them generally means you really had no other way to accomplish what you want. A few questions: What keeps Chrome IME's from responding to the WM_KEYDOWN/UP and WM_CHAR in sequence? TranslateMessage just sends the WM_CHAR to the thread's message queue. Since we're directly dispatching the WM_KEYDOWN/WM_KEYUP messages without translations, will there be message ordering changes that might cause problems? Before, WM_*CHARs would be posted before we handle the message. Now they won't. Will this subsume all Windows IME functionality? And a comment: If we keep this, keep the braces for the if and move this comment to right before DispatchMessage. This comment directly addresses the absense of TranslateMessage and should immediately precede the DispatchMessage call.
On 2015/08/11 04:28:45, yukawa wrote: > On 2015/08/11 00:57:18, Shu Chen wrote: > > > My remaining concerns are: > > > 1. We are going to heavily rely on PeekMessage to tweak message pump, which > I > > > think is still non-trivial. > > > 2. This CL may introduce non-zero performance drawback regardless of the > > > existence of Chrome-IME, which may make performance folks unhappy. > > I thought this cl won't cause perf regression. Can you please elaborate the > > potential perf issues? > > I'm not sure if this is still valid, but I saw some commit messages that claimed > PeekMessage is one of sources of jank. See http://crbug.com/440919, comment #8 > and #9 in particular. I think the impact would be limited because PeekMessage() here is called only for key messages. And this cl make it only call TranslateMessage() only for key messages (originally for all messages). robliao@, what's your opinion on this? > > > > 3. IIRC, TranslateMessage may internally call IME's ImeToAsciiEx, which may > > also > > > trigger some nested tasks before returning from it. Imagine the following > > > scenario. > > > > > > [A: IME is deactivated. w/o this CL] > > > I. WM_KEYDOWN is picked up from the message queue. > > > II. TranslateMessage is called from the message pump. > > > III. WM_KEYDOWN is dispatched to Chrome. > > > IV. Return from WM_KEYDOWN handler. > > > V. WM_CHAR is picked up from the message queue. > > > VI. WM_CHAR is dispatched to Chrome. > > > VII. Return from WM_CHAR handler. > > > > > > [B: IME is deactivated. w/ this CL] > > > I. WM_KEYDOWN is picked up from the message queue. > > > II. WM_KEYDOWN is dispatched to Chrome. > > > III. TranslateMessage is called from the message pump. > > > IV. Return from WM_KEYDOWN handler. > > > > > > [C: IME is activated. w/o this CL] > > > I. WM_KEYDOWN is picked up from the message queue. > > > II. TranslateMessage is called from the message pump, which allows IMEs > > to > > > do some non-trivial nested tasks before returning from it and to enqueue > some > > > additional WM_IME_*** messages to the message queue. > > > III. WM_KEYDOWN is dispatched to Chrome. > > > IV. Return from WM_KEYDOWN handler. > > > V. WM_IME_*** is picked up from the message queue. > > > VI. WM_IME_*** is dispatched to Chrome. > > > VII. Return from WM_IME_*** handler. > > > > > > [D: IME is activated. w/ this CL] > > > I. WM_KEYDOWN is picked up from the message queue. > > > II. WM_KEYDOWN is dispatched to Chrome. > > > III. TranslateMessage is called from the message pump, which allows IMEs > > to > > > do some non-trivial nested tasks before returning from it and to enqueue > some > > > additional WM_IME_*** messages to the message queue. > > > IV. Return from WM_KEYDOWN handler. > > > V. WM_IME_*** is picked up from the message queue. > > > VI. WM_IME_*** is dispatched to Chrome. > > > VII. Return from WM_IME_*** handler. > > > > > > Note that things that have been done between C:III and C:IV will be mapped > > into > > > D:II and D:IV, which can slightly change the order of existing event > > handlings. > > > I don't say this CL violates some basic rules in Windows, but there still be > a > > > chance that such a change introduce some unexpected behavior change. > > For desktop, InputMethod is the first guy to handle key event in chrome. > > So "WM_KEYDOWN is dispatched to Chrome", can be divided into: > > - "WM_KEYDOWN is dispatched to HWNDMessageHandler" > > - "WM_KEYDOWN is dispatched to Chrome by IMF" > > For C:II - C:IV: > > - TranslateMessage is called from message pump... > > - WM_KEYDOWN is dispatched to HWNDMessageHandler. > > - WM_KEYDOWN is dispatched to Chrome by IMF. > > - Return from WM_KEYDOWN handler. > > For D:II - D:IV: > > - WM_KEYDOWN is dispatched to HWNDMessageHandler. > > - TranslateMessage is called from IMF... > > - WM_KEYDOWN is dispatched to Chrome by IMF. > > - Return from WM_KEYDOWN handler. > > So the only behavior that is changed by this cl is that the HWNDMessageHandler > > now handles key event before TranslateMessage(). > > I don't see much risk from that, as there isn't heavy logic of key event > > handling in HWNDMessageHandler. > > Can we mention the actual behavior change in the commit message for future > readers? Done. > > > > For production I think we should have a feature flag so that we can revert > the > > > new behavior back to the previous one when something unexpected is found. > > Good idea. I will add a flag --enable-chrome-ime. WDYT? > > Or do you want it be a separated flag like --merge-key-char-events? > > To me --merge-key-char-events sounds more appropriate since it describes what we > are going to change more precisely. Will do.
https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... base/message_loop/message_pump_win.cc:403: // Don't call TranslateMessage() here but call TranslateMessage() in On 2015/08/11 18:39:27, robliao wrote: > TranslateMessage+DispatchMessage is generally idiomatic Windows message pump In MSDN for TranslateMessage, it says "If applications process virtual-key messages for some other purpose, they should not call TranslateMessage..." which means it is not rare to not call TranslateMessage in the message loop. > code. Separating them generally means you really had no other way to accomplish > what you want. > > A few questions: > What keeps Chrome IME's from responding to the WM_KEYDOWN/UP and WM_CHAR in > sequence? TranslateMessage just sends the WM_CHAR to the thread's message queue. Chrome IMEs will be implemented as an extension. Once it is activated, Chrome will forward all the key events to the extension. The extension can decide whether the key event is consumed. Let's see a concrete example when user pressed { Shift + 2 }: - WM_KEYDOWN(VKEY_2/SHIFT) happened. - Chrome forwards it to the extension, which is not interested. - WM_CHAR('@') happened. - Chrome forwards it to the extension, which is interested & consumed it. In this case, the IME is only interested in "@" key, which could be Shift+2 in US layout, or single quote key in UK layout. However, the WM_KEYDOWN message has already happened before IME can make decision of whether to consume the key. In a web page, according to W3C standard, if a key is consume by IME, "keydown" event with 229 "keycode" should be generated. Which means, when Chrome processes the WM_KEYDOWN, it needs to hold it until the it gets the result from WM_CHAR processing. And that is infeasible to implement, because if Chrome holds the WM_KEYDOWN, it doesn't know when to dispatch it if there is no WM_CHAR afterwards. This example demonstrates that combining WM_KEY* with WM_CHAR is necessary to support Chrome IMEs. > > Since we're directly dispatching the WM_KEYDOWN/WM_KEYUP messages without > translations, will there be message ordering changes that might cause problems? > Before, WM_*CHARs would be posted before we handle the message. Now they won't. IMF (e.g. InputMethodWin) is the first guy who handles the key events in Chrome. Well, before IMF, there is HWNDMessage. But it basically does nothing but forward the message. As I've described in the CL description, the order change may be less risky than we thought. > > Will this subsume all Windows IME functionality? Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. > > And a comment: > If we keep this, keep the braces for the if and move this comment to right > before DispatchMessage. This comment directly addresses the absense of > TranslateMessage and should immediately precede the DispatchMessage call. Done.
https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... base/message_loop/message_pump_win.cc:403: // Don't call TranslateMessage() here but call TranslateMessage() in On 2015/08/12 08:00:11, Shu Chen wrote: > On 2015/08/11 18:39:27, robliao wrote: > > TranslateMessage+DispatchMessage is generally idiomatic Windows message pump > In MSDN for TranslateMessage, it says "If applications process virtual-key > messages for some other purpose, they should not call TranslateMessage..." which > means it is not rare to not call TranslateMessage in the message loop. > > code. Separating them generally means you really had no other way to > accomplish > > what you want. > > > > A few questions: > > What keeps Chrome IME's from responding to the WM_KEYDOWN/UP and WM_CHAR in > > sequence? TranslateMessage just sends the WM_CHAR to the thread's message > queue. > Chrome IMEs will be implemented as an extension. Once it is activated, Chrome > will forward all the key events to the extension. The extension can decide > whether the key event is consumed. > Let's see a concrete example when user pressed { Shift + 2 }: > - WM_KEYDOWN(VKEY_2/SHIFT) happened. > - Chrome forwards it to the extension, which is not interested. > - WM_CHAR('@') happened. > - Chrome forwards it to the extension, which is interested & consumed it. > In this case, the IME is only interested in "@" key, which could be Shift+2 in > US layout, or single quote key in UK layout. > However, the WM_KEYDOWN message has already happened before IME can make > decision of whether to consume the key. > In a web page, according to W3C standard, if a key is consume by IME, "keydown" > event with 229 "keycode" should be generated. Which means, when Chrome processes > the WM_KEYDOWN, it needs to hold it until the it gets the result from WM_CHAR > processing. > And that is infeasible to implement, because if Chrome holds the WM_KEYDOWN, it > doesn't know when to dispatch it if there is no WM_CHAR afterwards. > This example demonstrates that combining WM_KEY* with WM_CHAR is necessary to > support Chrome IMEs. > > > > Since we're directly dispatching the WM_KEYDOWN/WM_KEYUP messages without > > translations, will there be message ordering changes that might cause > problems? > > Before, WM_*CHARs would be posted before we handle the message. Now they > won't. > IMF (e.g. InputMethodWin) is the first guy who handles the key events in Chrome. > Well, before IMF, there is HWNDMessage. But it basically does nothing but > forward the message. > As I've described in the CL description, the order change may be less risky than > we thought. > > > > Will this subsume all Windows IME functionality? > Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. > > > > And a comment: > > If we keep this, keep the braces for the if and move this comment to right > > before DispatchMessage. This comment directly addresses the absense of > > TranslateMessage and should immediately precede the DispatchMessage call. > Done. > which means it is not rare to not call TranslateMessage in the message loop. While MSDN mentions that you don't have to call TranslateMessage depending on your use case, omitting that call is pretty rare because you lose WM_CHAR and WM_SYSCHAR. http://blogs.msdn.com/b/oldnewthing/archive/2005/04/26/412116.aspx http://stackoverflow.com/questions/3152011/why-are-translatemessage-and-dispa... http://www.winprog.org/tutorial/message_loop.html Can the IME instead look ahead in the message loop via PeekMessage instead for the WM_CHARs when receiving a WM_KEYDOWN? This is already done with the TranslateMessage+PeekMessage combo in InputMethodWin. You'd simply move the TranslateMessage back to here. > Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. What if the user actually wants to use the Windows native IME? IIRC, the Windows Native IME kepts track of usage patterns to tailor suggestions to the user.
On 2015/08/12 18:04:12, robliao wrote: > https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... > File base/message_loop/message_pump_win.cc (right): > > https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... > base/message_loop/message_pump_win.cc:403: // Don't call TranslateMessage() here > but call TranslateMessage() in > On 2015/08/12 08:00:11, Shu Chen wrote: > > On 2015/08/11 18:39:27, robliao wrote: > > > TranslateMessage+DispatchMessage is generally idiomatic Windows message pump > > In MSDN for TranslateMessage, it says "If applications process virtual-key > > messages for some other purpose, they should not call TranslateMessage..." > which > > means it is not rare to not call TranslateMessage in the message loop. > > > code. Separating them generally means you really had no other way to > > accomplish > > > what you want. > > > > > > A few questions: > > > What keeps Chrome IME's from responding to the WM_KEYDOWN/UP and WM_CHAR in > > > sequence? TranslateMessage just sends the WM_CHAR to the thread's message > > queue. > > Chrome IMEs will be implemented as an extension. Once it is activated, Chrome > > will forward all the key events to the extension. The extension can decide > > whether the key event is consumed. > > Let's see a concrete example when user pressed { Shift + 2 }: > > - WM_KEYDOWN(VKEY_2/SHIFT) happened. > > - Chrome forwards it to the extension, which is not interested. > > - WM_CHAR('@') happened. > > - Chrome forwards it to the extension, which is interested & consumed it. > > In this case, the IME is only interested in "@" key, which could be Shift+2 in > > US layout, or single quote key in UK layout. > > However, the WM_KEYDOWN message has already happened before IME can make > > decision of whether to consume the key. > > In a web page, according to W3C standard, if a key is consume by IME, > "keydown" > > event with 229 "keycode" should be generated. Which means, when Chrome > processes > > the WM_KEYDOWN, it needs to hold it until the it gets the result from WM_CHAR > > processing. > > And that is infeasible to implement, because if Chrome holds the WM_KEYDOWN, > it > > doesn't know when to dispatch it if there is no WM_CHAR afterwards. > > This example demonstrates that combining WM_KEY* with WM_CHAR is necessary to > > support Chrome IMEs. > > > > > > Since we're directly dispatching the WM_KEYDOWN/WM_KEYUP messages without > > > translations, will there be message ordering changes that might cause > > problems? > > > Before, WM_*CHARs would be posted before we handle the message. Now they > > won't. > > IMF (e.g. InputMethodWin) is the first guy who handles the key events in > Chrome. > > Well, before IMF, there is HWNDMessage. But it basically does nothing but > > forward the message. > > As I've described in the CL description, the order change may be less risky > than > > we thought. > > > > > > Will this subsume all Windows IME functionality? > > Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. > > > > > > And a comment: > > > If we keep this, keep the braces for the if and move this comment to right > > > before DispatchMessage. This comment directly addresses the absense of > > > TranslateMessage and should immediately precede the DispatchMessage call. > > Done. > > > which means it is not rare to not call TranslateMessage in the message loop. > > While MSDN mentions that you don't have to call TranslateMessage depending on > your use case, omitting that call is pretty rare because you lose WM_CHAR and > WM_SYSCHAR. > http://blogs.msdn.com/b/oldnewthing/archive/2005/04/26/412116.aspx > > http://stackoverflow.com/questions/3152011/why-are-translatemessage-and-dispa... > > http://www.winprog.org/tutorial/message_loop.html > > Can the IME instead look ahead in the message loop via PeekMessage instead for > the WM_CHARs when receiving a WM_KEYDOWN? This is already done with the > TranslateMessage+PeekMessage combo in InputMethodWin. You'd simply move the > TranslateMessage back to here. I think this approach is feasible and has smaller impact. Let's follow this approach. > > > Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. > What if the user actually wants to use the Windows native IME? IIRC, the Windows > Native IME kepts track of usage patterns to tailor suggestions to the user. Native IMEs have higher priority, as a native IME will always get the key events first.
https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... base/message_loop/message_pump_win.cc:403: // Don't call TranslateMessage() here but call TranslateMessage() in On 2015/08/12 18:04:12, robliao wrote: > On 2015/08/12 08:00:11, Shu Chen wrote: > > On 2015/08/11 18:39:27, robliao wrote: > > > TranslateMessage+DispatchMessage is generally idiomatic Windows message pump > > In MSDN for TranslateMessage, it says "If applications process virtual-key > > messages for some other purpose, they should not call TranslateMessage..." > which > > means it is not rare to not call TranslateMessage in the message loop. > > > code. Separating them generally means you really had no other way to > > accomplish > > > what you want. > > > > > > A few questions: > > > What keeps Chrome IME's from responding to the WM_KEYDOWN/UP and WM_CHAR in > > > sequence? TranslateMessage just sends the WM_CHAR to the thread's message > > queue. > > Chrome IMEs will be implemented as an extension. Once it is activated, Chrome > > will forward all the key events to the extension. The extension can decide > > whether the key event is consumed. > > Let's see a concrete example when user pressed { Shift + 2 }: > > - WM_KEYDOWN(VKEY_2/SHIFT) happened. > > - Chrome forwards it to the extension, which is not interested. > > - WM_CHAR('@') happened. > > - Chrome forwards it to the extension, which is interested & consumed it. > > In this case, the IME is only interested in "@" key, which could be Shift+2 in > > US layout, or single quote key in UK layout. > > However, the WM_KEYDOWN message has already happened before IME can make > > decision of whether to consume the key. > > In a web page, according to W3C standard, if a key is consume by IME, > "keydown" > > event with 229 "keycode" should be generated. Which means, when Chrome > processes > > the WM_KEYDOWN, it needs to hold it until the it gets the result from WM_CHAR > > processing. > > And that is infeasible to implement, because if Chrome holds the WM_KEYDOWN, > it > > doesn't know when to dispatch it if there is no WM_CHAR afterwards. > > This example demonstrates that combining WM_KEY* with WM_CHAR is necessary to > > support Chrome IMEs. > > > > > > Since we're directly dispatching the WM_KEYDOWN/WM_KEYUP messages without > > > translations, will there be message ordering changes that might cause > > problems? > > > Before, WM_*CHARs would be posted before we handle the message. Now they > > won't. > > IMF (e.g. InputMethodWin) is the first guy who handles the key events in > Chrome. > > Well, before IMF, there is HWNDMessage. But it basically does nothing but > > forward the message. > > As I've described in the CL description, the order change may be less risky > than > > we thought. > > > > > > Will this subsume all Windows IME functionality? > > Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. > > > > > > And a comment: > > > If we keep this, keep the braces for the if and move this comment to right > > > before DispatchMessage. This comment directly addresses the absense of > > > TranslateMessage and should immediately precede the DispatchMessage call. > > Done. > > > which means it is not rare to not call TranslateMessage in the message loop. > > While MSDN mentions that you don't have to call TranslateMessage depending on > your use case, omitting that call is pretty rare because you lose WM_CHAR and > WM_SYSCHAR. > http://blogs.msdn.com/b/oldnewthing/archive/2005/04/26/412116.aspx > > http://stackoverflow.com/questions/3152011/why-are-translatemessage-and-dispa... > > http://www.winprog.org/tutorial/message_loop.html > > Can the IME instead look ahead in the message loop via PeekMessage instead for > the WM_CHARs when receiving a WM_KEYDOWN? This is already done with the > TranslateMessage+PeekMessage combo in InputMethodWin. You'd simply move the > TranslateMessage back to here. Good idea. Done. > > > Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. > What if the user actually wants to use the Windows native IME? IIRC, the Windows > Native IME kepts track of usage patterns to tailor suggestions to the user. Native IME receives the key event before Chrome receives it. So when pressed a key with a CJK IME activated, WM_KEYDOWN with key_code==229 will arrive at Chrome. Calling TranslateMessage() on that message will result in WM_IME_* messages. Chrome IMEs can simply ignore the key event with key_code==229, so that OS level IME can work as normal.
On 2015/08/13 02:09:26, Shu Chen wrote: > https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... > File base/message_loop/message_pump_win.cc (right): > > https://codereview.chromium.org/1267483003/diff/220001/base/message_loop/mess... > base/message_loop/message_pump_win.cc:403: // Don't call TranslateMessage() here > but call TranslateMessage() in > On 2015/08/12 18:04:12, robliao wrote: > > On 2015/08/12 08:00:11, Shu Chen wrote: > > > On 2015/08/11 18:39:27, robliao wrote: > > > > TranslateMessage+DispatchMessage is generally idiomatic Windows message > pump > > > In MSDN for TranslateMessage, it says "If applications process virtual-key > > > messages for some other purpose, they should not call TranslateMessage..." > > which > > > means it is not rare to not call TranslateMessage in the message loop. > > > > code. Separating them generally means you really had no other way to > > > accomplish > > > > what you want. > > > > > > > > A few questions: > > > > What keeps Chrome IME's from responding to the WM_KEYDOWN/UP and WM_CHAR > in > > > > sequence? TranslateMessage just sends the WM_CHAR to the thread's message > > > queue. > > > Chrome IMEs will be implemented as an extension. Once it is activated, > Chrome > > > will forward all the key events to the extension. The extension can decide > > > whether the key event is consumed. > > > Let's see a concrete example when user pressed { Shift + 2 }: > > > - WM_KEYDOWN(VKEY_2/SHIFT) happened. > > > - Chrome forwards it to the extension, which is not interested. > > > - WM_CHAR('@') happened. > > > - Chrome forwards it to the extension, which is interested & consumed it. > > > In this case, the IME is only interested in "@" key, which could be Shift+2 > in > > > US layout, or single quote key in UK layout. > > > However, the WM_KEYDOWN message has already happened before IME can make > > > decision of whether to consume the key. > > > In a web page, according to W3C standard, if a key is consume by IME, > > "keydown" > > > event with 229 "keycode" should be generated. Which means, when Chrome > > processes > > > the WM_KEYDOWN, it needs to hold it until the it gets the result from > WM_CHAR > > > processing. > > > And that is infeasible to implement, because if Chrome holds the WM_KEYDOWN, > > it > > > doesn't know when to dispatch it if there is no WM_CHAR afterwards. > > > This example demonstrates that combining WM_KEY* with WM_CHAR is necessary > to > > > support Chrome IMEs. > > > > > > > > Since we're directly dispatching the WM_KEYDOWN/WM_KEYUP messages without > > > > translations, will there be message ordering changes that might cause > > > problems? > > > > Before, WM_*CHARs would be posted before we handle the message. Now they > > > won't. > > > IMF (e.g. InputMethodWin) is the first guy who handles the key events in > > Chrome. > > > Well, before IMF, there is HWNDMessage. But it basically does nothing but > > > forward the message. > > > As I've described in the CL description, the order change may be less risky > > than > > > we thought. > > > > > > > > Will this subsume all Windows IME functionality? > > > Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. > > > > > > > > And a comment: > > > > If we keep this, keep the braces for the if and move this comment to right > > > > before DispatchMessage. This comment directly addresses the absense of > > > > TranslateMessage and should immediately precede the DispatchMessage call. > > > Done. > > > > > which means it is not rare to not call TranslateMessage in the message > loop. > > > > While MSDN mentions that you don't have to call TranslateMessage depending on > > your use case, omitting that call is pretty rare because you lose WM_CHAR and > > WM_SYSCHAR. > > http://blogs.msdn.com/b/oldnewthing/archive/2005/04/26/412116.aspx > > > > > http://stackoverflow.com/questions/3152011/why-are-translatemessage-and-dispa... > > > > http://www.winprog.org/tutorial/message_loop.html > > > > Can the IME instead look ahead in the message loop via PeekMessage instead for > > the WM_CHARs when receiving a WM_KEYDOWN? This is already done with the > > TranslateMessage+PeekMessage combo in InputMethodWin. You'd simply move the > > TranslateMessage back to here. > Good idea. Done. > > > > > > Yes, I've verified the Windows native IMEs in win7/win8 & win8 metro. > > What if the user actually wants to use the Windows native IME? IIRC, the > Windows > > Native IME kepts track of usage patterns to tailor suggestions to the user. > Native IME receives the key event before Chrome receives it. > So when pressed a key with a CJK IME activated, WM_KEYDOWN with key_code==229 > will arrive at Chrome. Calling TranslateMessage() on that message will result in > WM_IME_* messages. > Chrome IMEs can simply ignore the key event with key_code==229, so that OS level > IME can work as normal. Windows code is looking generally good. Just a few remaining questions.
Thanks robliao@. I thought I've addressed all the questions, can you please highlight the remaining questions?
Ah, sorry about that. Code review didn't send out my comments! https://codereview.chromium.org/1267483003/diff/260001/ui/base/ime/input_meth... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/260001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:108: // Peek & remove the following messages in the message queue. This comment isn't particularly necessary because we can gather we're doing this in the following code. Instead discuss the reasons for the code: Why are we doing this? - Your comment about WM_KEY* and WM_CHAR messages is a lead in to that. Talk about the IME support. - Why is it okay to throw away the WM_*DEADCHAR messages? It is safe to do so? https://codereview.chromium.org/1267483003/diff/260001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (left): https://codereview.chromium.org/1267483003/diff/260001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:931: mswr::ComPtr<winui::Core::ICharacterReceivedEventArgs> event_args; Have we confirmed no one sends WM_UNICHAR to us? https://codereview.chromium.org/1267483003/diff/260001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/260001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:917: // Peek & remove the following messages in the message queue. Similarly, this is clear from the code below. Instead, address why we're doing this and why it's safe it ignore WM_DEADCHAR.
https://codereview.chromium.org/1267483003/diff/260001/ui/base/ime/input_meth... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/260001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:108: // Peek & remove the following messages in the message queue. On 2015/08/14 16:42:34, robliao wrote: > This comment isn't particularly necessary because we can gather we're doing this > in the following code. Instead discuss the reasons for the code: > > Why are we doing this? > - Your comment about WM_KEY* and WM_CHAR messages is a lead in to that. Talk > about the IME support. Done. > > - Why is it okay to throw away the WM_*DEADCHAR messages? It is safe to do so? Chrome is not interested in WM_*DEADCHAR messages and never process them. Please refer to yukawa@'s comment in patchset #1. https://codereview.chromium.org/1267483003/diff/260001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (left): https://codereview.chromium.org/1267483003/diff/260001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:931: mswr::ComPtr<winui::Core::ICharacterReceivedEventArgs> event_args; On 2015/08/14 16:42:34, robliao wrote: > Have we confirmed no one sends WM_UNICHAR to us? Some apps may send WM_UNICHAR to us. However, Windows API framework handles it very well and Chrome doesn't need to handle it. Please refer to https://msdn.microsoft.com/en-us/library/windows/desktop/ms646288(v=vs.85).aspx, the DefWindowProc() call will transfer the WM_UNICHAR message into WM_CHAR messages. In facts, the existing code below: "character_received_handler_->Invoke()" on WM_UNICHAR has potential issues. Please refer to https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/remote_win..., the uint32 char is casted to base::char16, which is wrong. https://codereview.chromium.org/1267483003/diff/260001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/260001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:917: // Peek & remove the following messages in the message queue. On 2015/08/14 16:42:34, robliao wrote: > Similarly, this is clear from the code below. Instead, address why we're doing Done. > this and why it's safe it ignore WM_DEADCHAR. Chrome is not interested in WM_*DEADCHAR messages and never process them. Please refer to yukawa@'s comment in patchset #1.
Windows Code lgtm. https://codereview.chromium.org/1267483003/diff/260001/ui/base/ime/input_meth... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/260001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:108: // Peek & remove the following messages in the message queue. On 2015/08/17 04:09:21, Shu Chen wrote: > On 2015/08/14 16:42:34, robliao wrote: > > This comment isn't particularly necessary because we can gather we're doing > this > > in the following code. Instead discuss the reasons for the code: > > > > Why are we doing this? > > - Your comment about WM_KEY* and WM_CHAR messages is a lead in to that. Talk > > about the IME support. > Done. > > > > - Why is it okay to throw away the WM_*DEADCHAR messages? It is safe to do so? > Chrome is not interested in WM_*DEADCHAR messages and never process them. > Please refer to yukawa@'s comment in patchset #1. Add these comments to this comment and remove the comment portions that restate what the code does below. https://codereview.chromium.org/1267483003/diff/280001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/280001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:917: // Peek & remove the following messages in the message queue. Similarly, remove this comment section since it restates what the code does below. Keep the portion that explains why we're doing this.
ui/metro_viewer/metro_viewer_messages.h lgtm, but try to add some early validation of what is received via IPC (That is true besides this CL).
ui/metro_viewer/metro_viewer_messages.h lgtm, but try to add some early validation of what is received via IPC (That is true besides this CL).
(err, forgot to send the comments) https://chromiumcodereview.appspot.com/1267483003/diff/280001/ui/aura/remote_... File ui/aura/remote_window_tree_host_win.cc (right): https://chromiumcodereview.appspot.com/1267483003/diff/280001/ui/aura/remote_... ui/aura/remote_window_tree_host_win.cc:378: ui::RemoteInputMethodPrivateWin* remote_input_method_private = It's unfortunate that none of the IPC parameters are validated on-site (in this file) before being passes to DispatchKeyboardMessage(). Here it looks like most of them are not even used. Could you check that they are passed as (for instance) 0 and add a comment that if they need to get used, they should be validated carefully? https://chromiumcodereview.appspot.com/1267483003/diff/280001/win8/metro_driv... File win8/metro_driver/chrome_app_view_ash.cc (right): https://chromiumcodereview.appspot.com/1267483003/diff/280001/win8/metro_driv... win8/metro_driver/chrome_app_view_ash.cc:1272: char_code, status.RepeatCount, status.ScanCode, Most of these parameters don't seem to be used. Could you force sending them as 0 so that the other side can validate them as being 0?
On 2015/08/17 21:35:07, jln (slow on Chromium) wrote: > (err, forgot to send the comments) > > https://chromiumcodereview.appspot.com/1267483003/diff/280001/ui/aura/remote_... > File ui/aura/remote_window_tree_host_win.cc (right): > > https://chromiumcodereview.appspot.com/1267483003/diff/280001/ui/aura/remote_... > ui/aura/remote_window_tree_host_win.cc:378: ui::RemoteInputMethodPrivateWin* > remote_input_method_private = > It's unfortunate that none of the IPC parameters are validated on-site (in this > file) before being passes to DispatchKeyboardMessage(). > > Here it looks like most of them are not even used. Could you check that they are > passed as (for instance) 0 and add a comment that if they need to get used, they > should be validated carefully? > > https://chromiumcodereview.appspot.com/1267483003/diff/280001/win8/metro_driv... > File win8/metro_driver/chrome_app_view_ash.cc (right): > > https://chromiumcodereview.appspot.com/1267483003/diff/280001/win8/metro_driv... > win8/metro_driver/chrome_app_view_ash.cc:1272: char_code, status.RepeatCount, > status.ScanCode, > Most of these parameters don't seem to be used. Could you force sending them as > 0 so that the other side can validate them as being 0? Oh yeah, and the changelist description needs to be updated.
https://codereview.chromium.org/1267483003/diff/260001/ui/base/ime/input_meth... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/260001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:108: // Peek & remove the following messages in the message queue. On 2015/08/17 16:42:35, robliao wrote: > On 2015/08/17 04:09:21, Shu Chen wrote: > > On 2015/08/14 16:42:34, robliao wrote: > > > This comment isn't particularly necessary because we can gather we're doing > > this > > > in the following code. Instead discuss the reasons for the code: > > > > > > Why are we doing this? > > > - Your comment about WM_KEY* and WM_CHAR messages is a lead in to that. Talk > > > about the IME support. > > Done. > > > > > > - Why is it okay to throw away the WM_*DEADCHAR messages? It is safe to do > so? > > Chrome is not interested in WM_*DEADCHAR messages and never process them. > > Please refer to yukawa@'s comment in patchset #1. > > Add these comments to this comment and remove the comment portions that restate > what the code does below. Done. https://codereview.chromium.org/1267483003/diff/280001/ui/aura/remote_window_... File ui/aura/remote_window_tree_host_win.cc (right): https://codereview.chromium.org/1267483003/diff/280001/ui/aura/remote_window_... ui/aura/remote_window_tree_host_win.cc:378: ui::RemoteInputMethodPrivateWin* remote_input_method_private = On 2015/08/17 21:35:07, jln (slow on Chromium) wrote: > It's unfortunate that none of the IPC parameters are validated on-site (in this > file) before being passes to DispatchKeyboardMessage(). > > Here it looks like most of them are not even used. Could you check that they are > passed as (for instance) 0 and add a comment that if they need to get used, they > should be validated carefully? Done. I've removed the unused parameters. https://codereview.chromium.org/1267483003/diff/280001/win8/metro_driver/chro... File win8/metro_driver/chrome_app_view_ash.cc (right): https://codereview.chromium.org/1267483003/diff/280001/win8/metro_driver/chro... win8/metro_driver/chrome_app_view_ash.cc:1272: char_code, status.RepeatCount, status.ScanCode, On 2015/08/17 21:35:07, jln (slow on Chromium) wrote: > Most of these parameters don't seem to be used. Could you force sending them as > 0 so that the other side can validate them as being 0? Done. https://codereview.chromium.org/1267483003/diff/280001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/280001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:917: // Peek & remove the following messages in the message queue. On 2015/08/17 16:42:35, robliao wrote: > Similarly, remove this comment section since it restates what the code does > below. Keep the portion that explains why we're doing this. Done.
Pinging... yukawa@/thakis@/ananta@, mind taking a look? Thanks
On 2015/08/19 00:40:02, Shu Chen wrote: > Pinging... > > yukawa@/thakis@/ananta@, mind taking a look? Thanks One quick question: if you will add --merge-key-char-events after this CL, how do we ask users to check if any issue that occurs after this CL is relate to this or not?
On 2015/08/19 04:41:19, yukawa wrote: > On 2015/08/19 00:40:02, Shu Chen wrote: > > Pinging... > > > > yukawa@/thakis@/ananta@, mind taking a look? Thanks > > One quick question: if you will add --merge-key-char-events after this CL, how > do we ask users to check if any issue that occurs after this CL is relate to > this or not? Ah, sorry. I forgot to add the flag. Will do it soon.
Hi Yohei, I've added the disable-merge-key-char-events flag. Can you please take another look?
https://codereview.chromium.org/1267483003/diff/320001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1267483003/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:2134: IDS_FLAGS_DISABLE_MERGE_KEY_CHAR_EVENTS_NAME, If ENABLE_DISABLE_VALUE_TYPE works, can we rename this as IDS_FLAGS_MERGE_KEY_CHAR_EVENTS_NAME? https://codereview.chromium.org/1267483003/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:2137: SINGLE_VALUE_TYPE(switches::kDisableMergeKeyCharEvents)}, I'm not familiar with this, but would it be possible to use ENABLE_DISABLE_VALUE_TYPE? https://codereview.chromium.org/1267483003/diff/320001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/320001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:468: ::TranslateMessage(&msg); Would it be possible to preserve the current behavior when merge-key-char-events is disabled?
https://codereview.chromium.org/1267483003/diff/320001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1267483003/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:2134: IDS_FLAGS_DISABLE_MERGE_KEY_CHAR_EVENTS_NAME, On 2015/08/19 06:17:52, yukawa wrote: > If ENABLE_DISABLE_VALUE_TYPE works, can we rename this as > IDS_FLAGS_MERGE_KEY_CHAR_EVENTS_NAME? Done. https://codereview.chromium.org/1267483003/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:2137: SINGLE_VALUE_TYPE(switches::kDisableMergeKeyCharEvents)}, On 2015/08/19 06:17:52, yukawa wrote: > I'm not familiar with this, but would it be possible to use > ENABLE_DISABLE_VALUE_TYPE? Done. https://codereview.chromium.org/1267483003/diff/320001/win8/metro_driver/metr... File win8/metro_driver/metro_driver_win7.cc (right): https://codereview.chromium.org/1267483003/diff/320001/win8/metro_driver/metr... win8/metro_driver/metro_driver_win7.cc:468: ::TranslateMessage(&msg); On 2015/08/19 06:17:53, yukawa wrote: > Would it be possible to preserve the current behavior when merge-key-char-events > is disabled? Done.
Thanks. lgtm. Please update CL description that the new behavior would be enabled behind the flag.
On 2015/08/19 06:53:14, yukawa wrote: > Thanks. lgtm. > > Please update CL description that the new behavior would be enabled behind the > flag. Done. Thanks!
https://codereview.chromium.org/1267483003/diff/340001/base/base_switches.h File base/base_switches.h (right): https://codereview.chromium.org/1267483003/diff/340001/base/base_switches.h#n... base/base_switches.h:31: extern const char kDisableMergeKeyCharEvents[]; These flags aren't read from base/ so they shouldn't be defined in base_switches
https://codereview.chromium.org/1267483003/diff/340001/base/base_switches.h File base/base_switches.h (right): https://codereview.chromium.org/1267483003/diff/340001/base/base_switches.h#n... base/base_switches.h:31: extern const char kDisableMergeKeyCharEvents[]; On 2015/08/19 21:29:31, Nico (on vacation Thu Aug 20) wrote: > These flags aren't read from base/ so they shouldn't be defined in base_switches Is there a common switches header file that can be included from ui/base?
https://codereview.chromium.org/1267483003/diff/340001/base/base_switches.h File base/base_switches.h (right): https://codereview.chromium.org/1267483003/diff/340001/base/base_switches.h#n... base/base_switches.h:31: extern const char kDisableMergeKeyCharEvents[]; On 2015/08/20 00:54:27, Shu Chen wrote: > On 2015/08/19 21:29:31, Nico (on vacation Thu Aug 20) wrote: > > These flags aren't read from base/ so they shouldn't be defined in > base_switches > > Is there a common switches header file that can be included from ui/base? https://code.google.com/p/chromium/codesearch#search/&q=file:ui/base%20switch... maybe?
https://codereview.chromium.org/1267483003/diff/340001/base/base_switches.h File base/base_switches.h (right): https://codereview.chromium.org/1267483003/diff/340001/base/base_switches.h#n... base/base_switches.h:31: extern const char kDisableMergeKeyCharEvents[]; On 2015/08/20 02:51:57, Nico (on vacation Thu Aug 20) wrote: > On 2015/08/20 00:54:27, Shu Chen wrote: > > On 2015/08/19 21:29:31, Nico (on vacation Thu Aug 20) wrote: > > > These flags aren't read from base/ so they shouldn't be defined in > > base_switches > > > > Is there a common switches header file that can be included from ui/base? > > https://code.google.com/p/chromium/codesearch#search/&q=file:ui/base%20switch... > maybe? Done. Thank you!
Thanks! Taking a step back, why does this need a flag at all? It's a bugfix conceptually, right?
On 2015/08/20 03:01:53, Nico (on vacation Thu Aug 20) wrote: > Thanks! > > Taking a step back, why does this need a flag at all? It's a bugfix > conceptually, right? This is not a bug fix. This is kind of a refactoring to support the new feature: Chrome IMEs. As mentioned in the cl description, another potential benefit is that ui::KeyEvent::is_char() can be removed.
On 2015/08/20 03:03:49, Shu Chen wrote: > On 2015/08/20 03:01:53, Nico (on vacation Thu Aug 20) wrote: > > Thanks! > > > > Taking a step back, why does this need a flag at all? It's a bugfix > > conceptually, right? > > This is not a bug fix. This is kind of a refactoring to support the new feature: > Chrome IMEs. Ok, but if the flag is on by default, then why is a flag needed at all?
On 2015/08/20 03:14:19, Nico (on vacation Thu Aug 20) wrote: > On 2015/08/20 03:03:49, Shu Chen wrote: > > On 2015/08/20 03:01:53, Nico (on vacation Thu Aug 20) wrote: > > > Thanks! > > > > > > Taking a step back, why does this need a flag at all? It's a bugfix > > > conceptually, right? > > > > This is not a bug fix. This is kind of a refactoring to support the new > feature: > > Chrome IMEs. > > Ok, but if the flag is on by default, then why is a flag needed at all? Ah, can we make the flag off by default, Shu?
On 2015/08/20 03:14:19, Nico (on vacation Thu Aug 20) wrote: > On 2015/08/20 03:03:49, Shu Chen wrote: > > On 2015/08/20 03:01:53, Nico (on vacation Thu Aug 20) wrote: > > > Thanks! > > > > > > Taking a step back, why does this need a flag at all? It's a bugfix > > > conceptually, right? > > > > This is not a bug fix. This is kind of a refactoring to support the new > feature: > > Chrome IMEs. > > Ok, but if the flag is on by default, then why is a flag needed at all? Yukawa@ asked for the flag at #31: "For production I think we should have a feature flag so that we can revert the new behavior back to the previous one when something unexpected is found." The flag is enabled by default so that the coming Chrome IME API feature can be supported. As requested by yukawa@, the benefit of the flag is to easily investigate whether this cl is the root cause for potential regressions.
Pinging... ananta@, can you please take a look for owners of win8/metro_driver/...? Thanks
Yukawa@ asked for the flag at #31: "For production I think we should have a feature flag so that we can revert the new behavior back to the previous one when something unexpected is found." If there's bugs, we can fix them, or we can revert the CL (maybe only on the next release branch). We don't want to add a flag for every change we make – else every CL would add a flag! Flags should only be added for new features that need weeks of development time. Since this flag defaults to on, it suggests that the new code is considered functional and complete. I would probably not add a flag for this. (But if you do add it, please make sure to remove it after some time.)
cc scottmg@ just as a heads up. On 2015/08/24 16:20:58, Nico wrote: > Yukawa@ asked for the flag at #31: > "For production I think we should have a feature flag so that we can revert the > new behavior back to the previous one when something unexpected is found." > > If there's bugs, we can fix them, or we can revert the CL (maybe only on the > next release branch). We don't want to add a flag for every change we make > – else every CL would add a flag! Flags should only be added for new features > that need weeks of development time. Hmm, I thought this CL would perfectly fall into the category "new features that need weeks of development time". Followings are my understandings: 1. This CL is just a preparation of crbug.com/517773, and does not add any new functionality that is visible to users. We will see subsequent CLs that depend on this CL within several months and hence will coexist in several release branches. 2. Even if this CL doesn't violate any rule that is documented in MSDN site, there would be still chances that this CL would break some assumptions made in other non-system DLLs such as IMEs(!) and background utilities that inject their DLL to Chromium process. I wouldn't be surprised if we couldn't catch possible P1 issue in canary/dev/beta releases. I'm happy to help him to fix or revert the CL, but to be honest I'd be not comfortable if I needed to review urgent CLs. Please understand I'm not in Chrome team any more and has been reviewing IME changes in my spare time. (it's not even in 20% time!) 3. Potential risk of this CL is hard to predict. Not being able to revert the behavior in user side up to 6 weeks in the worst case scenario is what I wanted to avoid. > Since this flag defaults to on, it suggests > that the new code is considered functional and complete. I would probably not > add a flag for this. Again, I think we should start with flag off by default. Shu, what do you think? > (But if you do add it, please make sure to remove it after some time.) Sure. I totally agree that this kind of flag should be removed when all the change planned are successfully merged and stabilized.
On 2015/08/24 18:14:47, yukawa wrote: > cc scottmg@ just as a heads up. > > On 2015/08/24 16:20:58, Nico wrote: > > Yukawa@ asked for the flag at #31: > > "For production I think we should have a feature flag so that we can revert > the > > new behavior back to the previous one when something unexpected is found." > > > > If there's bugs, we can fix them, or we can revert the CL (maybe only on the > > next release branch). We don't want to add a flag for every change we make > > – else every CL would add a flag! Flags should only be added for new > features > > that need weeks of development time. > > Hmm, I thought this CL would perfectly fall into the category "new features that > need > weeks of development time". Followings are my understandings: > > 1. This CL is just a preparation of crbug.com/517773, and does not add any new > functionality that is visible to users. We will see subsequent CLs that > depend > on this CL within several months and hence will coexist in several release > branches. > 2. Even if this CL doesn't violate any rule that is documented in MSDN site, > there > would be still chances that this CL would break some assumptions made in > other > non-system DLLs such as IMEs(!) and background utilities that inject their > DLL > to Chromium process. I wouldn't be surprised if we couldn't catch possible > P1 > issue in canary/dev/beta releases. I'm happy to help him to fix or revert > the > CL, but to be honest I'd be not comfortable if I needed to review urgent CLs. > Please understand I'm not in Chrome team any more and has been reviewing IME > changes in my spare time. (it's not even in 20% time!) > 3. Potential risk of this CL is hard to predict. Not being able to revert the > behavior in user side up to 6 weeks in the worst case scenario is what I > wanted > to avoid. > > > Since this flag defaults to on, it suggests > > that the new code is considered functional and complete. I would probably not > > add a flag for this. > > Again, I think we should start with flag off by default. Shu, what do you > think? Thanks for explaining. It makes sense to implement this behind a flag.
On 2015/08/24 18:51:27, Nico wrote: > On 2015/08/24 18:14:47, yukawa wrote: > > cc scottmg@ just as a heads up. > > > > On 2015/08/24 16:20:58, Nico wrote: > > > Yukawa@ asked for the flag at #31: > > > "For production I think we should have a feature flag so that we can revert > > the > > > new behavior back to the previous one when something unexpected is found." > > > > > > If there's bugs, we can fix them, or we can revert the CL (maybe only on the > > > next release branch). We don't want to add a flag for every change we make > > > – else every CL would add a flag! Flags should only be added for new > > features > > > that need weeks of development time. > > > > Hmm, I thought this CL would perfectly fall into the category "new features > that > > need > > weeks of development time". Followings are my understandings: > > > > 1. This CL is just a preparation of crbug.com/517773, and does not add any new > > functionality that is visible to users. We will see subsequent CLs that > > depend > > on this CL within several months and hence will coexist in several release > > branches. > > 2. Even if this CL doesn't violate any rule that is documented in MSDN site, > > there > > would be still chances that this CL would break some assumptions made in > > other > > non-system DLLs such as IMEs(!) and background utilities that inject their > > DLL > > to Chromium process. I wouldn't be surprised if we couldn't catch possible > > P1 > > issue in canary/dev/beta releases. I'm happy to help him to fix or revert > > the > > CL, but to be honest I'd be not comfortable if I needed to review urgent > CLs. > > Please understand I'm not in Chrome team any more and has been reviewing > IME > > changes in my spare time. (it's not even in 20% time!) > > 3. Potential risk of this CL is hard to predict. Not being able to revert the > > behavior in user side up to 6 weeks in the worst case scenario is what I > > wanted > > to avoid. > > > > > Since this flag defaults to on, it suggests > > > that the new code is considered functional and complete. I would probably > not > > > add a flag for this. > > > > Again, I think we should start with flag off by default. Shu, what do you > > think? > > Thanks for explaining. It makes sense to implement this behind a flag. I'd prefer not to add a flag only for the fix made in this CL. IMHO, although crbug.com/517773 depends on this fix, it can also be considered as an independent code refactor to get rid of ui::KeyEvent::is_char(), which is always a weird thing. So I don't think it's necessary to add a flag only for this CL, as we can always fix potential bugs introduced by this code refactor. But I agree to add a flag for crbug.com/517773 in a whole. If you think this CL is too risky, we can hide this fix behind the all-in-one flag for crbug.com/517773 for now, and move it out of the flag later. But I'm wondering if the flag is off by default, how can we verify this fix in real environments?
On 2015/08/25 03:46:50, James Su wrote: > I'd prefer not to add a flag only for the fix made in this CL. IMHO, although > crbug.com/517773 depends on this fix, it can also be considered as an > independent code refactor to get rid of ui::KeyEvent::is_char(), which is always > a weird thing. > So I don't think it's necessary to add a flag only for this CL, as we can always > fix potential bugs introduced by this code refactor. I do not always say we should have flag for each huge refactoring, but being able to ask users to see any suspicious issue is correlated to this change by manually flipping the flag is still worth doing IMO. > But I agree to add a flag for crbug.com/517773 in a whole. If you think this CL > is too risky, we can hide this fix behind the all-in-one flag for > crbug.com/517773 for now, and move it out of the flag later. > > But I'm wondering if the flag is off by default, how can we verify this fix in > real environments? It depends on when crbug.com/517773 becomes ready. If it is not likely to happen in M47, perhaps it might want to consider to enable this refactoring in M47. One possible plan would be: 1. Land this CL with flag off by default. 2. Make sure there is no behavior change. 3. Turn on the flag in a subsequent CL. 4. At crbug.com, keep our eyes open and continue making sure that any newly filed issue is unrelated to this feature by asking to flip the flag until we are confident that nothing unexpected have occurred in stable channel (M47 in the best scenario). 5. Remove the flag. Having said that, I'm still wondering if starting manipulating message queue with PeekMessage API can be justified just to get rid of ui::KeyEvent::is_char(). I agree that crbug.com/517773 can justify it though. With this change so many users would be under the risk of breaking text input functionality, but it gives no clear benefit for users and web developer right now, right?
On 2015/08/25 05:38:48, yukawa wrote: > On 2015/08/25 03:46:50, James Su wrote: > > I'd prefer not to add a flag only for the fix made in this CL. IMHO, although > > crbug.com/517773 depends on this fix, it can also be considered as an > > independent code refactor to get rid of ui::KeyEvent::is_char(), which is > always > > a weird thing. > > So I don't think it's necessary to add a flag only for this CL, as we can > always > > fix potential bugs introduced by this code refactor. > > I do not always say we should have flag for each huge refactoring, but being > able > to ask users to see any suspicious issue is correlated to this change by > manually > flipping the flag is still worth doing IMO. > > > But I agree to add a flag for crbug.com/517773 in a whole. If you think this > CL > > is too risky, we can hide this fix behind the all-in-one flag for > > crbug.com/517773 for now, and move it out of the flag later. > > > > But I'm wondering if the flag is off by default, how can we verify this fix in > > real environments? > > It depends on when crbug.com/517773 becomes ready. If it is not likely to > happen > in M47, perhaps it might want to consider to enable this refactoring in M47. > One possible plan would be: > 1. Land this CL with flag off by default. > 2. Make sure there is no behavior change. > 3. Turn on the flag in a subsequent CL. > 4. At http://crbug.com, keep our eyes open and continue making sure that any newly > filed > issue is unrelated to this feature by asking to flip the flag until we are > confident that nothing unexpected have occurred in stable channel (M47 in the > best scenario). > 5. Remove the flag. > > Having said that, I'm still wondering if starting manipulating message queue > with > PeekMessage API can be justified just to get rid of ui::KeyEvent::is_char(). > I agree that crbug.com/517773 can justify it though. > > With this change so many users would be under the risk of breaking text input > functionality, but it gives no clear benefit for users and web developer right > now, > right? OK, it makes sense, yukawa@. I've by default turn off the flag, and will turn it on in a later cl after verifying the canary builds.
Pinging... Nico, mind taking another look? Now the flag is by default disabled. ananta@, can you please also take a look for owners of win8/metro_driver/...? Thanks!
Kindly reminder for review...
I think we should leave the metro code as is for now. It is very likely going to be removed in the near future. https://codereview.chromium.org/1267483003/diff/380001/ui/base/ime/input_meth... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/380001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:123: while (::PeekMessage(&msg, native_key_event.hwnd, WM_CHAR, WM_DEADCHAR, Peeking out WM_CHAR's here will solve the issue you guys are trying to address. However that effectively means that we lose the flexibility provided by the ui base event handling model, where in we can have handlers for messages at various stages. https://codereview.chromium.org/1267483003/diff/380001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:136: if (native_key_event.message == WM_CHAR) { Will this code execute if the kEnableMergeKeyCharEvents command line is enabled?
about_flags lgtm
https://codereview.chromium.org/1267483003/diff/380001/ui/base/ime/input_meth... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/380001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:123: while (::PeekMessage(&msg, native_key_event.hwnd, WM_CHAR, WM_DEADCHAR, On 2015/09/01 21:53:45, ananta wrote: > Peeking out WM_CHAR's here will solve the issue you guys are trying to address. > However that effectively means that we lose the flexibility provided by the ui > base event handling model, where in we can have handlers for messages at various > stages. > I don't see losing flexibility by this cl. Chrome will still support standalone WM_CHAR without physical key events. Also, Chrome can still handle the key events by its character information at any stages through KeyEvent::GetCharacter(). I will upload a new patch to use KeyEvent::set_character() to fill in the character information. However, in some edge cases (e.g. double dead keys), multiple WM_CHAR will be generated for single keydown. Therefore, there is no way to use KeyEvent::set_character() to fill in the character information. IMO, that is rare and it seems a reasonable cost to support Chrome IMEs. What's more, I don't think it is good to handle is_char()==true events at multiple stages, especially in Chrome common code, because is_char() is really specific for Windows platform. So, ideally, the use of is_char() and specially code for WM_CHAR should always be in the platform-dependent layer at lower level of Chrome. https://codereview.chromium.org/1267483003/diff/380001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:136: if (native_key_event.message == WM_CHAR) { On 2015/09/01 21:53:45, ananta wrote: > Will this code execute if the kEnableMergeKeyCharEvents command line is enabled? Yes. This remains the original behavior to support WM_CHAR messages without a physical key event. For example, some tests may generate WM_CHAR directly (or some 3rd party apps).
ananta@, can you please take another look at the latest patch? I've reverted the changes for win8 metro.
Pinging... ananta@, can you please check whether your concerns have been addressed?
The CQ bit was checked by shuchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267483003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267483003/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by shuchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267483003/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267483003/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Kindly pinging... ananta@, would you mind to take another look? Thanks
https://codereview.chromium.org/1267483003/diff/460001/ui/base/ime/input_meth... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/460001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:169: event->set_character(static_cast<base::char16>(char_msgs[0].wParam)); Is this code correct?. The wParams in WM_KEYDOWN and WM_CHAR messages are different things I think. They are virtual key codes in keydown and character codes in wm_char. Wondering if it would be better to disassemble the TranslateMessage function to see which WM_KEYDOWN messages translate into WM_CHARs' and use the same logic in the extension
https://codereview.chromium.org/1267483003/diff/460001/ui/base/ime/input_meth... File ui/base/ime/input_method_win.cc (right): https://codereview.chromium.org/1267483003/diff/460001/ui/base/ime/input_meth... ui/base/ime/input_method_win.cc:169: event->set_character(static_cast<base::char16>(char_msgs[0].wParam)); On 2015/09/22 19:27:30, ananta wrote: > Is this code correct?. The wParams in WM_KEYDOWN and WM_CHAR messages are > different things I think. They are virtual key codes in keydown and character > codes in wm_char. This code is correct. WM_CHAR's wParam carries the UTF16 code of the character. This code makes sure the ui::KeyEvent carries the correct UTF16 code instead of hard-coded one. For the hard-coded point, please refer to https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/event.cc... > > Wondering if it would be better to disassemble the TranslateMessage function to > see which WM_KEYDOWN messages translate into WM_CHARs' and use the same logic in > the extension That has been discussed. TranslateMessage internally uses ToUnicode() or ToUnicodeEx() to get the i18n character for the key code. It also maintains an internal state machine to deal with dead keys. Those logics cannot be copied into extension, and also the code of InputMethodWin cannot use ToUnicodeEx() directly.
This cl is pending for long and M47 has been cut. It should be safe to land this in M48 and I will ask QA to verify the cl. ananta@, if you have more concerns, please feel free to continue commenting on this cl. Thanks for your reviews!
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from suzhe@chromium.org, jln@chromium.org, robliao@chromium.org, yukawa@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1267483003/#ps500001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267483003/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267483003/500001
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukawa@chromium.org, thakis@chromium.org, jln@chromium.org, robliao@chromium.org, suzhe@chromium.org Link to the patchset: https://codereview.chromium.org/1267483003/#ps520001 (title: "fixed the compiling failure on win_chromium_compile_dbg_ng.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267483003/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267483003/520001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by shuchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukawa@chromium.org, thakis@chromium.org, jln@chromium.org, robliao@chromium.org, suzhe@chromium.org Link to the patchset: https://codereview.chromium.org/1267483003/#ps540001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267483003/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267483003/540001
Message was sent while issue was closed.
Committed patchset #28 (id:540001)
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/cb0b75575e1ebab71e76aaeb8784c18f95c0c145 Cr-Commit-Position: refs/heads/master@{#353106} |
