Implements tests for testing browser's overall key events handling behavior....

Implements tests for testing browser's overall key events handling behavior.

This CL implements some basic tests for testing browser's overall key events handling behavior. This CL depends on http://codereview.chromium.org/235039 and http://codereview.chromium.org/195062. Currently, only Linux and Windows are supported.
The tests assume US keyboard layout is used and no IME is activated. We still need to investigate how to write tests that involving different keyboard layout and input methods.

BUG=none
TEST=none

[Paweł Hajdan Jr., 2009-10-12 14:29:27]

Drive-by.

Thanks for writing the tests. I added some comments about style, but what I'm
mostly concerned about is the flakiness (more detailed comments inline).

http://codereview.chromium.org/268035/diff/3001/3008
File chrome/browser/browser_focus_uitest.cc (right):

http://codereview.chromium.org/268035/diff/3001/3008#newcode64
Line 64: void CheckViewHasFocus(ViewID vid) {
Please make this method return bool and assert in callers. This way output is
more informative in case of failure. Or maybe just paste the ASSERT_TRUE line
instead of calling this method and delete this method.

http://codereview.chromium.org/268035/diff/3001/3007
File chrome/browser/browser_keyevents_browsertest.cc (right):

http://codereview.chromium.org/268035/diff/3001/3007#newcode106
Line 106: void CheckViewHasFocus(ViewID vid) {
I'd prefere returning bool from this method and asserting in caller or just
removing this method.

http://codereview.chromium.org/268035/diff/3001/3007#newcode114
Line 114: void SuppressByType(int tab_index, const wchar_t* type, bool suppress)
{
Please add a brief comment. It's not obvious for me what suppressing means in
this context.

http://codereview.chromium.org/268035/diff/3001/3007#newcode149
Line 149: while (*actual_length < length && retry_count++ < kMaxRetryTimes) {
No, please, no polling! It *will* be flaky. Could you trigger some event from JS
that we can wait for? Like a navigation, closing a window, opening a window,
opening a modal dialog (alert)...

http://codereview.chromium.org/268035/diff/3001/3007#newcode306
Line 306: EXPECT_NO_FATAL_FAILURE(TestKeyEvent(tab_index, kTestNoInput[i]));
Could you print something that will help identify the failing test case? For
example the index.

http://codereview.chromium.org/268035/diff/3001/3007#newcode310
Line 310: EXPECT_NO_FATAL_FAILURE(TestKeyEvent(tab_index, kTestWithInput[i]));
Same here.

http://codereview.chromium.org/268035/diff/3001/3007#newcode473
Line 473: #else  // Windows
This might be mysterious when we finally have browser_tests on Mac. Please
either adjust the if or the comment.

http://codereview.chromium.org/268035/diff/3001/3006
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/268035/diff/3001/3006#newcode7
Line 7: #include "base/logging.h"
nit: Please move this header to correct place is section below.

http://codereview.chromium.org/268035/diff/3001/3006#newcode267
Line 267: static bool WidgetInFocusChain(GtkWidget* root, GtkWidget* target) {
nit: How about IsWidgetInFocusChain or something that clearly implies bool
return value?

http://codereview.chromium.org/268035/diff/3001/3006#newcode523
Line 523: bool CheckViewHasFocus(const Browser* browser, ViewID vid) {
Please consider splitting this into ui_test_utils_linux etc.

http://codereview.chromium.org/268035/diff/3001/3004
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/268035/diff/3001/3004#newcode125
Line 125: // Checks if a View is focused in the browser.
nit: The comment in line 125 is redundant. Line 126 well explains this function.

http://codereview.chromium.org/268035/diff/3001/3004#newcode127
Line 127: bool CheckViewHasFocus(const Browser* browser, ViewID vid);
nit: The Check in the name might be a little misleading. How about ViewHasFocus
or IsViewFocused?

[James Su, 2009-10-13 10:35:33]

Thanks a lot for your feedback. CL has been updated accordingly.

Regards
James Su

http://codereview.chromium.org/268035/diff/3001/3008
File chrome/browser/browser_focus_uitest.cc (right):

http://codereview.chromium.org/268035/diff/3001/3008#newcode64
Line 64: void CheckViewHasFocus(ViewID vid) {
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> Please make this method return bool and assert in callers. This way output is
> more informative in case of failure. Or maybe just paste the ASSERT_TRUE line
> instead of calling this method and delete this method.

Done.

http://codereview.chromium.org/268035/diff/3001/3007
File chrome/browser/browser_keyevents_browsertest.cc (right):

http://codereview.chromium.org/268035/diff/3001/3007#newcode106
Line 106: void CheckViewHasFocus(ViewID vid) {
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> I'd prefere returning bool from this method and asserting in caller or just
> removing this method.

Done.

http://codereview.chromium.org/268035/diff/3001/3007#newcode114
Line 114: void SuppressByType(int tab_index, const wchar_t* type, bool suppress)
{
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> Please add a brief comment. It's not obvious for me what suppressing means in
> this context.

Done.

http://codereview.chromium.org/268035/diff/3001/3007#newcode149
Line 149: while (*actual_length < length && retry_count++ < kMaxRetryTimes) {
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> No, please, no polling! It *will* be flaky. Could you trigger some event from
JS
> that we can wait for? Like a navigation, closing a window, opening a window,
> opening a modal dialog (alert)...

Done.

http://codereview.chromium.org/268035/diff/3001/3007#newcode306
Line 306: EXPECT_NO_FATAL_FAILURE(TestKeyEvent(tab_index, kTestNoInput[i]));
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> Could you print something that will help identify the failing test case? For
> example the index.

Done.

http://codereview.chromium.org/268035/diff/3001/3007#newcode310
Line 310: EXPECT_NO_FATAL_FAILURE(TestKeyEvent(tab_index, kTestWithInput[i]));
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> Same here.

Done.

http://codereview.chromium.org/268035/diff/3001/3007#newcode473
Line 473: #else  // Windows
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> This might be mysterious when we finally have browser_tests on Mac. Please
> either adjust the if or the comment.

Done.

http://codereview.chromium.org/268035/diff/3001/3006
File chrome/test/ui_test_utils.cc (right):

http://codereview.chromium.org/268035/diff/3001/3006#newcode7
Line 7: #include "base/logging.h"
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> nit: Please move this header to correct place is section below.

Done.

http://codereview.chromium.org/268035/diff/3001/3006#newcode267
Line 267: static bool WidgetInFocusChain(GtkWidget* root, GtkWidget* target) {
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> nit: How about IsWidgetInFocusChain or something that clearly implies bool
> return value?

Done.

http://codereview.chromium.org/268035/diff/3001/3006#newcode523
Line 523: bool CheckViewHasFocus(const Browser* browser, ViewID vid) {
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> Please consider splitting this into ui_test_utils_linux etc.

Done.

http://codereview.chromium.org/268035/diff/3001/3004
File chrome/test/ui_test_utils.h (right):

http://codereview.chromium.org/268035/diff/3001/3004#newcode125
Line 125: // Checks if a View is focused in the browser.
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> nit: The comment in line 125 is redundant. Line 126 well explains this
function.

Done.

http://codereview.chromium.org/268035/diff/3001/3004#newcode127
Line 127: bool CheckViewHasFocus(const Browser* browser, ViewID vid);
On 2009/10/12 14:29:27, Paweł Hajdan Jr. wrote:
> nit: The Check in the name might be a little misleading. How about
ViewHasFocus
> or IsViewFocused?

Done.

[Paweł Hajdan Jr., 2009-10-13 10:59:08]

I'm really happy to see this test properly waiting. Just some minor comments.

http://codereview.chromium.org/268035/diff/4006/3023
File chrome/chrome.gyp (right):

http://codereview.chromium.org/268035/diff/4006/3023#newcode3816
Line 3816: 'test/ui_test_utils_linux.cc',
You don't need to make a special condition block for it. _linux files will be
automatically filtered out for other platforms.

http://codereview.chromium.org/268035/diff/4006/3023#newcode3824
Line 3824: 'test/ui_test_utils_win.cc',
Same here.

http://codereview.chromium.org/268035/diff/4006/3023#newcode3829
Line 3829: 'test/ui_test_utils_mac.cc',
And here.

http://codereview.chromium.org/268035/diff/4006/3023#newcode6068
Line 6068: 'test/ui_test_utils_linux.cc',
Can you just depend on test_support_common here?

http://codereview.chromium.org/268035/diff/4006/3023#newcode6083
Line 6083: 'test/ui_test_utils_win.cc',
Same here.

http://codereview.chromium.org/268035/diff/4006/3023#newcode6291
Line 6291: 'test/ui_test_utils_win.cc',
Same here.

http://codereview.chromium.org/268035/diff/4006/3018
File chrome/test/ui_test_utils_mac.cc (right):

http://codereview.chromium.org/268035/diff/4006/3018#newcode8
Line 8: namespace ui_test_utils {
nit: Please add an empty line above.

[James Su, 2009-10-14 07:55:21]

CL updated, please help review again. I'm actually not familiar with gyp files.
chrome.gyp looks so messy for me. Especially, I don't understand why a condition
block for a specific platform (like 'OS=="win"') can contain sub condition
blocks for other platforms.

Regards
James Su

On 2009/10/13 10:59:08, Paweł Hajdan Jr. wrote:
> I'm really happy to see this test properly waiting. Just some minor comments.
> 
> http://codereview.chromium.org/268035/diff/4006/3023
> File chrome/chrome.gyp (right):
> 
> http://codereview.chromium.org/268035/diff/4006/3023#newcode3816
> Line 3816: 'test/ui_test_utils_linux.cc',
> You don't need to make a special condition block for it. _linux files will be
> automatically filtered out for other platforms.
> 
> http://codereview.chromium.org/268035/diff/4006/3023#newcode3824
> Line 3824: 'test/ui_test_utils_win.cc',
> Same here.
> 
> http://codereview.chromium.org/268035/diff/4006/3023#newcode3829
> Line 3829: 'test/ui_test_utils_mac.cc',
> And here.
> 
> http://codereview.chromium.org/268035/diff/4006/3023#newcode6068
> Line 6068: 'test/ui_test_utils_linux.cc',
> Can you just depend on test_support_common here?
> 
> http://codereview.chromium.org/268035/diff/4006/3023#newcode6083
> Line 6083: 'test/ui_test_utils_win.cc',
> Same here.
> 
> http://codereview.chromium.org/268035/diff/4006/3023#newcode6291
> Line 6291: 'test/ui_test_utils_win.cc',
> Same here.
> 
> http://codereview.chromium.org/268035/diff/4006/3018
> File chrome/test/ui_test_utils_mac.cc (right):
> 
> http://codereview.chromium.org/268035/diff/4006/3018#newcode8
> Line 8: namespace ui_test_utils {
> nit: Please add an empty line above.

[James Su, 2009-10-14 07:55:44]

http://codereview.chromium.org/268035/diff/4006/3023
File chrome/chrome.gyp (right):

http://codereview.chromium.org/268035/diff/4006/3023#newcode3816
Line 3816: 'test/ui_test_utils_linux.cc',
On 2009/10/13 10:59:08, Paweł Hajdan Jr. wrote:
> You don't need to make a special condition block for it. _linux files will be
> automatically filtered out for other platforms.

Done.

http://codereview.chromium.org/268035/diff/4006/3023#newcode3824
Line 3824: 'test/ui_test_utils_win.cc',
On 2009/10/13 10:59:08, Paweł Hajdan Jr. wrote:
> Same here.

Done.

http://codereview.chromium.org/268035/diff/4006/3023#newcode3829
Line 3829: 'test/ui_test_utils_mac.cc',
On 2009/10/13 10:59:08, Paweł Hajdan Jr. wrote:
> And here.

Done.

http://codereview.chromium.org/268035/diff/4006/3023#newcode6068
Line 6068: 'test/ui_test_utils_linux.cc',
On 2009/10/13 10:59:08, Paweł Hajdan Jr. wrote:
> Can you just depend on test_support_common here?

I don't know. I'm not familiar with this piece of code. Actually it looks weird
to me. This target is inside a "OS=="win" condition block, so I don't understand
these sub condition blocks here, which seems have no effect at all.

http://codereview.chromium.org/268035/diff/4006/3023#newcode6291
Line 6291: 'test/ui_test_utils_win.cc',
On 2009/10/13 10:59:08, Paweł Hajdan Jr. wrote:
> Same here.

Done.

[Paweł Hajdan Jr., 2009-10-14 17:01:15]

LGTM