A fix for Issue 3156 in chromium: "OmniBox: NavSuggest doesn't work fine when...

A fix for Issue 3156 in chromium: "OmniBox: NavSuggest doesn't work fine when you select NavSuggest suggested item before finalizing IME."To investigate this issue today, a rich-edit control of Windows XP does not finish an ongoing composition before changing its text with a SetWindowText() call. (Thanks pkasting for colleting my corrupted comments.)To solve this issue, this change manually finishes an ongoing composition before calling the function.

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

[Peter Kasting, 2008-10-28 20:12:00]

http://codereview.chromium.org/8669/diff/1/2
File chrome/browser/autocomplete/autocomplete_popup.cc (right):

http://codereview.chromium.org/8669/diff/1/2#newcode810
Line 810: // A rich-edit control of Windows XP doesn't finish an IME composition
I'm not sure this comment is totally accurate.  Now that we've made the popup
window unable to be activated, the CRichEditCtrl isn't really losing input focus
when the user clicks or arrows around the popup, is it?  In which case it makes
sense that the IME composition wouldn't be finished.

Also seems like the ReplaceSel() comment is a bit of a red herring since
ReplaceSel() isn't being called in the chain after this -- SetWindowText() is.

It seems like maybe this code belongs in the edit view, perhaps in
SetWindowTextAndCaretPos() or some similar low-level function?

http://codereview.chromium.org/8669/diff/1/2#newcode821
Line 821: HIMC imm_context = ::ImmGetContext(view_->m_hWnd);
Nit: No "::" qualifier needed on any of these calls

Shouldn't you be getting the context from the edit view HWND, rather than the
popup view?  I realize they're probably the same, but it seems more technically
correct the other way.

[Hironori Bono, 2008-10-28 21:39:06]

Thank you for your quick response.
My last change is embarrassing. 

On 2008/10/28 20:12:00, pkasting wrote:
> http://codereview.chromium.org/8669/diff/1/2
> File chrome/browser/autocomplete/autocomplete_popup.cc (right):
> 
> http://codereview.chromium.org/8669/diff/1/2#newcode810
> Line 810: // A rich-edit control of Windows XP doesn't finish an IME
composition
> I'm not sure this comment is totally accurate.  Now that we've made the popup
> window unable to be activated, the CRichEditCtrl isn't really losing input
focus
> when the user clicks or arrows around the popup, is it?  In which case it
makes
> sense that the IME composition wouldn't be finished.
> 
> Also seems like the ReplaceSel() comment is a bit of a red herring since
> ReplaceSel() isn't being called in the chain after this -- SetWindowText() is.
> 
> It seems like maybe this code belongs in the edit view, perhaps in
> SetWindowTextAndCaretPos() or some similar low-level function?

Sorry for my corrupted comments and thank you for noticing this. You are
definitely right.
(I have also noticed my comments are stupid after sending the previous review
request.)
I have moved my change to the SetWindowTextAndCaretPos() function and updated my
stupid comments.

> http://codereview.chromium.org/8669/diff/1/2#newcode821
> Line 821: HIMC imm_context = ::ImmGetContext(view_->m_hWnd);
> Nit: No "::" qualifier needed on any of these calls

Done. I have removed the qualifiers in my change.

> Shouldn't you be getting the context from the edit view HWND, rather than the
> popup view?  I realize they're probably the same, but it seems more
technically
> correct the other way.

Since I have moved my change to the AutocompleteEditView class, this change now
uses the context of its HWND.

Is it possible to review it again?

Regards,

Hironori Bono
E-mail: hbono@chromium.org

[Peter Kasting, 2008-10-28 22:42:36]

http://codereview.chromium.org/8669/diff/4/204
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/8669/diff/4/204#newcode308
Line 308: view_->SetWindowText(L"");
Do we need your fix here (which might mean, do we need to change this to a
SetWindowTextAndCaretPos() call)?  You should try typing in the name of a
keyword and hitting <tab> to see whether the IME composition window does what
you want... might be good to put in a comment either way.

http://codereview.chromium.org/8669/diff/4/204#newcode900
Line 900: // A rich-edit control of Windows XP does not finish an ongoing
composition
Nit: The English here is a bit unclear, so I propose this comment:

"In Windows Vista, SetWindowText() automatically completes any ongoing IME
composition, and updates the text of the underlying edit control.  In Windows
XP, however, SetWindowText() gets applied to the IME composition string if it
exists, and doesn't update the underlying edit control.  To avoid this, we force
the IME to complete any outstanding compositions here.  This is harmless in
Vista and in cases where the IME isn't composing."

I also suggest placing the comment inside the conditional rather than above it.

http://codereview.chromium.org/8669/diff/4/204#newcode912
Line 912: }
Nit: I'd add a newline after this block

http://codereview.chromium.org/8669/diff/4/204#newcode1060
Line 1060: SetWindowText(display_text.c_str());
Nit: Should perhaps add a comment here noting that the fix in
SetWindowTextAndCaretPos() is not needed here because UpdatePopup() disables
inline autocompletion when doing IME compositions, so we should never reach here
with the compose window open.

[Hironori Bono, 2008-10-29 18:09:05]

http://codereview.chromium.org/8669/diff/4/204
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/8669/diff/4/204#newcode308
Line 308: view_->SetWindowText(L"");
On 2008/10/28 22:42:37, pkasting wrote:
> Do we need your fix here (which might mean, do we need to change this to a
> SetWindowTextAndCaretPos() call)?  You should try typing in the name of a
> keyword and hitting <tab> to see whether the IME composition window does what
> you want... might be good to put in a comment either way.

Thank you for noticing this. As written in my comments here, IME eats tab keys
and rich-edit control cannot receive them while composing a text. So, I added a
comment noting this.

http://codereview.chromium.org/8669/diff/4/204#newcode900
Line 900: // A rich-edit control of Windows XP does not finish an ongoing
composition
On 2008/10/28 22:42:37, pkasting wrote:
> Nit: The English here is a bit unclear, so I propose this comment:
> 
> "In Windows Vista, SetWindowText() automatically completes any ongoing IME
> composition, and updates the text of the underlying edit control.  In Windows
> XP, however, SetWindowText() gets applied to the IME composition string if it
> exists, and doesn't update the underlying edit control.  To avoid this, we
force
> the IME to complete any outstanding compositions here.  This is harmless in
> Vista and in cases where the IME isn't composing."
> 
> I also suggest placing the comment inside the conditional rather than above
it.

Done. Thank you for correcting my ambiguous English.

http://codereview.chromium.org/8669/diff/4/204#newcode912
Line 912: }
On 2008/10/28 22:42:37, pkasting wrote:
> Nit: I'd add a newline after this block

Done.

http://codereview.chromium.org/8669/diff/4/204#newcode1060
Line 1060: SetWindowText(display_text.c_str());
On 2008/10/28 22:42:37, pkasting wrote:
> Nit: Should perhaps add a comment here noting that the fix in
> SetWindowTextAndCaretPos() is not needed here because UpdatePopup() disables
> inline autocompletion when doing IME compositions, so we should never reach
here
> with the compose window open.

Done. Thank you for noticing this.

[Peter Kasting, 2008-10-29 18:22:13]

LGTM with suggested comment fixes below.  Thanks!

http://codereview.chromium.org/8669/diff/7/207
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/8669/diff/7/207#newcode308
Line 308: // We don't have to manually complete ongoing compositoin here because
Nit: How about this comment, for readability:

"NOTE: We don't need the IME composition hack in SetWindowTextAndCaretPos()
here, because any active IME composition will eat <tab> characters, preventing
the user from using tab-to-search until the composition is ended."

http://codereview.chromium.org/8669/diff/7/207#newcode1063
Line 1063: // We don't have to manually complete ongoing IME composition here
because
Nit: Similarly, I suggest:

"NOTE: We don't need the IME composition hack in SetWindowTextAndCaretPos()
here, because UpdatePopup() disables inline autocomplete when a composition is
in progress, thus preventing us from reaching this code."