A tricky fix for Issue 1845....

A tricky fix for Issue 1845.

This change is a very tricky fix for Issue 1845 in chromium: cant alignt text to the right using right shift and right ctrl.This change consists of two parts listed below.

1. Emulating the implementation of Safari that changes the text-direction of an input element.

Safari uses context menus to change the text direction. This change adds an IPC message 'ViewMsg_SetTextDirection', which notifies the new text direction. Also, it adds two functions: RenderWidgetHost::UpdateTextDirection() and RenderWidgetHost::NotifyTextDirection(). They encapsulate the new IPC message so that we can use them both when we presses a set of keys and when we add context-menu items which change the text direction.

2. Calling the above interface when pressing right-shift and right-control keys, or when left-shift and left-control keys.

This modifies the RenderWidgetHostViewWin::OnKeyEvent() function and call the above text-direction interfaces when a user finishes pressing the keys. As you can imagine, if we send an IPC message every time when we receive a WM_KEYDOWN event, we continue sending IPC messages while a user is pressing the keys.

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

[Hironori Bono, 2009-03-09 10:02:30]

Hi Darin, I have added an IPC message that changes the text direction of a
selected input element, so that we can change it from both context menus (for
Mac) and keyboard events (for Windows).
Would it be possible to give me your comments about this design?

[jeremy, 2009-03-09 10:07:41]

LGTM with a few nits.

This fix will make many people very happy :)

http://codereview.chromium.org/39252/diff/30/1047
File chrome/renderer/render_view_unittest.cc (right):

http://codereview.chromium.org/39252/diff/30/1047#newcode347
Line 347: // TODO(hbono): should we also check the CSS 'direction' property?
Sounds like a good idea...

http://codereview.chromium.org/39252/diff/30/1047#newcode355
Line 355: // COpy the document content to std::wstring and compare with the
Copy

http://codereview.chromium.org/39252/diff/30/1035
File webkit/glue/webframe_impl.cc (right):

http://codereview.chromium.org/39252/diff/30/1035#newcode1824
Line 1824: void WebFrameImpl::SetTextDirection(bool right_to_left) {
Is there any chance that frame()->editor() might be NULL?

[Hironori Bono, 2009-03-09 13:28:54]

Hi Darin,

On 2009/03/09 10:02:30, hbono wrote:
> Hi Darin, I have added an IPC message that changes the text direction of a
> selected input element, so that we can change it from both context menus (for
> Mac) and keyboard events (for Windows).
> Would it be possible to give me your comments about this design?

I would like to add backgrounds of this implementation.
This Issue 1845 is a feature request of adding a keyboard command which changes
the text direction (or writing direction) of the selected input node. So, to fix
only this issue, we don't have to add IPC messages.
On the other hand, when I talked with Mac people, Mac people prefer changing the
text direction from context menus as Safari does. So, I assume we will also need
to implement context menus that changes the text direction.
To share the back-end code which changes the text direction among Windows, Mac,
and Linux, this change adds a view message 'ViewMsg_SetTextDirection' for both
the RenderView and RenderWidget classes. As you can see, this change is more
complicated than just adding editor commands. We can completely share the
back-end code. Also, we can use the RenderViewTest class to test the back-end.

Regards,

Hironori Bono
E-mail: hbono@chromium.org

[darin (slow to review), 2009-03-09 17:00:59]

http://codereview.chromium.org/39252/diff/30/1037
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/39252/diff/30/1037#newcode364
Line 364: virtual void SetTextDirection(bool right_to_left) = 0;
oh, this is operating on the current Selection.  hmm... seems like the function
name should correspond to that.  also, it seems like the text may already be RTL
in some cases.  what this is really doing is forcing the text to be RTL right? 
is there a way to reset the forcing of the text direction?

how about using a declaration like this:

 // declared at global scope
 enum WebTextDirection {
   WEB_TEXT_DIRECTION_DEFAULT,
   WEB_TEXT_DIRECTION_LTR,
   WEB_TEXT_DIRECTION_RTL,
 };

 // declared on WebFrame
 SetTextDirectionForCurrentSelection(WebTextDirection);

http://codereview.chromium.org/39252/diff/30/1039
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/39252/diff/30/1039#newcode86
Line 86: // modifiers for raw keyboard status.
does this comment imply that these flags are only used for RAW_KEY_DOWN events?

http://codereview.chromium.org/39252/diff/30/1036
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/39252/diff/30/1036#newcode340
Line 340: modifiers |= (keyboard_state[VK_LSHIFT] & 0x80) ? LEFT_SHIFT_KEY : 0;
nit: how about following the same pattern used above?

 if (..foo..)
   modifiers |= FOO_KEY;

also, if GetKeyState is slow, why don't we just do GetKeyboardState above and
use it to extract SHIFT_KEY, CTRL_KEY and ALT_KEY as well?

[jungshik at Google, 2009-03-10 22:44:01]

http://codereview.chromium.org/39252/diff/30/1037
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/39252/diff/30/1037#newcode364
Line 364: virtual void SetTextDirection(bool right_to_left) = 0;
On 2009/03/09 17:01:00, darin wrote:
> oh, this is operating on the current Selection. 

Judging from what's implemented in webframe_impl and a test, it seems that this
changes the base text direction of a textarea (or other input fields) with the
focus (rather than the current Selection). 


>  hmm... seems like the function
> name should correspond to that.  also, it seems like the text may already be
RTL
> in some cases.  what this is really doing is forcing the text to be RTL right?

> is there a way to reset the forcing of the text direction?
> 
> how about using a declaration like this:
> 
>  // declared at global scope
>  enum WebTextDirection {
>    WEB_TEXT_DIRECTION_DEFAULT,
>    WEB_TEXT_DIRECTION_LTR,
>    WEB_TEXT_DIRECTION_RTL,
>  };

I like this idea except that having 'DEFAULT' may require a bit more work
(retrieve/record what's the default. Hmm. having said that, I wonder if DEFAULT
is necessary)

>  // declared on WebFrame
>  SetTextDirectionForCurrentSelection(WebTextDirection);

[brettw, 2009-03-11 14:36:17]

http://codereview.chromium.org/39252/diff/30/1042
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/39252/diff/30/1042#newcode316
Line 316: if (key_event.windows_key_code == base::VKEY_SHIFT ||
Is this code Windows-only? If so, can it be moved to RenderWidgetHostViewWin?

http://codereview.chromium.org/39252/diff/30/1043
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/39252/diff/30/1043#newcode251
Line 251: virtual void UpdateTextDirection(bool right_to_left) {}
I'm not sure I understand why some of the code is in RenderViewHost and some is
in RenderWidgeHost. Can it all be in RenderWidgetHost?

[jungshik at Google, 2009-03-13 20:52:35]

http://codereview.chromium.org/39252/diff/3003/3005
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/39252/diff/3003/3005#newcode227
Line 227: void UpdateTextDirection(bool right_to_left);
I'm afraid this function is a bit confusing when someboday later looks at its
call sites. I like Darin's idea of having an enum. How about just having two
enums (without DEFAULT)?

[Hironori Bono, 2009-03-16 11:16:42]

Thank you all for your great comments and sorry for my slow updates.
I finally applied most of your comments and uploaded the latest one. (I'm
wondering if it is the best option to add a new header file "webtextdirection.h"
into "webkit/glue", though.)
 
Would it be possible to review the latest one?

http://codereview.chromium.org/39252/diff/30/1042
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/39252/diff/30/1042#newcode316
Line 316: if (key_event.windows_key_code == base::VKEY_SHIFT ||
On 2009/03/11 14:36:18, brettw wrote:
> Is this code Windows-only? If so, can it be moved to RenderWidgetHostViewWin?

Done. Thank you for noticing this.
Yes, this code is used only on Windows and I should have moved it to the
RenderWidgetHostViewWin class.
I have moved this code into the RenderWidgetHostViewWin class.

http://codereview.chromium.org/39252/diff/30/1043
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/39252/diff/30/1043#newcode251
Line 251: virtual void UpdateTextDirection(bool right_to_left) {}
On 2009/03/11 14:36:18, brettw wrote:
> I'm not sure I understand why some of the code is in
> RenderViewHost and some is
> in RenderWidgeHost. Can it all be in RenderWidgetHost?

Done. Sorry for the lack of my description.
This is just a legacy of my implementation.
I firstly ported the WebKit code, which changes the text direction in
"WebHTMLView.mm", into our RenderView classes.
After porting the code, I changed this code so that the RenderWidget classes can
use it.
To read now, the RenderView classes are derived from the RenderWidget classes
and we don't have to use the RenderView classes.
I have moved all implementation into the RenderWidget classes.

http://codereview.chromium.org/39252/diff/30/1047
File chrome/renderer/render_view_unittest.cc (right):

http://codereview.chromium.org/39252/diff/30/1047#newcode347
Line 347: // TODO(hbono): should we also check the CSS 'direction' property?
On 2009/03/09 10:07:41, jeremy wrote:
> Sounds like a good idea...

Done. Thank you for noticing this.

http://codereview.chromium.org/39252/diff/30/1047#newcode355
Line 355: // COpy the document content to std::wstring and compare with the
On 2009/03/09 10:07:41, jeremy wrote:
> Copy

Done. Sorry for my stupid typo.

http://codereview.chromium.org/39252/diff/30/1037
File webkit/glue/webframe.h (right):

http://codereview.chromium.org/39252/diff/30/1037#newcode364
Line 364: virtual void SetTextDirection(bool right_to_left) = 0;
On 2009/03/09 17:01:00, darin wrote:
> oh, this is operating on the current Selection.
> hmm... seems like the function name should correspond
> to that.  also, it seems like the text may already be RTL
> in some cases.  what this is really doing is forcing the
> text to be RTL right? 
> is there a way to reset the forcing of the text direction?
> 
> how about using a declaration like this:
> 
>  // declared at global scope
>  enum WebTextDirection {
>    WEB_TEXT_DIRECTION_DEFAULT,
>    WEB_TEXT_DIRECTION_LTR,
>    WEB_TEXT_DIRECTION_RTL,
>  };

Done. Thank you for noticing this.
I added a new header file "webkit/glue/webtextdirection.h" and defined this type
in it.
On the other hand, I'm wondering if we should change the function name, though.

http://codereview.chromium.org/39252/diff/3003/3005
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/39252/diff/3003/3005#newcode227
Line 227: void UpdateTextDirection(bool right_to_left);
On 2009/03/13 20:52:35, Jungshik Shin wrote:
> I'm afraid this function is a bit confusing when someday
> later looks at its call sites.
> I like Darin's idea of having an enum.
> How about just having two enums (without DEFAULT)?  

Done. Sorry for the lack of notices.
After uploading this temporal change, I was not able to upload changes to
Rietveld probably because of a network problem. Even though I changed this
parameter to a enum type, I was not able to upload it and ask for another
review. The latest one uses the enum type (as suggested by Darin).

[brettw, 2009-03-16 20:54:15]

LGTM. Can you address the following minor comments before you check in?

http://codereview.chromium.org/39252/diff/2015/2017
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/39252/diff/2015/2017#newcode230
Line 230: void NotifyTextDirection();
It's not clear to me as a caller how I should call these two functions. When
should I use one versus the other?

http://codereview.chromium.org/39252/diff/2015/2027
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/39252/diff/2015/2027#newcode833
Line 833: if ((GetKeyState(VK_RSHIFT) & 0x8000) &&
What do you think about making a constant local to this function with a
descriptive name instead of hardcoding 0x8000 everywhere?

http://codereview.chromium.org/39252/diff/2015/2030
File webkit/glue/webtextdirection.h (right):

http://codereview.chromium.org/39252/diff/2015/2030#newcode5
Line 5: #ifndef WEBKIT_GLUE_WEBTEXTDIRECTION_H__
Use only one underscore at the end of include guards now (they changed the style
guide).