Omnibox selects all text on tap.

Omnibox selects all text on tap.

Previously, ET_GESTURE_SHOW_PRESS was causing the omnibox to not select
all text. ET_GESTURE_SHOW_PRESS is now ignored by the omnibox.

BUG=313135
TEST=OmniboxViewViewsTest.SelectAllOnTap

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

[tdresser, 2013-11-01 16:48:55]

PTAL, thanks!

[tdresser, 2013-11-01 16:52:18]

Changed reviewer - jochen OOO.

[tdresser, 2013-11-04 21:08:48]

On 2013/11/01 16:52:18, tdresser wrote:
> Changed reviewer - jochen OOO.

@pkasting, PTAL.

[Peter Kasting, 2013-11-04 21:23:07]

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:18: #include
"ui/aura/root_window.h"
Don't we need to guard Aura-specific includes and code with the appropriate
preprocessor directive?  After all, one can still build views without Aura.

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:60:
rwhd_->OnHostTouchEvent(&press);
Tiny nit: It may make slightly more sense to interleave:

    ui::TouchEvent press(...);
    rwhd_->OnHostTouchEvent(&press);

    ui::TouchEvent release(...);
    rwhd_->OnHostTouchEvent(&release);

(2 places)

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:88: gfx::Point
press_point = omnibox_origin + press_offset;
Nit: I'd probably inline these two temps into the next two statements.

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:111:
aura::RootWindowHostDelegate* rwhd_;
Nit: Does this need to be a member?  Can't we just calculate it in the two
functions that need it?

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:195: const
gfx::Vector2d tap(40, 10);
Could this offset be outside the omnibox in a future omnibox update or on any
particular platforms?  Maybe instead of hardcoding 40,10 you should find the
omnibox center point or something.  Similar concerns apply to the other vectors
below.

Nit: |tap_offset| would be a better name than |tap|.

[tdresser, 2013-11-05 15:27:59]

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:18: #include
"ui/aura/root_window.h"
On 2013/11/04 21:23:07, Peter Kasting wrote:
> Don't we need to guard Aura-specific includes and code with the appropriate
> preprocessor directive?  After all, one can still build views without Aura.

Done. Are you aware of any bots which build views without Aura?
I can't compile this configuration on linux, and it sounds like all windows bots
now use Aura.

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:60:
rwhd_->OnHostTouchEvent(&press);
On 2013/11/04 21:23:07, Peter Kasting wrote:
> Tiny nit: It may make slightly more sense to interleave:
> 
>     ui::TouchEvent press(...);
>     rwhd_->OnHostTouchEvent(&press);
> 
>     ui::TouchEvent release(...);
>     rwhd_->OnHostTouchEvent(&release);
> 
> (2 places)

Done.

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:88: gfx::Point
press_point = omnibox_origin + press_offset;
On 2013/11/04 21:23:07, Peter Kasting wrote:
> Nit: I'd probably inline these two temps into the next two statements.

Done.

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:111:
aura::RootWindowHostDelegate* rwhd_;
On 2013/11/04 21:23:07, Peter Kasting wrote:
> Nit: Does this need to be a member?  Can't we just calculate it in the two
> functions that need it?

Done.

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:195: const
gfx::Vector2d tap(40, 10);
On 2013/11/04 21:23:07, Peter Kasting wrote:
> Could this offset be outside the omnibox in a future omnibox update or on any
> particular platforms?  Maybe instead of hardcoding 40,10 you should find the
> omnibox center point or something.  Similar concerns apply to the other
vectors
> below.
> 
> Nit: |tap_offset| would be a better name than |tap|.

Done. I changed this in the click test as well.

[Peter Kasting, 2013-11-05 19:54:07]

LGTM

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
File chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc (right):

https://codereview.chromium.org/55893004/diff/1/chrome/browser/ui/views/omnib...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:18: #include
"ui/aura/root_window.h"
On 2013/11/05 15:28:00, tdresser wrote:
> On 2013/11/04 21:23:07, Peter Kasting wrote:
> > Don't we need to guard Aura-specific includes and code with the appropriate
> > preprocessor directive?  After all, one can still build views without Aura.
> 
> Done. Are you aware of any bots which build views without Aura?

We've switched the bots over to not doing this, but it's still possible for
engineers to build this way.  That will probably change over the next several
months.

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

https://codereview.chromium.org/55893004/diff/170001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:78: 5,
base::TimeDelta::FromMilliseconds(0));
Nit: You interleaved the press/release calls in Tap(), but not in this function.

https://codereview.chromium.org/55893004/diff/170001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:149: const
gfx::Point click_location = omnibox_bounds.CenterPoint();
Nit: Define this just above first use.

I'd probably move both |omnibox_bounds| and |click_location| down to just under
the "Clicking in the omnibox should take focus and select all text." comment,
and remove the blank lines around them.

https://codereview.chromium.org/55893004/diff/170001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:207: const
gfx::Point tap_location = omnibox_bounds.CenterPoint();
Nit: Same comments as above.

https://codereview.chromium.org/55893004/diff/170001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:231: 
Nit: This blank line and the one below aren't necessary (and you don't have them
in the similar location in the previous test)

[tdresser, 2013-11-05 20:18:20]

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

https://codereview.chromium.org/55893004/diff/170001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:78: 5,
base::TimeDelta::FromMilliseconds(0));
On 2013/11/05 19:54:07, Peter Kasting wrote:
> Nit: You interleaved the press/release calls in Tap(), but not in this
function.

Done.

https://codereview.chromium.org/55893004/diff/170001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:149: const
gfx::Point click_location = omnibox_bounds.CenterPoint();
On 2013/11/05 19:54:07, Peter Kasting wrote:
> Nit: Define this just above first use.
> 
> I'd probably move both |omnibox_bounds| and |click_location| down to just
under
> the "Clicking in the omnibox should take focus and select all text." comment,
> and remove the blank lines around them.

Done.

https://codereview.chromium.org/55893004/diff/170001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:207: const
gfx::Point tap_location = omnibox_bounds.CenterPoint();
On 2013/11/05 19:54:07, Peter Kasting wrote:
> Nit: Same comments as above.

Done.

https://codereview.chromium.org/55893004/diff/170001/chrome/browser/ui/views/...
chrome/browser/ui/views/omnibox/omnibox_view_views_browsertest.cc:231: 
On 2013/11/05 19:54:07, Peter Kasting wrote:
> Nit: This blank line and the one below aren't necessary (and you don't have
them
> in the similar location in the previous test)

Done.

[commit-bot: I haz the power, 2013-11-06 17:07:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tdresser@chromium.org/55893004/410001

[commit-bot: I haz the power, 2013-11-06 20:30:40]

Change committed as 233342