A quick fix for Issue 3798....

A quick fix for Issue 3798.
Some Chinese IMEs insert whitespace characters (U+3000) instead of input characters while they are composing text. So, trimming these whitespace characters (at the beginning of an omnibox) prevents from inputting text on these IMEs. As a quick fix, this change prevent our AutocompleteEditViewWin::OnImeComposition() from starting auto-complete when an omnibox starts with whitespace characters.

BUG=3798
TEST=Type "google.com" on Omnibox, type "Tab" to start tab-to-search, type 'nihao' on Microsoft New Phonetic IME, and see we can see Chinese characters in the omnibox.

[Peter Kasting, 2009-11-16 08:04:11]

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

http://codereview.chromium.org/391070/diff/1/2#newcode455
Line 455: memset(&cf, 0, sizeof(cf));
Why is this block needed?

http://codereview.chromium.org/391070/diff/1/2#newcode470
Line 470: // Call SetInputScope().
Can you add comments?  Reading the MSDN docs on SetInputScope() does not tell me
what the precise effect of this is.  Also, is IS_URL appropriate for an input
that frequently contains freeform text?

I'm also concerned about the potential perf impact of loading another DLL on the
UI thread during browser startup (when this will be created).  Is there a set of
conditions under which this isn't needed, or some other way to mitigate the cost
here?  Maybe I'm worrying needlessly.

http://codereview.chromium.org/391070/diff/1/2#newcode475
Line 475: GetProcAddress(module, "SetInputScope"));
Nit: Indent 4

http://codereview.chromium.org/391070/diff/1/2#newcode1274
Line 1274: // beginning of this control (in OnAfterPossibleChange()) prevents
from
Nit: "prevents" -> "prevents users"

http://codereview.chromium.org/391070/diff/1/2#newcode1279
Line 1279: if (TrimWhitespace(GetText(), TRIM_ALL, &text) & TRIM_LEADING)
Nit: Do you need to pass TRIM_ALL or can you just pass TRIM_LEADING?

[Hironori Bono, 2009-11-16 08:29:40]

Thank you so much for your quick comments and sorry for my bonehead mistakes.
As you can see, I noticed my previous change included some garbages I forgot
deleting before uploading this change.
I have removed the garbages and applied your comments in OnImeComposition().
Would it be possible to review the updated one?

Regards,

Hironori Bono

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

http://codereview.chromium.org/391070/diff/1/2#newcode455
Line 455: memset(&cf, 0, sizeof(cf));
On 2009/11/16 08:04:11, Peter Kasting wrote:
> Why is this block needed?

Thank you for noticing this and sorry for my bonehead mistake.
This is a garbage I forgot deleting before uploading this change. I have deleted
this block.

http://codereview.chromium.org/391070/diff/1/2#newcode470
Line 470: // Call SetInputScope().
On 2009/11/16 08:04:11, Peter Kasting wrote:
> Can you add comments?  Reading the MSDN docs on SetInputScope() does not tell
me
> what the precise effect of this is.  Also, is IS_URL appropriate for an input
> that frequently contains freeform text?
> 
> I'm also concerned about the potential perf impact of loading another DLL on
the
> UI thread during browser startup (when this will be created).  Is there a set
of
> conditions under which this isn't needed, or some other way to mitigate the
cost
> here?  Maybe I'm worrying needlessly.

Thank you for this comment and sorry for my bonehead mistake. This block is also
a garbage I forgot deleting before uploading this change. I have deleted this
block.

http://codereview.chromium.org/391070/diff/1/2#newcode1274
Line 1274: // beginning of this control (in OnAfterPossibleChange()) prevents
from
On 2009/11/16 08:04:11, Peter Kasting wrote:
> Nit: "prevents" -> "prevents users"

Done.
Thank you for your comment.

http://codereview.chromium.org/391070/diff/1/2#newcode1279
Line 1279: if (TrimWhitespace(GetText(), TRIM_ALL, &text) & TRIM_LEADING)
On 2009/11/16 08:04:11, Peter Kasting wrote:
> Nit: Do you need to pass TRIM_ALL or can you just pass TRIM_LEADING?

Done.
Thank you for your comment.

[Peter Kasting, 2009-11-16 08:38:52]

Don't be so hard on yourself!

http://codereview.chromium.org/391070/diff/3001/2002
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/391070/diff/3001/2002#newcode1263
Line 1263: if (TrimWhitespace(GetText(), TRIM_LEADING, &text) & TRIM_LEADING)
My one concern here is whether it's possible/necessary to distinguish the
particular whitespace characters these IMEs insert from other whitespace.  For
example, if a user hits the spacebar and then composes some text with an IME,
will we autocomplete when we're supposed to, or not?

[Hironori Bono, 2009-11-16 11:13:54]

Thank you for your review and comment.
It seems my brain doesn't work well today. :)
As written in the reply to your comment, I have updated this change to solve
your concern as much as possible.
Would it be possible to review the updated change?

Regards,

Hironori Bono

http://codereview.chromium.org/391070/diff/3001/2002
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/391070/diff/3001/2002#newcode1263
Line 1263: if (TrimWhitespace(GetText(), TRIM_LEADING, &text) & TRIM_LEADING)
On 2009/11/16 08:38:52, Peter Kasting wrote:
> My one concern here is whether it's possible/necessary to distinguish the
> particular whitespace characters these IMEs insert from other whitespace.  For
> example, if a user hits the spacebar and then composes some text with an IME,
> will we autocomplete when we're supposed to, or not?

Thank you for noticing this.
As you concerned, I noticed my previous change didn't start autocomplete in the
said example with one IME, MS Japanese IME. (On the other hand, it starts
autocomple when we use the other  IMEs.)
To distinguish the whitespace characters, I updated my change to read the
composition string (i.e. dotted-underlined text) and check if it starts with a
whitespace character. From the point of an IME, it finishes a composition when
we change its composition string, i.e. it doesn't like for us to trim the
whitespace characters in its composition string.
As you can imagine, this change doesn't check the position of a composition
since I couldn't find good methods that work with all IMEs. Sorry.

[Peter Kasting, 2009-11-16 20:03:44]

LGTM with nit

http://codereview.chromium.org/391070/diff/2005/2006
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/391070/diff/2005/2006#newcode1267
Line 1267: scoped_array<wchar_t> text;
Nit: Use a std::wstring and string_util.h:WriteInto().

[Hironori Bono, 2009-11-17 09:22:52]

Thank you for your review and comment.
I have landed this change as r32166.

Regards,

Hironori Bono

http://codereview.chromium.org/391070/diff/2005/2006
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/391070/diff/2005/2006#newcode1267
Line 1267: scoped_array<wchar_t> text;
On 2009/11/16 20:03:44, Peter Kasting wrote:
> Nit: Use a std::wstring and string_util.h:WriteInto().

Done.