Deactivate omnibox touch editing on command execution

Deactivate omnibox touch editing on command execution

A Views textfield hides touch text selection handles on command
execution. Omnibox overrides command execution for some commands without
calling textfield's implementation; so, it needs to hide the handles for
those commands.
Also, introduced TextfieldTestApi class for accessing private members of
Textfield class in tests.

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

[mohsen, 2014-05-20 20:07:56]

Please take a look...

[sky, 2014-05-20 20:58:13]

https://codereview.chromium.org/297733002/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/297733002/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.h:306: friend class
::OmniboxViewViewsTest;
Ick. How about exposing a class, only compiled in tests, that is friended and
can access whatever all these classes need?

[mohsen, 2014-05-21 14:47:48]

Please take a look. I have created a TestApi class for Textfield. Do I need to
put this class in a test/ directory?

https://codereview.chromium.org/297733002/diff/1/ui/views/controls/textfield/...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/297733002/diff/1/ui/views/controls/textfield/...
ui/views/controls/textfield/textfield.h:306: friend class
::OmniboxViewViewsTest;
On 2014/05/20 20:58:14, sky wrote:
> Ick. How about exposing a class, only compiled in tests, that is friended and
> can access whatever all these classes need?

Done.

[sky, 2014-05-21 16:20:45]

You don't need to put the file in the test directory.

https://codereview.chromium.org/297733002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/297733002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:319:
DestroyTouchSelection();
Add a comment as to why the touch selection needs to be destroyed here.

https://codereview.chromium.org/297733002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/297733002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:309:
ui::ScopedClipboardWriter clipboard_writer(
Does this need to be in interactive ui tests since it's writing to the
clipboard? Interactive ui tests run serially, where as browser tests don't. This
means you could potentially have a race condition if someone else tried to
manipulate the clipboard.

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.h:16: #include
"testing/gtest/include/gtest/gtest_prod.h"
Why do you need this include?

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_test_api.cc (right):

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_test_api.cc:1: #include
"ui/views/controls/textfield/textfield_test_api.h"
copyright.

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_test_api.h (right):

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_test_api.h:13: class TextfieldTestApi {
I suspect you need VIEWS_EXPORT here.

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_test_api.h:37: };
DISALLOW_...

[mohsen, 2014-05-21 19:54:28]

https://codereview.chromium.org/297733002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/297733002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:319:
DestroyTouchSelection();
On 2014/05/21 16:20:45, sky wrote:
> Add a comment as to why the touch selection needs to be destroyed here.

Done.

https://codereview.chromium.org/297733002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/297733002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:309:
ui::ScopedClipboardWriter clipboard_writer(
On 2014/05/21 16:20:45, sky wrote:
> Does this need to be in interactive ui tests since it's writing to the
> clipboard? Interactive ui tests run serially, where as browser tests don't.
This
> means you could potentially have a race condition if someone else tried to
> manipulate the clipboard.

This file is actually part of interactive_ui_tests. I'm not sure why it is named
as a browser test.

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield.h:16: #include
"testing/gtest/include/gtest/gtest_prod.h"
On 2014/05/21 16:20:45, sky wrote:
> Why do you need this include?

Not needed. Residue of some tests, sorry! Removed.

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_test_api.cc (right):

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_test_api.cc:1: #include
"ui/views/controls/textfield/textfield_test_api.h"
On 2014/05/21 16:20:45, sky wrote:
> copyright.

Done.

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_test_api.h (right):

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_test_api.h:13: class TextfieldTestApi {
On 2014/05/21 16:20:45, sky wrote:
> I suspect you need VIEWS_EXPORT here.

Yep. Done.

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_test_api.h:37: };
On 2014/05/21 16:20:45, sky wrote:
> DISALLOW_...

Done.

[sky, 2014-05-21 21:02:02]

LGTM

[mohsen, 2014-05-21 21:47:31]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-05-21 21:51:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohsen@chromium.org/297733002/80001

[commit-bot: I haz the power, 2014-05-22 06:11:17]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[commit-bot: I haz the power, 2014-05-22 09:09:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-22 09:09:52]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[mohsen, 2014-05-22 14:53:23]

Can you take another look, please...

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
File ui/views/controls/textfield/textfield_test_api.h (right):

https://codereview.chromium.org/297733002/diff/60001/ui/views/controls/textfi...
ui/views/controls/textfield/textfield_test_api.h:13: class TextfieldTestApi {
On 2014/05/21 19:54:28, mohsen wrote:
> On 2014/05/21 16:20:45, sky wrote:
> > I suspect you need VIEWS_EXPORT here.
> 
> Yep. Done.

It seems that we don't need VIEWS_EXPORT here since views_test_support is
statically linked. (And if we needed, it should've been something other than
VIEWS_EXPORT or we'd get compile errors on Windows). So, I removed VIEWS_EXPORT.

[sky, 2014-05-23 17:53:58]

Ok, LGTM

[mohsen, 2014-05-23 17:58:31]

The CQ bit was checked by mohsen@chromium.org

[commit-bot: I haz the power, 2014-05-23 17:59:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mohsen@chromium.org/297733002/100001

[commit-bot: I haz the power, 2014-05-23 18:49:50]

Change committed as 272569