Wait for history backend in popup blocker test

Wait for history backend in popup blocker test

BUG=663333
R=avi@chromium.org

Committed: https://crrev.com/3c6423fb4576679a36f1dbbf547e13fb37eafee2
Cr-Commit-Position: refs/heads/master@{#431794}

[jochen (gone - plz use gerrit), 2016-11-12 20:05:06]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-12 20:05:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-11-12 20:35:55]

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
File chrome/browser/history/history_test_utils.cc (right):

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
chrome/browser/history/history_test_utils.cc:36:
base::MessageLoop::current()->QuitWhenIdle();
Use a MessageLoopRunner.

[jochen (gone - plz use gerrit), 2016-11-12 20:48:21]

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
File chrome/browser/history/history_test_utils.cc (right):

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
chrome/browser/history/history_test_utils.cc:36:
base::MessageLoop::current()->QuitWhenIdle();
On 2016/11/12 at 20:35:55, Avi wrote:
> Use a MessageLoopRunner.

not sure I understand? MessageLoopRunner doesn't have support for QuitWhenIdle?

[Avi (use Gerrit), 2016-11-12 20:58:41]

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
File chrome/browser/history/history_test_utils.cc (right):

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
chrome/browser/history/history_test_utils.cc:36:
base::MessageLoop::current()->QuitWhenIdle();
On 2016/11/12 20:48:21, jochen (travelling til Nov 18) wrote:
> On 2016/11/12 at 20:35:55, Avi wrote:
> > Use a MessageLoopRunner.
> 
> not sure I understand? MessageLoopRunner doesn't have support for
QuitWhenIdle?

If you look at content/public/test/test_utils.cc line 50, it essentially
emulates QuitWhenIdle, so I think it's a drop-in replacement.

[commit-bot: I haz the power, 2016-11-12 20:58:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-12 20:58:57]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-11-12 21:10:54]

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
File chrome/browser/history/history_test_utils.cc (right):

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
chrome/browser/history/history_test_utils.cc:36:
base::MessageLoop::current()->QuitWhenIdle();
On 2016/11/12 at 20:58:41, Avi wrote:
> On 2016/11/12 20:48:21, jochen (travelling til Nov 18) wrote:
> > On 2016/11/12 at 20:35:55, Avi wrote:
> > > Use a MessageLoopRunner.
> > 
> > not sure I understand? MessageLoopRunner doesn't have support for
QuitWhenIdle?
> 
> If you look at content/public/test/test_utils.cc line 50, it essentially
emulates QuitWhenIdle, so I think it's a drop-in replacement.

yeah, but it gives up after a few tasks and quits anyways. I can see how that
makes sense for a message loop that has always tasks on it, but the history
backend loop should really be idle...

I'm also a bit hesitant about changing this code, as I just extracted it from
the existing history_browsertest.cc and I don't know for sure whether it depends
on this difference or not.

[Avi (use Gerrit), 2016-11-12 21:23:00]

lgtm

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
File chrome/browser/history/history_test_utils.cc (right):

https://codereview.chromium.org/2495053003/diff/1/chrome/browser/history/hist...
chrome/browser/history/history_test_utils.cc:36:
base::MessageLoop::current()->QuitWhenIdle();
On 2016/11/12 21:10:54, jochen (travelling til Nov 18) wrote:
> On 2016/11/12 at 20:58:41, Avi wrote:
> > On 2016/11/12 20:48:21, jochen (travelling til Nov 18) wrote:
> > > On 2016/11/12 at 20:35:55, Avi wrote:
> > > > Use a MessageLoopRunner.
> > > 
> > > not sure I understand? MessageLoopRunner doesn't have support for
> QuitWhenIdle?
> > 
> > If you look at content/public/test/test_utils.cc line 50, it essentially
> emulates QuitWhenIdle, so I think it's a drop-in replacement.
> 
> yeah, but it gives up after a few tasks and quits anyways. I can see how that
> makes sense for a message loop that has always tasks on it, but the history
> backend loop should really be idle...
> 
> I'm also a bit hesitant about changing this code, as I just extracted it from
> the existing history_browsertest.cc and I don't know for sure whether it
depends
> on this difference or not.

Fair enough. I'm really not a fan of hammering directly on the MessageLoop,
because MessageLoopRunner has protections against things like the condition
firing before the wait happens, but I'll let this go.

[jochen (gone - plz use gerrit), 2016-11-12 22:20:42]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2016-11-12 22:20:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-12 22:24:14]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-12 22:25:57]

Description was changed from

==========
Wait for history backend in popup blocker test

BUG=663333
R=avi@chromium.org
==========

to

==========
Wait for history backend in popup blocker test

BUG=663333
R=avi@chromium.org

Committed: https://crrev.com/3c6423fb4576679a36f1dbbf547e13fb37eafee2
Cr-Commit-Position: refs/heads/master@{#431794}
==========

[commit-bot: I haz the power, 2016-11-12 22:25:58]

Patchset 1 (id:??) landed as
https://crrev.com/3c6423fb4576679a36f1dbbf547e13fb37eafee2
Cr-Commit-Position: refs/heads/master@{#431794}