Adds ui_test_utils::SendAndWaitForKeyPress and converts callers (where

Adds ui_test_utils::SendAndWaitForKeyPress and converts callers (where
appropriate) to use it. Hopefully this will help isolate why tests are
failing on bot.

BUG=none
TEST=none; test only change.

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

[Paweł Hajdan Jr., 2010-08-20 21:51:13]

I don't really understand how this change helps. Please remember we still have a
timeout in out of process test runner, and the timeout using PostDelayedTask is
going to be removed at some point.

Some minor comments.

http://codereview.chromium.org/3117030/diff/2001/3008
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/3117030/diff/2001/3008#newcode195
chrome/test/ui_test_utils.h:195: //
ASSERT_NO_FATAL_FAILURE(ui_test_utils::Send...).
How about making it return false on failure (you can use HasFatalFailure or
something like that from gtest, and adding WARN_UNUSED_RESULT to make sure
people really check the value?

http://codereview.chromium.org/3117030/diff/2001/3008#newcode196
chrome/test/ui_test_utils.h:196: void SendAndWaitForKeyPress(gfx::NativeWindow
window,
nit: How about SimulateKeyPress? SendAndWaitForKeyPress is a bit weird name.

[sky, 2010-08-20 21:56:57]

On Fri, Aug 20, 2010 at 2:51 PM,  <phajdan.jr@chromium.org> wrote:
> I don't really understand how this change helps. Please remember we still
> have a
> timeout in out of process test runner, and the timeout using PostDelayedTask
> is
> going to be removed at some point.

This makes it so that if the key event isn't received in the timeout
we fail at the waiting point rather than at some random point later on
(such as the DCHECK in ui_controls if the test attempts to send
another key). You're right that this doesn't address the underlying
problem, but it should avoid some false noise we're getting. Does that
sound ok?

> Some minor comments.
>
>
> http://codereview.chromium.org/3117030/diff/2001/3008
> File chrome/test/ui_test_utils.h (right):
>
> http://codereview.chromium.org/3117030/diff/2001/3008#newcode195
> chrome/test/ui_test_utils.h:195: //
> ASSERT_NO_FATAL_FAILURE(ui_test_utils::Send...).
> How about making it return false on failure (you can use HasFatalFailure
> or something like that from gtest, and adding WARN_UNUSED_RESULT to make
> sure people really check the value?

I'm fine with that.

> http://codereview.chromium.org/3117030/diff/2001/3008#newcode196
> chrome/test/ui_test_utils.h:196: void
> SendAndWaitForKeyPress(gfx::NativeWindow window,
> nit: How about SimulateKeyPress? SendAndWaitForKeyPress is a bit weird
> name.
>
> http://codereview.chromium.org/3117030/show

I want block or wait in the name so that it's clear this blocks.
SimulateKeyPress doesn't give that. Does you like SendKeyPressAndBlock
any better?

  -Scott

[Paweł Hajdan Jr., 2010-08-20 21:58:56]

On Fri, Aug 20, 2010 at 14:56, Scott Violet <sky@chromium.org> wrote:

> On Fri, Aug 20, 2010 at 2:51 PM,  <phajdan.jr@chromium.org> wrote:
> > I don't really understand how this change helps. Please remember we still
> > have a
> > timeout in out of process test runner, and the timeout using
> PostDelayedTask
> > is
> > going to be removed at some point.
>
> This makes it so that if the key event isn't received in the timeout
> we fail at the waiting point rather than at some random point later on
> (such as the DCHECK in ui_controls if the test attempts to send
> another key). You're right that this doesn't address the underlying
> problem, but it should avoid some false noise we're getting. Does that
> sound ok?


Ah, yes, I see the issue now.


> I want block or wait in the name so that it's clear this blocks.
> SimulateKeyPress doesn't give that. Does you like SendKeyPressAndBlock
> any better?
>

SendKeyPressSync?

[sky, 2010-08-20 23:02:53]

Good idea. New patch uploaded

  -Scott

[Paweł Hajdan Jr., 2010-08-21 02:44:27]

LGTM with some comments.

http://codereview.chromium.org/3117030/diff/9001/10007
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/3117030/diff/9001/10007#newcode562
chrome/test/ui_test_utils.cc:562: DLOG(ERROR) <<
"ui_controls::SendKeyPressNotifyWhenDone failed";
nit: You can consider using LOG instead of DLOG here. We should rarely enter the
failure case. Up to you.

http://codereview.chromium.org/3117030/diff/9001/10007#newcode565
chrome/test/ui_test_utils.cc:565: // Run the message loop, it'll quit because
either the key was received or the
nit: I think this comment is confusing. I'd rather remove it, but if you think
it should really be there, that's fine.

http://codereview.chromium.org/3117030/diff/9001/10008
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/3117030/diff/9001/10008#newcode195
chrome/test/ui_test_utils.h:195: // The proper way to use this in a test is:
nit: I suggest to remove the "The proper way to use ..." part, because the
compiler is going to issue a warning about that on Linux and Mac.