Enable interactive_ui_tests

Enable interactive_ui_tests

 * changed DCHECK in tree_node_model to make it easy to find how it failed.
 * Fixed compilation error in find_bar_host_interactive_uitest.cc on linux/views.
 * Disabled BrowserKeyEventsTest.AccessKeys for Chromeos
   Added/Changed ui_test_utils::RunAllPendingInMessageLoop where necessary.
 * Disabled tab_dragging_tests. I included the source so that we can catch
   compilation error even if tests do not run. Changed the comment from // -> /*
   as GCC complains the use of "\" (and potentially dangerous)
 * Added SleepInMessageLoop to allow event loop to execute the tasks/events
   while waiting.
 * For RunAllPendingInMessageLoop change, please see
   http://codereview.chromium.org/1591004

BUG=39736
TEST=interactive_ui_tests should pass with following cls
http://codereview.chromium.org/1594003
http://codereview.chromium.org/1545011.

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

[Paweł Hajdan Jr., 2010-04-01 06:49:19]

Drive-by with test comments.

Please let me take another look before you commit.

http://codereview.chromium.org/1576008/diff/2001/3005
File chrome/browser/views/tabs/tab_dragging_test.cc (right):

http://codereview.chromium.org/1576008/diff/2001/3005#newcode108
chrome/browser/views/tabs/tab_dragging_test.cc:108: /*
nit: Why change this? We prefer // for comments.

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

http://codereview.chromium.org/1576008/diff/2001/3008#newcode52
chrome/test/ui_test_utils.h:52: void SleepInMessageLoop(int sleep_ms);
Please don't add functions like this. We don't want to encourage sleeping. All
browser tests should wait properly, always.

[oshima, 2010-04-01 07:54:49]

http://codereview.chromium.org/1576008/diff/2001/3005
File chrome/browser/views/tabs/tab_dragging_test.cc (right):

http://codereview.chromium.org/1576008/diff/2001/3005#newcode108
chrome/browser/views/tabs/tab_dragging_test.cc:108: /*
On 2010/04/01 06:49:19, Paweł Hajdan Jr. wrote:
> nit: Why change this? We prefer // for comments.

having \ in // make it multi line comment, which is causing "warning" (thus
error) in our GCC setting. This is because \ in // is dangerous, like this.

 case '\': // handle \
      callFoo();
      break;

And I don't think we want to change this compiler option, so we need
to change something here.
I'm okay to change \ to something else and keep //, so please let me know.

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

http://codereview.chromium.org/1576008/diff/2001/3008#newcode52
chrome/test/ui_test_utils.h:52: void SleepInMessageLoop(int sleep_ms);
On 2010/04/01 06:49:19, Paweł Hajdan Jr. wrote:
> Please don't add functions like this. We don't want to encourage sleeping. All
> browser tests should wait properly, always.

I agree that "sleeping" is not good idea in general, and I wish I have better
solution. Unfortunately, some of our test code already depend on timing, and
that's why I need this for now. Please see my change in browser_focus_test.cc.
Do you have better solution? I'm happy to change if you do. Otherwise, I'd like
to enable the test rather than not enabling it. Please file a bug and assign to
me. I'll investigate possible solution.

[Paweł Hajdan Jr., 2010-04-01 08:29:27]

http://codereview.chromium.org/1576008/diff/2001/3005
File chrome/browser/views/tabs/tab_dragging_test.cc (right):

http://codereview.chromium.org/1576008/diff/2001/3005#newcode108
chrome/browser/views/tabs/tab_dragging_test.cc:108: /*
On 2010/04/01 07:54:49, oshima wrote:
> On 2010/04/01 06:49:19, Paweł Hajdan Jr. wrote:
> > nit: Why change this? We prefer // for comments.
> 
> having \ in // make it multi line comment, which is causing "warning" (thus
> error) in our GCC setting. This is because \ in // is dangerous, like this.
> 
>  case '\': // handle \
>       callFoo();
>       break;

I see. /* */ are fine for me then.

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

http://codereview.chromium.org/1576008/diff/2001/3008#newcode52
chrome/test/ui_test_utils.h:52: void SleepInMessageLoop(int sleep_ms);
On 2010/04/01 07:54:49, oshima wrote:
> On 2010/04/01 06:49:19, Paweł Hajdan Jr. wrote:
> > Please don't add functions like this. We don't want to encourage sleeping.
All
> > browser tests should wait properly, always.
> 
> I agree that "sleeping" is not good idea in general, and I wish I have better
> solution. Unfortunately, some of our test code already depend on timing, and
> that's why I need this for now. Please see my change in browser_focus_test.cc.

The correct solution is waiting for some notification. It's probably
non-trivial, but I'd prefer not to have a "convenient" Sleep function in
ui_test_utils.h, even with some kind of warning message, because it encourages
even more people to use it.

Could you just use PostDelayedTask and RunMessageLoop inline in cases where it's
needed?

[oshima, 2010-04-01 15:22:02]

On Thu, Apr 1, 2010 at 1:29 AM, <phajdan.jr@chromium.org> wrote:

>
> http://codereview.chromium.org/1576008/diff/2001/3005
> File chrome/browser/views/tabs/tab_dragging_test.cc (right):
>
> http://codereview.chromium.org/1576008/diff/2001/3005#newcode108
> chrome/browser/views/tabs/tab_dragging_test.cc:108: /*
> On 2010/04/01 07:54:49, oshima wrote:
>
>> On 2010/04/01 06:49:19, Paweł Hajdan Jr. wrote:
>> > nit: Why change this? We prefer // for comments.
>>
>
>  having \ in // make it multi line comment, which is causing "warning"
>>
> (thus
>
>> error) in our GCC setting. This is because \ in // is dangerous, like
>>
> this.
>
>   case '\': // handle \
>>       callFoo();
>>       break;
>>
>
> I see. /* */ are fine for me then.
>
>
> http://codereview.chromium.org/1576008/diff/2001/3008
> File chrome/test/ui_test_utils.h (right):
>
> http://codereview.chromium.org/1576008/diff/2001/3008#newcode52
> chrome/test/ui_test_utils.h:52: void SleepInMessageLoop(int sleep_ms);
> On 2010/04/01 07:54:49, oshima wrote:
>
>> On 2010/04/01 06:49:19, Paweł Hajdan Jr. wrote:
>> > Please don't add functions like this. We don't want to encourage
>>
> sleeping. All
>
>> > browser tests should wait properly, always.
>>
>
>  I agree that "sleeping" is not good idea in general, and I wish I have
>>
> better
>
>> solution. Unfortunately, some of our test code already depend on
>>
> timing, and
>
>> that's why I need this for now. Please see my change in
>>
> browser_focus_test.cc.
>
> The correct solution is waiting for some notification. It's probably
> non-trivial, but I'd prefer not to have a "convenient" Sleep function in
> ui_test_utils.h, even with some kind of warning message, because it
> encourages even more people to use it.
>

Understood. I'll move them from ui_test_utils from inline then. Yes, I did
add
notification for bookmark tests. I will fix this in separate CL. Thank you
for your
feedback.

>

>
> Could you just use PostDelayedTask and RunMessageLoop inline in cases
> where it's needed?


>
> http://codereview.chromium.org/1576008
>

[zel, 2010-04-01 17:00:27]

LGTM

please make sure mac buildbots are happy before submitting

[tfarina (gmail-do not use), 2010-04-05 19:43:13]

http://codereview.chromium.org/1576008/diff/24001/4002
File app/tree_node_model.h (right):

http://codereview.chromium.org/1576008/diff/24001/4002#newcode85
app/tree_node_model.h:85: DCHECK(child);
just curious, why do you need this change?

[tfarina (gmail-do not use), 2010-04-05 19:44:15]

On 2010/04/05 19:43:13, tfarina wrote:
> http://codereview.chromium.org/1576008/diff/24001/4002
> File app/tree_node_model.h (right):
> 
> http://codereview.chromium.org/1576008/diff/24001/4002#newcode85
> app/tree_node_model.h:85: DCHECK(child);
> just curious, why do you need this change?
Nevermind, it's explained in the description.