A quick fix for Issue 9762 and 9763....

A quick fix for Issue 9762 and 9763.
This is caused by my change r12434 that does not check if there are any other keys pressed when a user presses left-control and left-shift keys (or right-control and right-shift keys).
To fix this issue, this change does not only add the above check, but also it adds code that cancels updating the text direction if a user presses another key while he/she is pressing control and shift keys.

BUG=9762
"Regression: Shift+Ctrl+Arrow selection combination also changes text direction in RTL UIs"
BUG=9763
"Ctrl+Shift text alignment depends of the sides of both the Ctrl and the Shift keys"

TEST=In a LTR <textarea> element, pressing a control key, pressing a right-shift key, releasing the right-shift key, and verify its text direction is changed to RTL.
TEST=In an RTL <textarea> element, pressing a control key, pressing a left-shift key, releasing the left-shift key, and verify its text direction is changed to LTR.
TEST=In a LTR <textarea> element, pressing a right-shift key, pressing a control key, releasing the right-shift key, and verify its text direction is still LTR.
TEST=In a LTR <textarea> element, pressing a control key, pressing a right-shift key, releasing the control key, and verify its text direction is changed to RTL.
TEST=In an RTL <textarea> element, pressing a left-shift key, pressing a control key, releasing the left-shift key, and verify its text direction is still RTL.
TEST=In an RTL <textarea> element, pressing a control key, pressing a left-shift key, releasing the control key, and verify its text direction is changed to LTR.
TEST=In a LTR <textarea> element, pressing a control key, pressing a right-shift key, pressing an arrow key, releasing the right-shift key, and verify its text direction is still LTR.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=13588

[jungshik at Google, 2009-04-08 21:33:17]

http://codereview.chromium.org/63117/diff/8/1011
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/63117/diff/8/1011#newcode96
Line 96: (keystate[VK_LCONTROL] & kKeyDownMask)) {
It seems that you're fixing issue 9762 but not issue 9763. Shouldn't you have :

if (Control-key-down) 
  if (RShift-down)
     ....
  else if (LShift-Down)
     ....

[Hironori Bono, 2009-04-09 10:40:19]

Thank you so much for your review and comment.
I have updated my change (and also its descriptions) to apply your comment.
Is it possible to review it the updated one?

http://codereview.chromium.org/63117/diff/8/1011
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/63117/diff/8/1011#newcode96
Line 96: (keystate[VK_LCONTROL] & kKeyDownMask)) {
On 2009/04/08 21:33:17, Jungshik Shin wrote:
> It seems that you're fixing issue 9762 but not issue 9763. Shouldn't you have
:
> 
> if (Control-key-down) 
>   if (RShift-down)
>      ....
>   else if (LShift-Down)
>      ....

Done. Thank you for noticing this. I forgot changing this code to fix Issue
9763.

[jungshik at Google, 2009-04-09 17:18:04]

http://codereview.chromium.org/63117/diff/11/1013
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/63117/diff/11/1013#newcode405
Line 405: bool text_direction_canceled_;
We have 2 boolean state variables, but in the implementation, there are only 3
states referred to:

1. Update(T), Cancelled(T)
2. U(T),      C(F)
3. U(F),      C(F)

And, when sending an IPC (Notify....()), #1 and #3 don't seem to make any
difference.  Can we just have a single state variable (text_direction_updated)
by combining #1 and #3 into one state? Then, Cancel...() method will just have
to set it to false.  Am I missing something?

[Hironori Bono, 2009-04-10 07:25:03]

Thank you for your review and comments.

http://codereview.chromium.org/63117/diff/11/1013
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/63117/diff/11/1013#newcode405
Line 405: bool text_direction_canceled_;
On 2009/04/09 17:18:05, Jungshik Shin wrote:
> We have 2 boolean state variables, but in the implementation, there are only 3
> states referred to:
>
> 1. Update(T), Cancelled(T)
> 2. U(T),      C(F)
> 3. U(F),      C(F)
>
> And, when sending an IPC (Notify....()), #1 and #3 don't seem to make any
> difference.  Can we just have a single state variable (text_direction_updated)
> by combining #1 and #3 into one state? Then, Cancel...() method will just have
> to set it to false.  Am I missing something? 

Sorry for confusing you with the lack of comments.
This flag is not only for canceling an update, but also ignoring succeeding
update requests until we call NotifyTextDirection().
Because of auto-repeat, we may call UpdateTextDirection(）after calling
CancelUpdateTextDirection() to cancel updating the text direction. (For example,
when we hold SHIFT+CONTROL keys after releasing an arrow key, Windows sends
keydown events and notices SHIFT+CONTROL keys are down.)

So, when we merge them into one variable, we have to determine an
UpdateTextDirection() call is called before calling CancelUpdateTextDirection()
or after the call for preventing us from changing the text direction in the
above case.

To prevent handling such complicated state transition, I added another flag.

I agree this flag is confusing and I have added a comment to describe this. :)

[jungshik at Google, 2009-04-10 16:22:54]

LGTM !

Thank you for the explanation and the additional comment.

http://codereview.chromium.org/63117/diff/4004/4007
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/63117/diff/4004/4007#newcode74
Line 74: //   If only left-control and left-shift keys are down.
nit: left-control and right-control here should be just 'a control key' (either
left or right).