Fix omnibox mouse click highlight/word select/focus issue in aura build.

Fix omnibox mouse click highlight/word select/focus issue in aura build.

BUG=101774
TEST=NONE


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

[jennyz, 2011-11-11 01:13:48]

Please ignore the dirty code in metrics_aura.cc and view_aura.cc. The cl for
this change has not landed yet. I will sync to it later.

[msw, 2011-11-11 01:28:57]

Awesome, this looks pretty good.

http://codereview.chromium.org/8527015/diff/1/chrome/browser/ui/views/omnibox...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/8527015/diff/1/chrome/browser/ui/views/omnibox...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:275: if
(event.IsOnlyLeftMouseButton() && !textfield_->HasFocus() &&
We might want to do this for right mouse too. On Windows, the right mouse button
also selects all and takes focus; and then the context menu is shown.

http://codereview.chromium.org/8527015/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:97: if
(textfield_->OnMousePressed(event)) {
Please add a comment here like "Allow the textfield/omnibox to optionally handle
the mouse pressed event." Also, remove the braces for a single statement.

[oshima, 2011-11-11 01:52:32]

http://codereview.chromium.org/8527015/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:99: }
This should be after OnBeforeUserAction() and has to
call OnAfteruserAction before it returns true.
I also think you need to update last_click_time_ to get double/triple clicking
work correctly.

Please mention that this should be removed once native textfield impls are
consolidated to textfield.

http://codereview.chromium.org/8527015/diff/1/views/metrics_aura.cc
File views/metrics_aura.cc (right):

http://codereview.chromium.org/8527015/diff/1/views/metrics_aura.cc#newcode17
views/metrics_aura.cc:17: // Use the default double click interval value on gtk.
ditto

http://codereview.chromium.org/8527015/diff/1/views/view_aura.cc
File views/view_aura.cc (right):

http://codereview.chromium.org/8527015/diff/1/views/view_aura.cc#newcode14
views/view_aura.cc:14: // Use the default horizontal drag threshold on gtk for
now.
same as what gtk uses.

http://codereview.chromium.org/8527015/diff/1/views/view_aura.cc#newcode16
views/view_aura.cc:16: return 8;
This has to be adjusted for different display density.

(i've been told to avoid "we":)

[jennyz, 2011-11-12 01:50:17]

I removed the change in view_aura.cc and metrics_aura.cc since it is in cl:
http://codereview.chromium.org/8524008/

I will land that cl before this one.

http://codereview.chromium.org/8527015/diff/1/chrome/browser/ui/views/omnibox...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/8527015/diff/1/chrome/browser/ui/views/omnibox...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:275: if
(event.IsOnlyLeftMouseButton() && !textfield_->HasFocus() &&
On 2011/11/11 01:28:58, msw wrote:
> We might want to do this for right mouse too. On Windows, the right mouse
button
> also selects all and takes focus; and then the context menu is shown.

Done.

http://codereview.chromium.org/8527015/diff/1/views/controls/textfield/native...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:97: if
(textfield_->OnMousePressed(event)) {
On 2011/11/11 01:28:58, msw wrote:
> Please add a comment here like "Allow the textfield/omnibox to optionally
handle
> the mouse pressed event." Also, remove the braces for a single statement.

Done.

http://codereview.chromium.org/8527015/diff/1/views/controls/textfield/native...
views/controls/textfield/native_textfield_views.cc:99: }
On 2011/11/11 01:52:32, oshima wrote:
> This should be after OnBeforeUserAction() and has to
> call OnAfteruserAction before it returns true.
> I also think you need to update last_click_time_ to get double/triple clicking
> work correctly.
> 
> Please mention that this should be removed once native textfield impls are
> consolidated to textfield.
> 

Done.

[msw, 2011-11-12 02:19:21]

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:1058: #if defined(TOUCH_UI)
Please add a comment here, something like "Temporarily disable drag and drop on
touch builds; see crbug.com/97845."

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:1067: break;
outdent break by two spaces, it's not part of the else.

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.h:218: // Tracks the mouse
clicks for single/double/tripple clicks.
typo: triple.

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.h:221: // Handles mouse press
event.
grammar: pluralize events.

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.h:222: void
HandleMousePresseEvent(const MouseEvent& event);
typo: HandleMousePress*e*Events

[jennyz, 2011-11-14 18:28:57]

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:1058: #if defined(TOUCH_UI)
On 2011/11/12 02:19:21, msw wrote:
> Please add a comment here, something like "Temporarily disable drag and drop
on
> touch builds; see crbug.com/97845."

Done.

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:1067: break;
On 2011/11/12 02:19:21, msw wrote:
> outdent break by two spaces, it's not part of the else.

Done.

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.h (right):

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.h:218: // Tracks the mouse
clicks for single/double/tripple clicks.
On 2011/11/12 02:19:21, msw wrote:
> typo: triple.

Done.

http://codereview.chromium.org/8527015/diff/5001/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.h:222: void
HandleMousePresseEvent(const MouseEvent& event);
On 2011/11/12 02:19:21, msw wrote:
> typo: HandleMousePress*e*Events

Done.

[msw, 2011-11-14 19:00:33]

LGTM with a nit.

http://codereview.chromium.org/8527015/diff/4002/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/4002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:1058: // Temporarily disable
drag and drop on touch builds; see crbug.com/97845.
nit: move this into the touchui block.

[jennyz, 2011-11-14 19:09:51]

http://codereview.chromium.org/8527015/diff/4002/views/controls/textfield/nat...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/4002/views/controls/textfield/nat...
views/controls/textfield/native_textfield_views.cc:1058: // Temporarily disable
drag and drop on touch builds; see crbug.com/97845.
On 2011/11/14 19:00:33, msw wrote:
> nit: move this into the touchui block.

Done.

[oshima, 2011-11-14 19:31:09]

LGTM

http://codereview.chromium.org/8527015/diff/13001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/8527015/diff/13001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:275: if
(!textfield_->HasFocus() && !textfield_->HasSelection()) {
We may need to check if the textfield has IME composition text. Please ask
penghuang/suzhe/susukes if it's necessary.
(separate CL is fine)

http://codereview.chromium.org/8527015/diff/13001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/13001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:105: }
nuke {}

[jennyz, 2011-11-14 19:50:09]

http://codereview.chromium.org/8527015/diff/13001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

http://codereview.chromium.org/8527015/diff/13001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:275: if
(!textfield_->HasFocus() && !textfield_->HasSelection()) {
On 2011/11/14 19:31:09, oshima wrote:
> We may need to check if the textfield has IME composition text. Please ask
> penghuang/suzhe/susukes if it's necessary.
> (separate CL is fine)

Done. Will do.

http://codereview.chromium.org/8527015/diff/13001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/8527015/diff/13001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:105: }
On 2011/11/14 19:31:09, oshima wrote:
> nuke {}

Done.

[commit-bot: I haz the power, 2011-11-15 19:13:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/8527015/1012

[commit-bot: I haz the power, 2011-11-15 19:13:53]

Presubmit check for 8527015-1012 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
views/controls/textfield/native_textfield_views.cc,views/controls/textfield/native_textfield_views.h

Presubmit checks took 1.6s to calculate.

[jennyz, 2011-11-15 19:19:15]

msw and oshima have reviewed the cl, ben, would you please take a look? I need
owner's lgtm.

[Ben Goodger (Google), 2011-11-15 21:24:37]

LGTM, but this is the kind of thing we're going to wish we had unit tests for
before long...

[commit-bot: I haz the power, 2011-11-15 22:12:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennyz@chromium.org/8527015/1012

[commit-bot: I haz the power, 2011-11-15 23:12:21]

Change committed as 110202