[Autofill] Fix MessageLoop in test.

[Autofill] Fix MessageLoop in test.

The old approach (RunMessageLoop) only works for dialogs that
actually post messages. This seems to be true for Views, but is
not true for the upcoming OSX dialogs.
R=isherman@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195588

[Ilya Sherman, 2013-04-13 03:14:28]

What do you mean by "dialogs that actually post messages"?  I'm not following
what's wrong with the existing pattern.  RunAllPendingInMessageLoop() is much
more fragile -- it requires that there's only one set of asynchronous events to
wait for -- so I'd strongly prefer to avoid relying on it in tests if possible.

[groby-ooo-7-16, 2013-04-13 03:25:45]

For OSX, ViewClosed() is reached from SubmitForTesting/CancelForTesting.

That means OSX will call MessageLoop::current()->Quit(), followed by
content::RunMessageLoop. And thus, the tests will never terminate on OSX.

There's another alternative which sticks with RunMessageLoop, but ensures
there's at least one pending message when it's called, even on OSX Sketched out
below - would that work better?


TestAutofillDialogController::ViewClosed()
{
  MessageLoop::Current->PostTask(FROM_HERE,
base::Bind(this,&TestAutofillDialogController::ViewClosed2));
}

TestAutofillDialogController::ViewClosed2()
{
  AutofillDialogControllerImpl::ViewClosed();
  MessageLoop::current()->Quit();
}

[Evan Stade, 2013-04-13 03:27:41]

https://codereview.chromium.org/14136005/diff/1/chrome/browser/ui/autofill/au...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/14136005/diff/1/chrome/browser/ui/autofill/au...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:89: virtual
void ViewClosed() OVERRIDE {
if you do go with this CL, you don't need this function any more

[Ilya Sherman, 2013-04-13 03:38:48]

On 2013/04/13 03:25:45, groby wrote:
> For OSX, ViewClosed() is reached from SubmitForTesting/CancelForTesting.
> 
> That means OSX will call MessageLoop::current()->Quit(), followed by
> content::RunMessageLoop. And thus, the tests will never terminate on OSX.

Ah, I see.  Sounds like what we want in that case is to use a MessageLoopRunner,
so that Run() calls will be ignored if they come after a Quit().

[groby-ooo-7-16, 2013-04-17 00:00:09]

PTAL

[Ilya Sherman, 2013-04-17 00:03:55]

LGTM, thanks

https://codereview.chromium.org/14136005/diff/8001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/14136005/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:109: void
RunMessageLoop(void) {
nit: Docs

https://codereview.chromium.org/14136005/diff/8001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:109: void
RunMessageLoop(void) {
nit: Omit the second "void"

[commit-bot: I haz the power, 2013-04-18 00:11:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/14136005/8001

[commit-bot: I haz the power, 2013-04-18 03:14:14]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[groby-ooo-7-16, 2013-04-19 19:30:20]

PTAL - my previous patch missed the fact that the controller self-destroys. New
patch ensures MessageLoopRunner stays around until both RunMessageLoop has been
called and the controller has been destroyed.

(Prev. patch worked locally since I used Release tests)

[Ilya Sherman, 2013-04-20 01:19:07]

LGTM

https://codereview.chromium.org/14136005/diff/35001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/14136005/diff/35001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:145: }
nit: Please leave a blank line after this one.

[commit-bot: I haz the power, 2013-04-20 02:28:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/14136005/35002

[groby-ooo-7-16, 2013-04-20 02:28:37]

https://codereview.chromium.org/14136005/diff/35001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc
(right):

https://codereview.chromium.org/14136005/diff/35001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_browsertest.cc:145: }
On 2013/04/20 01:19:07, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

[commit-bot: I haz the power, 2013-04-20 02:46:20]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-21 18:18:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/14136005/35002

[commit-bot: I haz the power, 2013-04-21 18:24:48]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-22 16:53:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/14136005/35002

[commit-bot: I haz the power, 2013-04-22 21:06:33]

Change committed as 195588