Reverts my r4300....

Reverts my r4300 and re-fixes Issue 3156 and 13500.
My r4300, which is a fix Issue 3156, has been causing regressions (Issue 9596, Issue 13500, Issue 29290, etc.)
Every time when this change caused regressions, we added more code to Chromium and made Chromium code more complicated.
So, I think it is better to revert this change once and find another solution for Issue 3156 rather than to add another workaround for Issue 29290.

To look into Issue 3156 more deeply, it is caused by a recursive message-handler call.
When SetWindowText() is called while an IME is composing text, the IME calls SendMessage() to send a WM_IME_COMPOSITION message.
When we receive this WM_IME_COMPOSITION message, it updates the omnibox and calls SetWindowText()...
This recursive call caused not only Issue 3156 but also caused some other IME issues, such as Issue 13500.

BUG=3156, 13500, 29290
TEST=On XP, open a new tab, set the IME to Hiragana, type 'c', and then click an entry in the popup. The browser should navigate and show the popup URL without a crash.
TEST=Open Gmail, compose a new e-mail (without using a tear-off window), set the IME to Hiragana, type 'c' in its message body, and wait for the draft auto-saving. The browser should show the 'c' character.

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

[Peter Kasting, 2009-12-04 06:13:00]

Do you have an idea of how to fix this in a different way?  I'm not sure it's
better to revert if not.

If you are going to revert, make sure to revert all the bits we have added
connected to this.  For example, you need to revert the rest of the
ignore_ime_messages_ references (I think).

[Hironori Bono, 2009-12-04 11:53:37]

Thank you for your comments.
I should have merged my new fix for Issue 3156.
To look into Issue 3156 deeply these days, I noticed this issue was caused by a
recursive message-handler call. When we call SetWindowText(), i.e.
SendMessage(WM_SETTEXT,...), while an IME is composing text, it calls
SendMessage(WM_IME_COMPOSITION,...). When we receive this WM_IME_COMPOSITION
message, we update the omnibox and maybe call SetWindowText() again...
As you know, our previous canceled an IME composition but it didn't fix the
above recursive call and caused side-effects. So, I thought it was better to
stop this recursive call than to cancel an IME composition.
I have merged my new fix for Issue 3156 to this change. Is it possible to review
the updated change again?

Regards,

Hironori Bono

[Peter Kasting, 2009-12-04 22:22:56]

http://codereview.chromium.org/467015/diff/1002/2003
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (left):

http://codereview.chromium.org/467015/diff/1002/2003#oldcode625
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:625: // control. To
avoid this, we force the IME to cancel any outstanding
Now that we're not explicitly canceling, do things still work correctly on
Windows XP, i.e. do we somehow manage to cancel the composition?  You new code
and comments no longer touch on this Vista/XP difference and how it's handled.

http://codereview.chromium.org/467015/diff/1002/2003
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/467015/diff/1002/2003#newcode1701
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1701: // this
WM_IME_COMPOSITION message, we update an omnibox and may call
Nit: "an" -> "the"

http://codereview.chromium.org/467015/diff/1002/2003#newcode1703
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1703: // stop updating
the omnibox while we processes a WM_SETTEXT message.
Nit: "processes" -> "process"

I don't know if you want to also add a motivating clause/sentence at the end,
e.g. "We wouldn't need to do this update anyway, because either we're in the
middle of updating the omnibox already or the caller of SetWindowText() is going
to update the omnibox next."

http://codereview.chromium.org/467015/diff/1002/2003#newcode1704
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1704:
ignore_ime_messages_ = true;
Nit: Can you modify this to use the AutoReset object (available in
base/auto_reset.h) instead of the explicit toggles?

[Hironori Bono, 2009-12-07 09:53:30]

Thank you for your perceptive and great comments.

http://codereview.chromium.org/467015/diff/1002/2003
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (left):

http://codereview.chromium.org/467015/diff/1002/2003#oldcode625
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:625: // control. To
avoid this, we force the IME to cancel any outstanding
On 2009/12/04 22:22:56, Peter Kasting wrote:
> Now that we're not explicitly canceling, do things still work correctly on
> Windows XP, i.e. do we somehow manage to cancel the composition?  Your new
code
> and comments no longer touch on this Vista/XP difference and how it's handled.

In brief, it seems we don't need to cancel the composition any longer on Windows
XP. That is, Windows XP can also automatically cancel the composition when we
update the text.

(The below is a long excuse of mine.)

Last week, I re-investigated how our original code (i.e. the code before my
r4300) worked to check Issue 3156, 13500, and 29290.
Before r4300, Chrome worked as listed below when we click a popup item while an
IME is composing text.

1. Chrome sends a WM_SETTEXT message (e.g. "amazon.com/");
2. Chrome returns an error in processing this WM_SETTEXT message.

To fix the error in the above step 2, I wrote my r4300. But, it changed the
behavior of Chrome as listed below, and caused Issue 13500. (This issue is fixed
by your change that prevents the step 4.)

1. Chrome cancels an ongoing composition;
2. IME sends a WM_IME_COMPOSITION message;
3. Chrome starts processing this WM_IME_COMPOSITION message;
4. Chrome starts updating the omnibox;
5. Chrome finishes processing the WM_IME_COMPOSITION message;
6. Chrome crashes because of the update in Step 4. (Issue 13500)

On the other hand, when I reverted my r4300 from the latest Chrome and tested
this binary with my WinXP PC, it behavior became as listed below.

1. Chrome sends a WM_SETTEXT message (e.g. "amazon.com/");
2. Chrome starts processing this WM_SETTEXT message;
3. IME sends a WM_IME_COMPOSITION message;
4. Chrome starts processing this WM_IME_COMPOSITION message;
5. Chrome starts updating the omnibox;
6. Chrome finishes processing the WM_IME_COMPOSITION message;
7. Chrome finishes processing the WM_SETTEXT message;
8. Chrome crashes. (Issue 13500)

Even though I cannot tell which change of Chrome (or Microsoft) fixed Issue
3156, I could not reproduce Issue 3156 any longer even when I reverted my r4300.
(But I can still reproduce Issue 13500.) Also, my cancellation code causes Issue
29290. So, I wrote this change that prevents Chrome from updating the omnibox at
the above step 5.

http://codereview.chromium.org/467015/diff/1002/2003
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/467015/diff/1002/2003#newcode1701
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1701: // this
WM_IME_COMPOSITION message, we update an omnibox and may call
On 2009/12/04 22:22:56, Peter Kasting wrote:
> Nit: "an" -> "the"

Done.

http://codereview.chromium.org/467015/diff/1002/2003#newcode1703
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1703: // stop updating
the omnibox while we processes a WM_SETTEXT message.
On 2009/12/04 22:22:56, Peter Kasting wrote:
> Nit: "processes" -> "process"
> 
> I don't know if you want to also add a motivating clause/sentence at the end,
> e.g. "We wouldn't need to do this update anyway, because either we're in the
> middle of updating the omnibox already or the caller of SetWindowText() is
going
> to update the omnibox next."

Done. Thank you for your comment. I like this motivating comment.

http://codereview.chromium.org/467015/diff/1002/2003#newcode1704
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1704:
ignore_ime_messages_ = true;
On 2009/12/04 22:22:56, Peter Kasting wrote:
> Nit: Can you modify this to use the AutoReset object (available in
> base/auto_reset.h) instead of the explicit toggles?

Done. Thank you for noticing this class. It is great. :)

[Peter Kasting, 2009-12-07 19:47:08]

LGTM.  Your explanation makes me wonder if we'll be broken on XP < SP3, or on
certain IMEs, or something, but I don't have the ability to verify that, so
let's see what happens.