Chromium side of Writing Direction Context Menu on OS X

WebKit side is in https://bugs.webkit.org/show_bug.cgi?id=34524
    
 BUG=NONE
 TEST=Right clicking on a text input field in Web content on OS X should now show a Writing Direction submenu.  Clicking on "Left To Right" should align the input to the left, and "Right To Left" should do the opposit.  The "Default" menu item should always be disabled.

[Avi (use Gerrit), 2010-02-03 15:15:08]

LG with notes below

http://codereview.chromium.org/566031/diff/2001/3003
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/566031/diff/2001/3003#newcode874
chrome/browser/tab_contents/render_view_context_menu.cc:874: }
Why the three previous times we have these we ifdef mac them but not here?

[Hironori Bono, 2010-02-04 02:53:12]

LGTM with a style nit.
For your WebKit change, it may be better to call
Editor::setBaseWritingDirection(), which is used by Safari to change the writing
direction, instead of changing the CSS property directly.

Regards,

Hironori Bono

http://codereview.chromium.org/566031/diff/2001/3003
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/566031/diff/2001/3003#newcode342
chrome/browser/tab_contents/render_view_context_menu.cc:342: // OS X provides a
contextual menu to set writing direction for BiDi languages.
nit: need two spaces before this comment?

[jeremy, 2010-02-04 12:53:36]

I  don't change the CSS properties directly anywhere.

There are 2  parts to this change:
1. Update the checked/unchecked state of the menu items (on the webkit side).
2. Actually perform the action.

2. is implemented in terms of RenderViewHost::UpdateTextDirection() and
NotifyTextDirection() which is exactly the same mechanism used on Windows/Linux
and ultimately calls through to Editor::setBaseWritingDirection() as you
describe.

1. This part doesn't exist on Windows since there's no menu that needs updating
there.  The code here is identical to what Safari uses for the same menu and
checks the value of the CSS property in order to determine which is the
currently selected option in the context menu.

On 2010/02/04 02:53:12, hbono wrote:
> LGTM with a style nit.
> For your WebKit change, it may be better to call
> Editor::setBaseWritingDirection(), which is used by Safari to change the
writing
> direction, instead of changing the CSS property directly.
> 
> Regards,
> 
> Hironori Bono
> 
> http://codereview.chromium.org/566031/diff/2001/3003
> File chrome/browser/tab_contents/render_view_context_menu.cc (right):
> 
> http://codereview.chromium.org/566031/diff/2001/3003#newcode342
> chrome/browser/tab_contents/render_view_context_menu.cc:342: // OS X provides
a
> contextual menu to set writing direction for BiDi languages.
> nit: need two spaces before this comment?

[Hironori Bono, 2010-02-04 12:59:22]

Sorry, this is just my misunderstanding. You are correct.

Regards,

Hironori Bono