Uncrash BrowserFocusTest.*

Uncrash BrowserFocusTest.*

It didn't wait for things to finish, which was clearly broken.

TBR=xji

TEST=interactive_ui_tests:BrowserFocusTest.* on Linux, no crashes
BUG=50696
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54251

[Paweł Hajdan Jr., 2010-07-30 01:17:42]

The test was shutting down the browser before the respective pages have finished
loading. It seems wrong to me, that's why I have added RunAllPending calls.

Please verify whether it is the right fix. I was making the change in a hurry,
"fixing" the tree, so my analysis of things could be wrong.

Oh, and in fact, my original change to download code could be breaking things.
I'll investigate that too.

[James Su, 2010-07-30 01:22:41]

Looks good to me.

However I still don't understand why the tests crashes when exiting. IMO they
shouldn't crash at all regardless of this fix.

[Evan Stade, 2010-07-30 01:25:29]

On 2010/07/30 01:17:42, Paweł Hajdan Jr. wrote:
> The test was shutting down the browser before the respective pages have
finished
> loading. It seems wrong to me, that's why I have added RunAllPending calls.

why does that seem wrong? if the thing it's testing (focus) happens
synchronously, why do we need to wait for the loading to finish? chrome should
not crash if it shuts down while a page is loading...

> 
> Please verify whether it is the right fix. I was making the change in a hurry,
> "fixing" the tree, so my analysis of things could be wrong.
> 
> Oh, and in fact, my original change to download code could be breaking things.
> I'll investigate that too.

[James Su, 2010-07-30 01:33:37]

Agree. Actually when InProcessBrowserTest::QuitBrowsers() gets called after
running the test, it'll destroy all browsers in a running message loop. It
should have the same effect than the ui_test_utils::RunAllPendingInMessageLoop()
call added by this CL.

On 2010/07/30 01:25:29, Evan Stade wrote:
> On 2010/07/30 01:17:42, Paweł Hajdan Jr. wrote:
> > The test was shutting down the browser before the respective pages have
> finished
> > loading. It seems wrong to me, that's why I have added RunAllPending calls.
> 
> why does that seem wrong? if the thing it's testing (focus) happens
> synchronously, why do we need to wait for the loading to finish? chrome should
> not crash if it shuts down while a page is loading...
> 
> > 
> > Please verify whether it is the right fix. I was making the change in a
hurry,
> > "fixing" the tree, so my analysis of things could be wrong.
> > 
> > Oh, and in fact, my original change to download code could be breaking
things.
> > I'll investigate that too.

[Paweł Hajdan Jr., 2010-07-30 16:34:47]

Thanks for the review. Yeah, you're right. It's not a fix, rather a band-aid.

Brett helped me with fixing this properly. I'll send you a review link, it'll
undo these changes to the test. I'm sorry for calling it "broken" too early.