Fix the double/tripple click behavior of NataiveTextfieldViews objects.

Fix the double/tripple click behavior of NataiveTextfieldViews on aura.


BUG=101774
TEST=NONE


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

[sky, 2011-11-10 20:18:11]

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

http://codereview.chromium.org/8524008/diff/1/views/metrics_aura.cc#newcode18
views/metrics_aura.cc:18: // TODO(jennyz): This value may need to be adjusted on
different platform.
platform -> platforms

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

http://codereview.chromium.org/8524008/diff/1/views/view_aura.cc#newcode16
views/view_aura.cc:16: return 8;
platform -> platforms

[msw, 2011-11-10 20:30:57]

I'd prefer to see these magic numbers as local constants defined in anonymous
namespaces. Then GetVerticalDragThreshold can return the same kDragThreshold
rather than call GetHorizontalDragThreshold.

[jennyz, 2011-11-10 21:12:00]

On 2011/11/10 20:30:57, msw wrote:
> I'd prefer to see these magic numbers as local constants defined in anonymous
> namespaces. Then GetVerticalDragThreshold can return the same kDragThreshold
> rather than call GetHorizontalDragThreshold.
Done

[jennyz, 2011-11-10 21:23:16]

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

http://codereview.chromium.org/8524008/diff/1/views/metrics_aura.cc#newcode18
views/metrics_aura.cc:18: // TODO(jennyz): This value may need to be adjusted on
different platform.
On 2011/11/10 20:18:11, sky wrote:
> platform -> platforms

Done.

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

http://codereview.chromium.org/8524008/diff/1/views/view_aura.cc#newcode16
views/view_aura.cc:16: return 8;
On 2011/11/10 20:18:11, sky wrote:
> platform -> platforms

Done.

[sky, 2011-11-10 21:37:02]

LGTM

http://codereview.chromium.org/8524008/diff/6001/views/metrics_aura.cc
File views/metrics_aura.cc (right):

http://codereview.chromium.org/8524008/diff/6001/views/metrics_aura.cc#newcode12
views/metrics_aura.cc:12: // Default double click interval in milliseconds.
nit: newline between 11 and 12.

http://codereview.chromium.org/8524008/diff/6001/views/view_aura.cc
File views/view_aura.cc (right):

http://codereview.chromium.org/8524008/diff/6001/views/view_aura.cc#newcode8
views/view_aura.cc:8: // Default horizontal drag threshold in pixels.
nit: newline between 7 and 8

[jennyz, 2011-11-10 21:47:25]

http://codereview.chromium.org/8524008/diff/6001/views/metrics_aura.cc
File views/metrics_aura.cc (right):

http://codereview.chromium.org/8524008/diff/6001/views/metrics_aura.cc#newcode12
views/metrics_aura.cc:12: // Default double click interval in milliseconds.
On 2011/11/10 21:37:03, sky wrote:
> nit: newline between 11 and 12.

Done.

http://codereview.chromium.org/8524008/diff/6001/views/view_aura.cc
File views/view_aura.cc (right):

http://codereview.chromium.org/8524008/diff/6001/views/view_aura.cc#newcode8
views/view_aura.cc:8: // Default horizontal drag threshold in pixels.
On 2011/11/10 21:37:03, sky wrote:
> nit: newline between 7 and 8

Done.

[msw, 2011-11-10 22:10:56]

http://codereview.chromium.org/8524008/diff/2002/views/view_aura.cc
File views/view_aura.cc (right):

http://codereview.chromium.org/8524008/diff/2002/views/view_aura.cc#newcode27
views/view_aura.cc:27: return GetHorizontalDragThreshold();
nit: Make the const independent of vertical or horizontal and just return the
const value here.

[commit-bot: I haz the power, 2011-11-10 22:22:42]

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

[commit-bot: I haz the power, 2011-11-10 23:28:17]

Change committed as 109535

[jennyz, 2011-11-11 23:48:23]

The cl has been committed then reverted due to the RTL unit test failure on
win_aura. Fix the test now.

[msw, 2011-11-12 00:09:08]

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:1541:
base::PlatformThread::Sleep(1000);
See the code around line 1377 in NativeTextfieldViewsTest, HitInsideTextAreaTest
that calls NonClientMouseClick to avoid double click.

// To avoid trigger double click. Not using sleep() since it takes longer
// for the test to run if using sleep().
NonClientMouseClick();

[jennyz, 2011-11-12 00:13:35]

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:1541:
base::PlatformThread::Sleep(1000);
On 2011/11/12 00:09:08, msw wrote:
> See the code around line 1377 in NativeTextfieldViewsTest,
HitInsideTextAreaTest
> that calls NonClientMouseClick to avoid double click.
> 
> // To avoid trigger double click. Not using sleep() since it takes longer
> // for the test to run if using sleep().
> NonClientMouseClick();
I tried NonClientMouseClick() first, but it does not work here, it was still
considered as a double click. I discussed with Xiaomei, she thinks it is ok to
see sleep here.

[xji, 2011-11-12 01:26:32]

lgtm.

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:1541:
base::PlatformThread::Sleep(1000);
On 2011/11/12 00:13:35, jennyz wrote:
> On 2011/11/12 00:09:08, msw wrote:
> > See the code around line 1377 in NativeTextfieldViewsTest,
> HitInsideTextAreaTest
> > that calls NonClientMouseClick to avoid double click.
> > 
> > // To avoid trigger double click. Not using sleep() since it takes longer
> > // for the test to run if using sleep().
> > NonClientMouseClick();
> I tried NonClientMouseClick() first, but it does not work here, it was still
> considered as a double click. I discussed with Xiaomei, she thinks it is ok to
> see sleep here.

I tried sleep() before but that makes a test (with while loop) takes longer to
run
https://cs.corp.google.com/#chrome/src/views/controls/textfield/native_textfi...

I think it is ok for this case.
But we probably need to check why NonClientClick does not work here.

[msw, 2011-11-12 02:02:45]

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:1541:
base::PlatformThread::Sleep(1000);
On 2011/11/12 01:26:32, xji wrote:
> On 2011/11/12 00:13:35, jennyz wrote:
> > On 2011/11/12 00:09:08, msw wrote:
> > > See the code around line 1377 in NativeTextfieldViewsTest,
> > HitInsideTextAreaTest
> > > that calls NonClientMouseClick to avoid double click.
> > > 
> > > // To avoid trigger double click. Not using sleep() since it takes longer
> > > // for the test to run if using sleep().
> > > NonClientMouseClick();
> > I tried NonClientMouseClick() first, but it does not work here, it was still
> > considered as a double click. I discussed with Xiaomei, she thinks it is ok
to
> > see sleep here.
> 
> I tried sleep() before but that makes a test (with while loop) takes longer to
> run
>
https://cs.corp.google.com/#chrome/src/views/controls/textfield/native_textfi...
> 
> I think it is ok for this case.
> But we probably need to check why NonClientClick does not work here.

I'd prefer that we fix NonClientClick first; but if we must make this hack,
please file a bug and add a comment with the bug number here.

[jennyz, 2011-11-14 17:26:42]

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:1541:
base::PlatformThread::Sleep(1000);
On 2011/11/12 02:02:45, msw wrote:
> On 2011/11/12 01:26:32, xji wrote:
> > On 2011/11/12 00:13:35, jennyz wrote:
> > > On 2011/11/12 00:09:08, msw wrote:
> > > > See the code around line 1377 in NativeTextfieldViewsTest,
> > > HitInsideTextAreaTest
> > > > that calls NonClientMouseClick to avoid double click.
> > > > 
> > > > // To avoid trigger double click. Not using sleep() since it takes
longer
> > > > // for the test to run if using sleep().
> > > > NonClientMouseClick();
> > > I tried NonClientMouseClick() first, but it does not work here, it was
still
> > > considered as a double click. I discussed with Xiaomei, she thinks it is
ok
> to
> > > see sleep here.
> > 
> > I tried sleep() before but that makes a test (with while loop) takes longer
to
> > run
> >
>
https://cs.corp.google.com/#chrome/src/views/controls/textfield/native_textfi...
> > 
> > I think it is ok for this case.
> > But we probably need to check why NonClientClick does not work here.
> 
> I'd prefer that we fix NonClientClick first; but if we must make this hack,
> please file a bug and add a comment with the bug number here.

Done.
Filed a bug for NonClientMouseClick. http://crbug.com/104150.

[msw, 2011-11-14 18:21:25]

LGTM with one nit if tests pass now.

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/8524008/diff/10001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:1541:
base::PlatformThread::Sleep(1000);
> Filed a bug for NonClientMouseClick. http://crbug.com/104150.

Please add a comment here with that bug number.