[Mac]Refactor input method related code.

[Mac]Refactor input method related code.

BUG=30670
Cannot input any characters after typing CTRL+H on IME
BUG=33824
Shortcut key Ctrl+K in Japanese IME reset cursor position to end of the string
BUG=42690
first ime keydown has the wrong keycode on Mac
BUG=43087
1st time type CJK character with IME in any text input field of websites,1st character is always deleted.
BUG=43454
When converting a Hangul to Chinese character, a new line is inserted before the character to convert.
TEST=See bug reports.

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

[James Su, 2010-05-05 18:01:19]

Hi all,
  Please kindly help review this CL. It'll be best if it can be merged into M5,
as the bug is pretty serious, which may affect all CJK users.

Regards
James Su

[Peter Kasting, 2010-05-05 18:03:05]

I'm not really capable of reviewing this -- ObjC, IMEs, and the
RenderWidgetHostView are all unfamiliar territory for me.

[Avi (use Gerrit), 2010-05-05 18:09:10]

On 2010/05/05 18:03:05, Peter Kasting wrote:
> I'm not really capable of reviewing this -- ObjC, IMEs, and the
> RenderWidgetHostView are all unfamiliar territory for me.

I'd highly recommend hbono for review. He's the Mac IME expert. I'll look at it
too.

[Avi (use Gerrit), 2010-05-05 18:12:25]

The only thing that jumped out at me was a typo. I'm really not sure what's
going on here, and would need to study up on IME to be able to competently
review it.

http://codereview.chromium.org/1908006/diff/6001/1003
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/1908006/diff/6001/1003#newcode120
chrome/browser/renderer_host/render_widget_host_view_mac.h:120: // Cancel onging
composition (abandone the marked text).
abandon

[James Su, 2010-05-05 18:13:55]

On 2010/05/05 18:09:10, Avi wrote:
> On 2010/05/05 18:03:05, Peter Kasting wrote:
> > I'm not really capable of reviewing this -- ObjC, IMEs, and the
> > RenderWidgetHostView are all unfamiliar territory for me.
> 
> I'd highly recommend hbono for review. He's the Mac IME expert. I'll look at
it
> too.
I'll be great if he can review it. But as we'd like to merge it into M5, so we
may finish it today.

[James Su, 2010-05-05 18:27:23]

Let me explain the motivation of this fix:
According to http://www.w3.org/TR/DOM-Level-3-Events/#keyset, when using an
input method, a typical key event sequence might be:

keydown
compositionstart
compositionupdate
keyup
keydown
compositionend
textinput
keyup

And in order to match the behavior on Windows, if a key down event was handled
by the input method, then we need to change its keycode to 229 (VK_PROCESSED).
Many web apps rely on this behavior.

The key problem of original code is: the key down event will be changed
according to the result of previous key event and dispatched to webkit before
calling the input method. So the first key down event would always wrong.

This CL fixes this problem by calling the input method first then deciding what
should be done for the key event according to the result of input method.

On 2010/05/05 18:12:25, Avi wrote:
> The only thing that jumped out at me was a typo. I'm really not sure what's
> going on here, and would need to study up on IME to be able to competently
> review it.
> 
> http://codereview.chromium.org/1908006/diff/6001/1003
> File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):
> 
> http://codereview.chromium.org/1908006/diff/6001/1003#newcode120
> chrome/browser/renderer_host/render_widget_host_view_mac.h:120: // Cancel
onging
> composition (abandone the marked text).
> abandon

[Scott Hess - ex-Googler, 2010-05-05 19:48:27]

Just a bunch of nits, I'm still trying to wrap my head around what's going on.

http://codereview.chromium.org/1908006/diff/6001/1003
File chrome/browser/renderer_host/render_widget_host_view_mac.h (left):

http://codereview.chromium.org/1908006/diff/6001/1003#oldcode240
chrome/browser/renderer_host/render_widget_host_view_mac.h:240: int
im_modifiers_;
Where did this one go?  Hmm.  It was never used, right?

http://codereview.chromium.org/1908006/diff/6001/1004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/1908006/diff/6001/1004#newcode332
chrome/browser/renderer_host/render_widget_host_view_mac.mm:332:
caret_rect.width(), caret_rect.height())];
While you're here, maybe clean this up.  I hate multi-line calls :-).  I think
there's a gfx::Rect->NSRect conversion, converting to NSRect then adjusting the
y-position would be reasonable.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode881
chrome/browser/renderer_host/render_widget_host_view_mac.mm:881: [NSCursor
setHiddenUntilMouseMoves:YES];
Aside: that code is always YES.  I wonder if this should be removed at some
point?

http://codereview.chromium.org/1908006/diff/6001/1004#newcode890
chrome/browser/renderer_host/render_widget_host_view_mac.mm:890: BOOL
oldHasMarkedText = hasMarkedText_;
Was going to suggest AutoReset, but ... dammit!  Not for BOOL.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode894
chrome/browser/renderer_host/render_widget_host_view_mac.mm:894:
handlingKeyDown_ = YES;
Could you DCHECK(!handleKeyDown_) beforehand, just to make sure we're never ever
reentrant?

http://codereview.chromium.org/1908006/diff/6001/1004#newcode896
chrome/browser/renderer_host/render_widget_host_view_mac.mm:896: // These two
variable might be set when handling the keyboard event.
"variables"

http://codereview.chromium.org/1908006/diff/6001/1004#newcode917
chrome/browser/renderer_host/render_widget_host_view_mac.mm:917: } else {
It's not obvious to me why the previous code could allow the following to be
tucked into the else?

http://codereview.chromium.org/1908006/diff/6001/1004#newcode929
chrome/browser/renderer_host/render_widget_host_view_mac.mm:929: //
immediatelly.
"immediately".  Also, while the previous commentary was a bit verbose, you
should definitely indicate that ForwardKeyboardEvent() could have destroyed the
widget.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode945
chrome/browser/renderer_host/render_widget_host_view_mac.mm:945: if
(textToBeInserted_.length() > (oldHasMarkedText ? 0 : 1)) {
I think this test is excessively cute.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode1618
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1618:
markedRange_.length = length;
NSMakeRange().

http://codereview.chromium.org/1908006/diff/6001/1004#newcode1620
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1620: newMarkedText_
= UTF8ToUTF16([im_text UTF8String]);
Suggest base::SysNSStringToUTF16() while you're here.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode1677
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1677:
textToBeInserted_.append(UTF8ToUTF16([im_text UTF8String]));
Convert this one, too.

[James Su, 2010-05-05 21:27:38]

http://codereview.chromium.org/1908006/diff/6001/1003
File chrome/browser/renderer_host/render_widget_host_view_mac.h (left):

http://codereview.chromium.org/1908006/diff/6001/1003#oldcode240
chrome/browser/renderer_host/render_widget_host_view_mac.h:240: int
im_modifiers_;
On 2010/05/05 19:48:28, shess wrote:
> Where did this one go?  Hmm.  It was never used, right?

It was not used since long time ago.

http://codereview.chromium.org/1908006/diff/6001/1003
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/1908006/diff/6001/1003#newcode120
chrome/browser/renderer_host/render_widget_host_view_mac.h:120: // Cancel onging
composition (abandone the marked text).
On 2010/05/05 18:12:25, Avi wrote:
> abandon

Done.

http://codereview.chromium.org/1908006/diff/6001/1004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/1908006/diff/6001/1004#newcode332
chrome/browser/renderer_host/render_widget_host_view_mac.mm:332:
caret_rect.width(), caret_rect.height())];
On 2010/05/05 19:48:28, shess wrote:
> While you're here, maybe clean this up.  I hate multi-line calls :-).  I think
> there's a gfx::Rect->NSRect conversion, converting to NSRect then adjusting
the
> y-position would be reasonable.

Done.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode881
chrome/browser/renderer_host/render_widget_host_view_mac.mm:881: [NSCursor
setHiddenUntilMouseMoves:YES];
On 2010/05/05 19:48:28, shess wrote:
> Aside: that code is always YES.  I wonder if this should be removed at some
> point?

The effect of [NSCursor setHiddenUntilMouseMoves:YES] will be reverted when the
mouse moves, so we need to call it every time.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode894
chrome/browser/renderer_host/render_widget_host_view_mac.mm:894:
handlingKeyDown_ = YES;
On 2010/05/05 19:48:28, shess wrote:
> Could you DCHECK(!handleKeyDown_) beforehand, just to make sure we're never
ever
> reentrant?

Done.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode896
chrome/browser/renderer_host/render_widget_host_view_mac.mm:896: // These two
variable might be set when handling the keyboard event.
On 2010/05/05 19:48:28, shess wrote:
> "variables"

Done.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode917
chrome/browser/renderer_host/render_widget_host_view_mac.mm:917: } else {
On 2010/05/05 19:48:28, shess wrote:
> It's not obvious to me why the previous code could allow the following to be
> tucked into the else?

The old code may have problem here. We should only match a key combination
against hotkeys for edit commands if the input method did not handle it.
Otherwise if the key combination was both handled by the input method and
matched  an edit command, the result may confuse the user.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode929
chrome/browser/renderer_host/render_widget_host_view_mac.mm:929: //
immediatelly.
On 2010/05/05 19:48:28, shess wrote:
> "immediately".  Also, while the previous commentary was a bit verbose, you
> should definitely indicate that ForwardKeyboardEvent() could have destroyed
the
> widget.

Done.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode945
chrome/browser/renderer_host/render_widget_host_view_mac.mm:945: if
(textToBeInserted_.length() > (oldHasMarkedText ? 0 : 1)) {
On 2010/05/05 19:48:28, shess wrote:
> I think this test is excessively cute.

It can make the line much shorter. :) I can change it if you don't like it.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode1620
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1620: newMarkedText_
= UTF8ToUTF16([im_text UTF8String]);
On 2010/05/05 19:48:28, shess wrote:
> Suggest base::SysNSStringToUTF16() while you're here.

Done.

http://codereview.chromium.org/1908006/diff/6001/1004#newcode1677
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1677:
textToBeInserted_.append(UTF8ToUTF16([im_text UTF8String]));
On 2010/05/05 19:48:28, shess wrote:
> Convert this one, too.

Done.

[Hironori Bono, 2010-05-06 00:43:45]

Unfortunately, I cannot evaluate this change anytime soon because I cannot get
access to my Mac until the end of the next week. So, I don't have any opinions
for or against this change. If refactoring this code, it might be better to
implement the NSTextInputClient protocol rather than to continue using the
NSTextInput protocol as recommended by Apple?

Regards,

Hironori Bono

[James Su, 2010-05-06 01:01:08]

NSTextInputClient protocol is only available after 10.5. I'm wondering if we
still support older system? And it's not that trivial if we want to fully
support NSTextInputClient protocol, as it requires full access of the DOM node
content from browser side. And this feature is actually also necessary for
Windows and Linux ports.
So I'd like to submit this CL first and then work on those new features later.

On 2010/05/06 00:43:45, hbono wrote:
> Unfortunately, I cannot evaluate this change anytime soon because I cannot get
> access to my Mac until the end of the next week. So, I don't have any opinions
> for or against this change. If refactoring this code, it might be better to
> implement the NSTextInputClient protocol rather than to continue using the
> NSTextInput protocol as recommended by Apple?
> 
> Regards,
> 
> Hironori Bono

[Avi (use Gerrit), 2010-05-06 14:16:12]

On 2010/05/06 01:01:08, James Su wrote:
> NSTextInputClient protocol is only available after 10.5. I'm wondering if we
> still support older system?

No, we do not support anything older than 10.5. It's safe to implement
NSTextInputClient.

> So I'd like to submit this CL first and then work on those new features later.

That's fine; we already have a bug for NSTextInputClient: bug 25037.

[James Su, 2010-05-06 23:05:51]

Seems that this CL can fix more issues than I expected.

Regards
James Su

[Scott Hess - ex-Googler, 2010-05-07 00:13:04]

I remain incapable of giving you a confident LGTM, I just do not understand what
the thrust of the overall code is well enough, and trying to figure it out won't
mean I'm right!

Meaning that if neither pink nor Avi jump up and say otherwise, I guess it's
alright to check in, but please not before you're sure they've cut and pushed a
new dev channel.  I'd much rather this go in just after a dev channel than just
before, so we get the best baking by in-house developers.

http://codereview.chromium.org/1908006/diff/6001/1004
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/1908006/diff/6001/1004#newcode881
chrome/browser/renderer_host/render_widget_host_view_mac.mm:881: [NSCursor
setHiddenUntilMouseMoves:YES];
On 2010/05/05 21:27:38, James Su wrote:
> On 2010/05/05 19:48:28, shess wrote:
> > Aside: that code is always YES.  I wonder if this should be removed at some
> > point?
> 
> The effect of [NSCursor setHiddenUntilMouseMoves:YES] will be reverted when
the
> mouse moves, so we need to call it every time.

+[RenderWidgetHostViewCocoa shouldAutohideCursorForEvent:] returns YES if
theEvent is not NSKeyDown.  So -setHiddenUntilMouseMoves: will always be called
(which should be appropriate).  Unless I'm missing something?

http://codereview.chromium.org/1908006/diff/15001/16001
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/1908006/diff/15001/16001#newcode120
chrome/browser/renderer_host/render_widget_host_view_mac.h:120: // Cancel onging
composition (abandon the marked text).
s/onging/ongoing/

http://codereview.chromium.org/1908006/diff/15001/16002
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/1908006/diff/15001/16002#newcode328
chrome/browser/renderer_host/render_widget_host_view_mac.mm:328: //
-RectToNSRect: method does the trick for us.
Last sentence of comment replicates the code, just drop it.

http://codereview.chromium.org/1908006/diff/15001/16002#newcode955
chrome/browser/renderer_host/render_widget_host_view_mac.mm:955: } else if
([theEvent modifierFlags] & (NSControlKeyMask | NSCommandKeyMask) &&
This is uncommon enough that you should probably put () around the &.  Whether
it's right or not, it would freak out fewer readers.

http://codereview.chromium.org/1908006/diff/15001/16002#newcode1181
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1181:
renderWidgetHostView_->render_widget_host_->ImeSetInputMode(true);
Should this be true ANY time we have key focus?  The old code seemed wrong
because it never reset this.  But AFAICT it never set it until you had marked
text.  [I do not know how it should really work.]

http://codereview.chromium.org/1908006/diff/15001/16002#newcode1528
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1528: if
(!validAttributesForMarkedText_.get()) {
Don't use the get() if you don't need it.

[James Su, 2010-05-07 17:02:36]

http://codereview.chromium.org/1908006/diff/15001/16001
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/1908006/diff/15001/16001#newcode120
chrome/browser/renderer_host/render_widget_host_view_mac.h:120: // Cancel onging
composition (abandon the marked text).
On 2010/05/07 00:13:04, shess wrote:
> s/onging/ongoing/

Done.

http://codereview.chromium.org/1908006/diff/15001/16002
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/1908006/diff/15001/16002#newcode328
chrome/browser/renderer_host/render_widget_host_view_mac.mm:328: //
-RectToNSRect: method does the trick for us.
On 2010/05/07 00:13:04, shess wrote:
> Last sentence of comment replicates the code, just drop it.

Done.

http://codereview.chromium.org/1908006/diff/15001/16002#newcode955
chrome/browser/renderer_host/render_widget_host_view_mac.mm:955: } else if
([theEvent modifierFlags] & (NSControlKeyMask | NSCommandKeyMask) &&
On 2010/05/07 00:13:04, shess wrote:
> This is uncommon enough that you should probably put () around the &.  Whether
> it's right or not, it would freak out fewer readers.

Done.

http://codereview.chromium.org/1908006/diff/15001/16002#newcode1181
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1181:
renderWidgetHostView_->render_widget_host_->ImeSetInputMode(true);
On 2010/05/07 00:13:04, shess wrote:
> Should this be true ANY time we have key focus?  The old code seemed wrong
> because it never reset this.  But AFAICT it never set it until you had marked
> text.  [I do not know how it should really work.]
I think we need to enable it as soon as we get the focus, otherwise the renderer
even won't send us the ime enable/disable event. This is necessary for fixing
issue 23219 and 41876.

http://codereview.chromium.org/1908006/diff/15001/16002#newcode1528
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1528: if
(!validAttributesForMarkedText_.get()) {
On 2010/05/07 00:13:04, shess wrote:
> Don't use the get() if you don't need it.

Done.

[Scott Hess - ex-Googler, 2010-05-07 17:09:31]

http://codereview.chromium.org/1908006/diff/15001/16002
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/1908006/diff/15001/16002#newcode1181
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1181:
renderWidgetHostView_->render_widget_host_->ImeSetInputMode(true);
On 2010/05/07 17:02:37, James Su wrote:
> On 2010/05/07 00:13:04, shess wrote:
> > Should this be true ANY time we have key focus?  The old code seemed wrong
> > because it never reset this.  But AFAICT it never set it until you had
marked
> > text.  [I do not know how it should really work.]
> I think we need to enable it as soon as we get the focus, otherwise the
renderer
> even won't send us the ime enable/disable event. This is necessary for fixing
> issue 23219 and 41876.

I understand the Windows code even less than this code, but it seems to only set
this in certain circumstances.  See
RenderWidgetHostViewWin::OnInputLangChange().  AFAICT if your input language is
English, you will not set this.  It's unclear how these things should map to
Mac, but we shouldn't degrade languages which don't have IMEs at all.

[James Su, 2010-05-07 21:14:05]

On 2010/05/07 17:09:31, shess wrote:
> http://codereview.chromium.org/1908006/diff/15001/16002
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
> http://codereview.chromium.org/1908006/diff/15001/16002#newcode1181
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1181:
> renderWidgetHostView_->render_widget_host_->ImeSetInputMode(true);
> On 2010/05/07 17:02:37, James Su wrote:
> > On 2010/05/07 00:13:04, shess wrote:
> > > Should this be true ANY time we have key focus?  The old code seemed wrong
> > > because it never reset this.  But AFAICT it never set it until you had
> marked
> > > text.  [I do not know how it should really work.]
> > I think we need to enable it as soon as we get the focus, otherwise the
> renderer
> > even won't send us the ime enable/disable event. This is necessary for
fixing
> > issue 23219 and 41876.
> 
> I understand the Windows code even less than this code, but it seems to only
set
> this in certain circumstances.  See
> RenderWidgetHostViewWin::OnInputLangChange().  AFAICT if your input language
is
> English, you will not set this.  It's unclear how these things should map to
> Mac, but we shouldn't degrade languages which don't have IMEs at all.

On Mac, all languages are handled in the same way. Even for English keyboard,
the input still goes through the input method related APIs. It even can have
marked text. The input manager is tightly integrated with the Cocoa system and
always activated for any process, as long as you call -interpretKeyEvents: the
events will always be dispatched to the input manager and handled in a unified
way.

[Scott Hess - ex-Googler, 2010-05-07 21:18:30]

I'm not entirely sure that answers my point, because it would probably
be easier on Windows to just enable the renderer/browser interaction
and suppress at the browser.  My concern is whether this causes a
bunch of back-and-forth communication which otherwise would not be
happening, and would slow stuff down, so it would be nice if there
were someone who knows about the Chrome site of this to sign off at a
high level on always turning that on when we have focus.

-scott


On Fri, May 7, 2010 at 2:14 PM,  <suzhe@chromium.org> wrote:
> On 2010/05/07 17:09:31, shess wrote:
>>
>> http://codereview.chromium.org/1908006/diff/15001/16002
>> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):
>
>> http://codereview.chromium.org/1908006/diff/15001/16002#newcode1181
>> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1181:
>> renderWidgetHostView_->render_widget_host_->ImeSetInputMode(true);
>> On 2010/05/07 17:02:37, James Su wrote:
>> > On 2010/05/07 00:13:04, shess wrote:
>> > > Should this be true ANY time we have key focus?  The old code seemed
>> > > wrong
>> > > because it never reset this.  But AFAICT it never set it until you had
>> marked
>> > > text.  [I do not know how it should really work.]
>> > I think we need to enable it as soon as we get the focus, otherwise the
>> renderer
>> > even won't send us the ime enable/disable event. This is necessary for
>
> fixing
>>
>> > issue 23219 and 41876.
>
>> I understand the Windows code even less than this code, but it seems to
>> only
>
> set
>>
>> this in certain circumstances.  See
>> RenderWidgetHostViewWin::OnInputLangChange().  AFAICT if your input
>> language
>
> is
>>
>> English, you will not set this.  It's unclear how these things should map
>> to
>> Mac, but we shouldn't degrade languages which don't have IMEs at all.
>
> On Mac, all languages are handled in the same way. Even for English
> keyboard,
> the input still goes through the input method related APIs. It even can have
> marked text. The input manager is tightly integrated with the Cocoa system
> and
> always activated for any process, as long as you call -interpretKeyEvents:
> the
> events will always be dispatched to the input manager and handled in a
> unified
> way.
>
>
> http://codereview.chromium.org/1908006/show
>

[James Su, 2010-05-07 22:02:42]

On 2010/05/07 21:18:30, shess wrote:
> I'm not entirely sure that answers my point, because it would probably
> be easier on Windows to just enable the renderer/browser interaction
> and suppress at the browser.  My concern is whether this causes a
> bunch of back-and-forth communication which otherwise would not be
> happening, and would slow stuff down, so it would be nice if there
> were someone who knows about the Chrome site of this to sign off at a
> high level on always turning that on when we have focus.

Yes, this may cause a bunch of useless communication. But among that
communication, the input method enable/disable events are really required for
browser to support some features (see issue 23219 and 41876) correctly.

Of course we can only enable it when there is a marked text, but that way we
must enable the input method all the time, which causes the other two issues I
mentioned above. And we even can't know if the input method is already activated
and requires input method related states (especially the caret position) before
having any marked text.

Actually, Chrome Linux also enables/disables it when getting or losing focus.

> 
> -scott
> 
> 
> On Fri, May 7, 2010 at 2:14 PM,  <mailto:suzhe@chromium.org> wrote:
> > On 2010/05/07 17:09:31, shess wrote:
> >>
> >> http://codereview.chromium.org/1908006/diff/15001/16002
> >> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):
> >
> >> http://codereview.chromium.org/1908006/diff/15001/16002#newcode1181
> >> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1181:
> >> renderWidgetHostView_->render_widget_host_->ImeSetInputMode(true);
> >> On 2010/05/07 17:02:37, James Su wrote:
> >> > On 2010/05/07 00:13:04, shess wrote:
> >> > > Should this be true ANY time we have key focus? &nbsp;The old code
seemed
> >> > > wrong
> >> > > because it never reset this. &nbsp;But AFAICT it never set it until you
had
> >> marked
> >> > > text. &nbsp;[I do not know how it should really work.]
> >> > I think we need to enable it as soon as we get the focus, otherwise the
> >> renderer
> >> > even won't send us the ime enable/disable event. This is necessary for
> >
> > fixing
> >>
> >> > issue 23219 and 41876.
> >
> >> I understand the Windows code even less than this code, but it seems to
> >> only
> >
> > set
> >>
> >> this in certain circumstances. &nbsp;See
> >> RenderWidgetHostViewWin::OnInputLangChange(). &nbsp;AFAICT if your input
> >> language
> >
> > is
> >>
> >> English, you will not set this. &nbsp;It's unclear how these things should
map
> >> to
> >> Mac, but we shouldn't degrade languages which don't have IMEs at all.
> >
> > On Mac, all languages are handled in the same way. Even for English
> > keyboard,
> > the input still goes through the input method related APIs. It even can have
> > marked text. The input manager is tightly integrated with the Cocoa system
> > and
> > always activated for any process, as long as you call -interpretKeyEvents:
> > the
> > events will always be dispatched to the input manager and handled in a
> > unified
> > way.
> >
> >
> > http://codereview.chromium.org/1908006/show
> >
>

[James Su, 2010-05-10 20:56:47]

In order to catch up with the release schedule of M5. I landed this CL on trunk
today, so that it can be included in this week's dev channel release and baked
for enough time.

Please continue to review this CL and I'll fix upcoming issues in new CLs.

Thanks
James Su