Hide IME autocompletion when Omnibox popup closes.

Hide IME autocompletion when Omnibox popup closes.

Tgus CL clears the text of the IME inline autocompletion when Omnibox popup gets closed.

BUG=310407
TEST=Test manually with --enable-omnibox-auto-completion-for-ime command line flag.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235694

[Yuki, 2013-11-12 09:01:52]

Could you review this CL?

[Peter Kasting, 2013-11-12 21:22:02]

This seems like the wrong fix.

We shouldn't be needing to treat IME autocompletion specially when closing the
omnibox popup, and I wouldn't think we'd necessarily always want it to disappear
when the popup closes anyway.

I would think the IME autocompletion should be cleared at the same time normal
autocompeltions would be cleared.  Then doing something like navigating by
clicking a popup result should clear the IME autocompletion by way of the revert
it already performs.

[Yuki, 2013-11-14 13:44:33]

Thanks for the comment.  I agree with you, and updated the code.

Since the inline autocompletion is implemented as text selection, I clear the
IME inline autocompletion when the text selection is changed.

Could you take another look?

[Peter Kasting, 2013-11-14 21:53:31]

I think this fix still isn't right.  I think we actually would be better served
by directly hooking the places where the edit model can change
|inline_autocomplete_text_|.

So here's my suggestion:
(1) Add a new OmniboxView function, OnInlineAutocompleteTextCleared().  This is
a no-op for non-views, and for views unconditionally hides the IME
autocompletion label.
(2) Insert calls to this function in the edit model in the three places that
touch |inline_autocomplete_text_|:
(i) In OnPopupDataChanged() after clear()ing it.
(ii) In OnPopupDataChanged() after assigning to it, conditionally if we've just
set it to the empty string.
(iii) In InternalSetUserText() after clear()ing it.

Combined with the unchanged existing code in
OnInlineAutocompleteTextMaybeChanged(), I think this should show and hide this
label at all the right times.

[Yuki, 2013-11-15 06:25:54]

Thanks for the thoughtful and detailed comments.
Addressed your comments.

[Peter Kasting, 2013-11-15 23:23:05]

LGTM.  Can this be tested somehow?  It would be nice to have tests to verify
that we don't break this in the future.

https://codereview.chromium.org/68103008/diff/330001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/68103008/diff/330001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view.h:178: // Called when the inline
autocomplete text in the model has cleared.
Nit: has been cleared

[Yuki, 2013-11-18 06:10:35]

> LGTM.  Can this be tested somehow?  It would be nice to have tests to verify
> that we don't break this in the future.

Let me add a unittest in a separate CL.
I think I can add a testcase in
c/b/ui/views/omnibox/omnibox_view_views_browsertest.cc .

https://codereview.chromium.org/68103008/diff/330001/chrome/browser/ui/omnibo...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/68103008/diff/330001/chrome/browser/ui/omnibo...
chrome/browser/ui/omnibox/omnibox_view.h:178: // Called when the inline
autocomplete text in the model has cleared.
On 2013/11/15 23:23:05, Peter Kasting wrote:
> Nit: has been cleared

Done.

[commit-bot: I haz the power, 2013-11-18 06:11:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/68103008/440001

[commit-bot: I haz the power, 2013-11-18 08:44:41]

Retried try job too often on win_rel for step(s) browser_tests,
chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests,
content_browsertests, mini_installer_test, sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-18 08:50:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/68103008/440001

[commit-bot: I haz the power, 2013-11-18 11:05:55]

Change committed as 235694