Adds unit tests for offline_pages::PrerenderAdapter calling PrerenderManager.

Adds unit tests for offline_pages::PrerenderAdapter calling PrerenderManager.

BUG=601173
Committed: https://crrev.com/db13d18a0372ba0ddf03f7c1901dba898b713528
Cr-Commit-Position: refs/heads/master@{#400185}

[dougarnett, 2016-06-07 00:16:30]

dougarnett@chromium.org changed reviewers:
+ gabadie@chromium.org, pasko@chromium.org, petewil@chromium.org

[dougarnett, 2016-06-07 00:16:31]

Preliminary. If approach looks viable, will add observer tests too.

I'm looking for a reasonable way to add test coverage to the
offline_pages::PrerenderAdapter.
The approach here is to follow the pattern of unit testing used by
prerender_unittests.cc. In
particular, it defines a PrerenderManager subclass named
UnittestPrerenderManager and a dummy/stub
subclass of PrerenderContents. These are able to override
PM::CreatePrerenderContents() and 
PC::StartPrerendering() methods. In order to do this, there is one change in the
prerender codebase
to make the offline_pages::UnittestPrerenderManager a friend. That seemed like
the simplist change
- that is, to work with the existing virtual methods but happy to hear of a
better ideas. Please advise.

[pasko, 2016-06-07 11:32:48]

On 2016/06/07 00:16:31, dougarnett wrote:
> Preliminary. If approach looks viable, will add observer tests too.
> 
> I'm looking for a reasonable way to add test coverage to the
> offline_pages::PrerenderAdapter.

If the goal is to cover all methods of PrerenderAdapter, then the approach looks
viable to me.

The part I am not sure about is whether we actually need verbose testing of this
thin layer (which we designed to be as thin as possible). It is not easy to make
mistakes in PrerenderAdapter code, but it is easy to mess up the more
complicated tests and make them crashy/flaky with various ownership issues (like
"who owns the various stubs?").

So I am not super excited, but I would LG-TM if you really want to do this kind
of testing.

> The approach here is to follow the pattern of unit testing used by
> prerender_unittests.cc. In
> particular, it defines a PrerenderManager subclass named
> UnittestPrerenderManager and a dummy/stub
> subclass of PrerenderContents.

I am wondering whether composition would be less verbose and less surprising
here, not sure now, please just consider composition as an option for
PrerenderContents stub.

> These are able to override
> PM::CreatePrerenderContents() and 
> PC::StartPrerendering() methods. In order to do this, there is one change in
the
> prerender codebase
> to make the offline_pages::UnittestPrerenderManager a friend. That seemed like
> the simplist change
> - that is, to work with the existing virtual methods but happy to hear of a
> better ideas. Please advise.

This looks ok to me (but mmenke@ may have another opinion)

[pasko, 2016-06-07 11:33:00]

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:49:
prerender::PrerenderContents* CreatePrerenderContents(
or, alternatively, make this method protected.  Not sure, maybe you need more
methods.

[gabadie, 2016-06-07 12:02:08]

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:130:
EXPECT_TRUE(adapter()->CanPrerender());
CanPrerender() { return true; } would pass the tests as well. I think a better
would be to add making sure CanPrerender() return false if
prerender::PrerenderManager::ActuallyPrerendering() return false.

[dougarnett, 2016-06-07 16:42:22]

On 2016/06/07 11:32:48, pasko wrote:
> On 2016/06/07 00:16:31, dougarnett wrote:
> > Preliminary. If approach looks viable, will add observer tests too.
> > 
> > I'm looking for a reasonable way to add test coverage to the
> > offline_pages::PrerenderAdapter.
> 
> If the goal is to cover all methods of PrerenderAdapter, then the approach
looks
> viable to me.
> 
> The part I am not sure about is whether we actually need verbose testing of
this
> thin layer (which we designed to be as thin as possible). It is not easy to
make
> mistakes in PrerenderAdapter code, but it is easy to mess up the more
> complicated tests and make them crashy/flaky with various ownership issues
(like
> "who owns the various stubs?").
> 
> So I am not super excited, but I would LG-TM if you really want to do this
kind
> of testing.

The adapter ended up a bit more involved than direct calls so wanted to cover
some
of its IsActive state-based logic plus wanted some trybot-time checks on
divergence
between the components that compiler might not catch. I don't mean for it to be
very verbose.

> > The approach here is to follow the pattern of unit testing used by
> > prerender_unittests.cc. In
> > particular, it defines a PrerenderManager subclass named
> > UnittestPrerenderManager and a dummy/stub
> > subclass of PrerenderContents.
> 
> I am wondering whether composition would be less verbose and less surprising
> here, not sure now, please just consider composition as an option for
> PrerenderContents stub.

Not sure I understand how you mean to use composition - just return a real
PrerenderContents and let its StartPrerendering() try to do its thing? That
sounds more brittle than if I can intercept that method call and just verify
that it was made.
 
> > These are able to override
> > PM::CreatePrerenderContents() and 
> > PC::StartPrerendering() methods. In order to do this, there is one change in
> the
> > prerender codebase
> > to make the offline_pages::UnittestPrerenderManager a friend. That seemed
like
> > the simplist change
> > - that is, to work with the existing virtual methods but happy to hear of a
> > better ideas. Please advise.
> 
> This looks ok to me (but mmenke@ may have another opinion)

Ok, will add mmenke@. Just making the
PrerenderManager::CreatePrerenderContents() protected
would be sufficient for this approach (and seems better, more isolated than
friend so
happy to revise that way if there is agreement).

[dougarnett, 2016-06-07 16:45:38]

dougarnett@chromium.org changed reviewers:
+ mmenke@chromium.org

[dougarnett, 2016-06-07 16:45:39]

Adding mmenke@ for guidance - in particular, on making CreatePrerenderContents
protected or adding subclass as friend or other ideas to make
PrerenderManager.AddPrerender() path testable.

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:49:
prerender::PrerenderContents* CreatePrerenderContents(
On 2016/06/07 11:33:00, pasko wrote:
> or, alternatively, make this method protected.  Not sure, maybe you need more
> methods.

Yes, making this protected is sufficient and seems preferable to me.

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:130:
EXPECT_TRUE(adapter()->CanPrerender());
On 2016/06/07 12:02:08, gabadie wrote:
> CanPrerender() { return true; } would pass the tests as well. I think a better
> would be to add making sure CanPrerender() return false if
> prerender::PrerenderManager::ActuallyPrerendering() return false.

True - but really just wanted to exercise the direct call to
ActuallyPrerendering() call here at least once to be sure it is allowed. I can
add another test if you think it is important and this CL moves forward.

[Pete Williamson, 2016-06-07 17:30:51]

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:37: class
UnitTestPrerenderManager : public prerender::PrerenderManager {
Instead of inheriting from the PrerenderManager, should we be inheriting from an
interface that PrerenderManager implements?  We want to test the adapter, not
the prerenderManager as I understand it.  This may mean splitting
PrerenderManager into interface and implementation classes.

[gabadie, 2016-06-07 18:41:12]

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:130:
EXPECT_TRUE(adapter()->CanPrerender());
On 2016/06/07 16:45:39, dougarnett wrote:
> On 2016/06/07 12:02:08, gabadie wrote:
> > CanPrerender() { return true; } would pass the tests as well. I think a
better
> > would be to add making sure CanPrerender() return false if
> > prerender::PrerenderManager::ActuallyPrerendering() return false.
> 
> True - but really just wanted to exercise the direct call to
> ActuallyPrerendering() call here at least once to be sure it is allowed. I can
> add another test if you think it is important and this CL moves forward.

I think it would improve this CL. I think this same test should also tests what
append in StartPrerender() if CanPrerender() == false

[mmenke, 2016-06-09 15:43:51]

On 2016/06/07 16:45:39, dougarnett wrote:
> Adding mmenke@ for guidance - in particular, on making CreatePrerenderContents
> protected or adding subclass as friend or other ideas to make
> PrerenderManager.AddPrerender() path testable.

There's already a way to inject your own PrerenderContents -
PrerenderManager::SetPrerenderContentsFactory.  Does this not meet your needs? 
I'm not a big fan of making MockPrerenderContents that inherit from the real
PrerenderContents class, but looks like that does work, and is done in
prerender_unittest.cc.

>
https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
> File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc
(right):
> 
>
https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
> chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:49:
> prerender::PrerenderContents* CreatePrerenderContents(
> On 2016/06/07 11:33:00, pasko wrote:
> > or, alternatively, make this method protected.  Not sure, maybe you need
more
> > methods.
> 
> Yes, making this protected is sufficient and seems preferable to me.
> 
>
https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
> chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:130:
> EXPECT_TRUE(adapter()->CanPrerender());
> On 2016/06/07 12:02:08, gabadie wrote:
> > CanPrerender() { return true; } would pass the tests as well. I think a
better
> > would be to add making sure CanPrerender() return false if
> > prerender::PrerenderManager::ActuallyPrerendering() return false.
> 
> True - but really just wanted to exercise the direct call to
> ActuallyPrerendering() call here at least once to be sure it is allowed. I can
> add another test if you think it is important and this CL moves forward.

[dougarnett, 2016-06-09 17:20:25]

On 2016/06/09 15:43:51, mmenke wrote:
> On 2016/06/07 16:45:39, dougarnett wrote:
> > Adding mmenke@ for guidance - in particular, on making
CreatePrerenderContents
> > protected or adding subclass as friend or other ideas to make
> > PrerenderManager.AddPrerender() path testable.
> 
> There's already a way to inject your own PrerenderContents -
> PrerenderManager::SetPrerenderContentsFactory.  Does this not meet your needs?

> I'm not a big fan of making MockPrerenderContents that inherit from the real
> PrerenderContents class, but looks like that does work, and is done in
> prerender_unittest.cc.

Great, looks like SetPrerenderContentsFactory will work for injecting the mocked
contents (so no need to necessarily touch prerender code base after all - yay!).

I'm not a big fan of extending actual PrerenderContents either - I wish it had
an interface. 
I do see another use of it (along with the SetPrerenderContentsFactory) in: 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
Any other ideas here short of refactoring PrerenderContents into separate
interface and implementation?

Ideally, for my unittest purposes, I wish I could create a mock PrerenderHandle
and return
it from an overridden PrerenderManager::AddPrerender(). That would be the
cleanest to understand from the calling code perspective. I am only needing to
mock PrerenderContents and get it injected in order for PrerenderManager to
return me a PrerenderHandle instance. So if we are motivated to do some
refactoring in the prerender stack to make it more unittestable by callers, I
might be more interested in exploring that angle.

[mmenke, 2016-06-09 17:41:42]

On 2016/06/09 17:20:25, dougarnett wrote:
> On 2016/06/09 15:43:51, mmenke wrote:
> > On 2016/06/07 16:45:39, dougarnett wrote:
> > > Adding mmenke@ for guidance - in particular, on making
> CreatePrerenderContents
> > > protected or adding subclass as friend or other ideas to make
> > > PrerenderManager.AddPrerender() path testable.
> > 
> > There's already a way to inject your own PrerenderContents -
> > PrerenderManager::SetPrerenderContentsFactory.  Does this not meet your
needs?
> 
> > I'm not a big fan of making MockPrerenderContents that inherit from the real
> > PrerenderContents class, but looks like that does work, and is done in
> > prerender_unittest.cc.
> 
> Great, looks like SetPrerenderContentsFactory will work for injecting the
mocked
> contents (so no need to necessarily touch prerender code base after all -
yay!).
> 
> I'm not a big fan of extending actual PrerenderContents either - I wish it had
> an interface. 
> I do see another use of it (along with the SetPrerenderContentsFactory) in: 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> Any other ideas here short of refactoring PrerenderContents into separate
> interface and implementation?
> 
> Ideally, for my unittest purposes, I wish I could create a mock
PrerenderHandle
> and return
> it from an overridden PrerenderManager::AddPrerender(). That would be the
> cleanest to understand from the calling code perspective. I am only needing to
> mock PrerenderContents and get it injected in order for PrerenderManager to
> return me a PrerenderHandle instance. So if we are motivated to do some
> refactoring in the prerender stack to make it more unittestable by callers, I
> might be more interested in exploring that angle.

Yea, I was going to suggest thinking about mocking out PrerenderHandles instead
(You'd need to either mock out PrerenderManager, too, or provide a
PrerenderHAndleFactory, or make a method that creates PrerenderHandles and
overrides it).

I guess we still don't run BrowserTests on Android, so you can't really add any
full integration tests as well.

[dougarnett, 2016-06-11 00:21:55]

PTAL - switch to using a PrerenderContentsFactory and added coverage for
observer paths.

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:37: class
UnitTestPrerenderManager : public prerender::PrerenderManager {
On 2016/06/07 17:30:51, Pete Williamson wrote:
> Instead of inheriting from the PrerenderManager, should we be inheriting from
an
> interface that PrerenderManager implements?  We want to test the adapter, not
> the prerenderManager as I understand it.  This may mean splitting
> PrerenderManager into interface and implementation classes.

Change to injecting PrerenderContentsFactory. 
[Btw, if we did create interfaces for PrerenderManager and PrerenderHandle had
interfaces, we could drop the PrerenderAdapter]

https://codereview.chromium.org/2044613003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:130:
EXPECT_TRUE(adapter()->CanPrerender());
On 2016/06/07 18:41:12, gabadie wrote:
> On 2016/06/07 16:45:39, dougarnett wrote:
> > On 2016/06/07 12:02:08, gabadie wrote:
> > > CanPrerender() { return true; } would pass the tests as well. I think a
> better
> > > would be to add making sure CanPrerender() return false if
> > > prerender::PrerenderManager::ActuallyPrerendering() return false.
> > 
> > True - but really just wanted to exercise the direct call to
> > ActuallyPrerendering() call here at least once to be sure it is allowed. I
can
> > add another test if you think it is important and this CL moves forward.
> 
> I think it would improve this CL. I think this same test should also tests
what
> append in StartPrerender() if CanPrerender() == false

Added CanPrerender == false test.

[mmenke, 2016-06-11 00:40:32]

https://codereview.chromium.org/2044613003/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/2044613003/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.h:401: friend class
offline_pages::PrerenderAdapterTest;
Is this still needed?  If so, I recommend adding BlahForTest[ing] methods
instead.  That gives a more defined API to deal with.  I know nothing else is
doing that, but IMHO, they should be.

[Pete Williamson, 2016-06-11 00:52:02]

lgtm

[dougarnett, 2016-06-12 16:43:09]

https://codereview.chromium.org/2044613003/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/2044613003/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.h:401: friend class
offline_pages::PrerenderAdapterTest;
On 2016/06/11 00:40:32, mmenke wrote:
> Is this still needed?  If so, I recommend adding BlahForTest[ing] methods
> instead.  That gives a more defined API to deal with.  I know nothing else is
> doing that, but IMHO, they should be.

SetPrerenderContentsFactory() is what the test needs access to (possibly all
that the ::InstantSearchPrerendererTest friend above needs too).

Looks like this is only used for browsertest and instantsearch test so options
might be to just make public named as is or make public and revise name to
*ForTest and change the callers.

[mmenke, 2016-06-13 14:52:59]

LGTM, modulo comments - no need to wait for another signoff from me.  Don't want
to delay you more than I already have.

https://codereview.chromium.org/2044613003/diff/80001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_manager.h (right):

https://codereview.chromium.org/2044613003/diff/80001/chrome/browser/prerende...
chrome/browser/prerender/prerender_manager.h:401: friend class
offline_pages::PrerenderAdapterTest;
On 2016/06/12 16:43:09, dougarnett wrote:
> On 2016/06/11 00:40:32, mmenke wrote:
> > Is this still needed?  If so, I recommend adding BlahForTest[ing] methods
> > instead.  That gives a more defined API to deal with.  I know nothing else
is
> > doing that, but IMHO, they should be.
> 
> SetPrerenderContentsFactory() is what the test needs access to (possibly all
> that the ::InstantSearchPrerendererTest friend above needs too).
> 
> Looks like this is only used for browsertest and instantsearch test so options
> might be to just make public named as is or make public and revise name to
> *ForTest and change the callers. 

Could you please rename it to SetPrerenderContentsFactoryForTest, and make it
public, and update all callers.

If that allows you to remove the friend declaration for
::InstantSearchPrerendererTest (Or anything else), please do so.

[dougarnett, 2016-06-13 18:10:21]

On 2016/06/13 14:52:59, mmenke wrote:
> LGTM, modulo comments - no need to wait for another signoff from me.  Don't
want
> to delay you more than I already have.
> 
>
https://codereview.chromium.org/2044613003/diff/80001/chrome/browser/prerende...
> File chrome/browser/prerender/prerender_manager.h (right):
> 
>
https://codereview.chromium.org/2044613003/diff/80001/chrome/browser/prerende...
> chrome/browser/prerender/prerender_manager.h:401: friend class
> offline_pages::PrerenderAdapterTest;
> On 2016/06/12 16:43:09, dougarnett wrote:
> > On 2016/06/11 00:40:32, mmenke wrote:
> > > Is this still needed?  If so, I recommend adding BlahForTest[ing] methods
> > > instead.  That gives a more defined API to deal with.  I know nothing else
> is
> > > doing that, but IMHO, they should be.
> > 
> > SetPrerenderContentsFactory() is what the test needs access to (possibly all
> > that the ::InstantSearchPrerendererTest friend above needs too).
> > 
> > Looks like this is only used for browsertest and instantsearch test so
options
> > might be to just make public named as is or make public and revise name to
> > *ForTest and change the callers. 
> 
> Could you please rename it to SetPrerenderContentsFactoryForTest, and make it
> public, and update all callers.
> 
> If that allows you to remove the friend declaration for
> ::InstantSearchPrerendererTest (Or anything else), please do so.

Updated as requested - public SetPrerenderContentsFactoryForTest()

Egor and Guillaume, let me know if not satisfied with this resolution or other
concerns.

[dougarnett, 2016-06-13 20:06:46]

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

[commit-bot: I haz the power, 2016-06-13 20:08:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044613003/100001

[commit-bot: I haz the power, 2016-06-13 20:13:49]

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

[commit-bot: I haz the power, 2016-06-13 20:13:51]

Dry run: This issue passed the CQ dry run.

[dougarnett, 2016-06-13 20:45:27]

Description was changed from

==========
Adds unit tests for offline_pages::PrerenderAdapter calling PrerenderManager.

BUG=601173
==========

to

==========
Adds unit tests for offline_pages::PrerenderAdapter calling PrerenderManager.

BUG=601173
==========

[dougarnett, 2016-06-13 20:45:27]

dougarnett@chromium.org changed reviewers:
+ kmadhusu@chromium.org

[dougarnett, 2016-06-13 20:48:25]

+kmadhusu - please review instant_search_prerenderer_unittest.cc

Renamed PrerenderManager method with *ForTest suffix and made public (and also
removed PrerenderManager friend status for instant search unit tests).

[pasko, 2016-06-15 12:25:00]

sorry for slow response, various conferences

lgtm to unblock, a few comments below that you might want to address

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:177: void
PrerenderAdapterTest::SetUp() {
also I think various *_called should be reset in SetUp()

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:202:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
DCHECKs are not nice in tests because they kill the process, can this be
converted to ASSERT_*?

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:207: }
it would be nice to also EXPECT_FALSE for these:
observer_start_called()
observer_dom_content_loaded_called()
observer_stop_loading_called()
observer_stop_called()

[dougarnett, 2016-06-15 15:45:04]

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:177: void
PrerenderAdapterTest::SetUp() {
On 2016/06/15 12:25:00, pasko wrote:
> also I think various *_called should be reset in SetUp()

thanks - done

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:202:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2016/06/15 12:25:00, pasko wrote:
> DCHECKs are not nice in tests because they kill the process, can this be
> converted to ASSERT_*?

Unfortunately I don't find an ASSERT_CURRENTLY_ON macro and the DCHECK macros
comparision is hard to read so don't want to EXPECT here on it. 

Actually, I don't think I need them in the tests so just keeping the one in
Setup to ensure the test is set up on the right thread.

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:207: }
On 2016/06/15 12:25:00, pasko wrote:
> it would be nice to also EXPECT_FALSE for these:
> observer_start_called()
> observer_dom_content_loaded_called()
> observer_stop_loading_called()
> observer_stop_called()

Done.

[dougarnett, 2016-06-15 15:45:33]

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

[commit-bot: I haz the power, 2016-06-15 15:46:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044613003/120001

[commit-bot: I haz the power, 2016-06-15 16:37:21]

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

[commit-bot: I haz the power, 2016-06-15 16:37:27]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-06-15 17:20:27]

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:202:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2016/06/15 15:45:04, dougarnett wrote:
> On 2016/06/15 12:25:00, pasko wrote:
> > DCHECKs are not nice in tests because they kill the process, can this be
> > converted to ASSERT_*?
> 
> Unfortunately I don't find an ASSERT_CURRENTLY_ON macro and the DCHECK macros
> comparision is hard to read so don't want to EXPECT here on it. 
> 
> Actually, I don't think I need them in the tests so just keeping the one in
> Setup to ensure the test is set up on the right thread.

There is ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI))

Totally up to you :)

[dougarnett, 2016-06-15 18:15:07]

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
File chrome/browser/android/offline_pages/prerender_adapter_unittest.cc (right):

https://codereview.chromium.org/2044613003/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/prerender_adapter_unittest.cc:202:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2016/06/15 17:20:27, pasko wrote:
> On 2016/06/15 15:45:04, dougarnett wrote:
> > On 2016/06/15 12:25:00, pasko wrote:
> > > DCHECKs are not nice in tests because they kill the process, can this be
> > > converted to ASSERT_*?
> > 
> > Unfortunately I don't find an ASSERT_CURRENTLY_ON macro and the DCHECK
macros
> > comparision is hard to read so don't want to EXPECT here on it. 
> > 
> > Actually, I don't think I need them in the tests so just keeping the one in
> > Setup to ensure the test is set up on the right thread.
> 
> There is ASSERT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::UI))
> 
> Totally up to you :)

Duh - Done ;-)

[dougarnett, 2016-06-15 18:16:57]

dougarnett@chromium.org changed reviewers:
+ treib@chromium.org

[dougarnett, 2016-06-15 18:18:29]

+treib - Marc, can you review instant_search_prerenderer_unittest.cc for simple
API change

[Marc Treib, 2016-06-16 10:18:40]

On 2016/06/15 18:18:29, dougarnett wrote:
> +treib - Marc, can you review instant_search_prerenderer_unittest.cc for
simple
> API change

instant_search_prerenderer_unittest.cc LGTM

[dougarnett, 2016-06-16 15:52:47]

dougarnett@chromium.org changed reviewers:
- kmadhusu@chromium.org

[dougarnett, 2016-06-16 15:52:56]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-06-16 15:52:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, mmenke@chromium.org, pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2044613003/#ps140001
(title: "Updated DCHECK to ASSERT")

[commit-bot: I haz the power, 2016-06-16 15:53:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2044613003/140001

[commit-bot: I haz the power, 2016-06-16 17:19:48]

Description was changed from

==========
Adds unit tests for offline_pages::PrerenderAdapter calling PrerenderManager.

BUG=601173
==========

to

==========
Adds unit tests for offline_pages::PrerenderAdapter calling PrerenderManager.

BUG=601173
==========

[commit-bot: I haz the power, 2016-06-16 17:19:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-16 17:19:53]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 17:21:23]

Description was changed from

==========
Adds unit tests for offline_pages::PrerenderAdapter calling PrerenderManager.

BUG=601173
==========

to

==========
Adds unit tests for offline_pages::PrerenderAdapter calling PrerenderManager.

BUG=601173

Committed: https://crrev.com/db13d18a0372ba0ddf03f7c1901dba898b713528
Cr-Commit-Position: refs/heads/master@{#400185}
==========

[commit-bot: I haz the power, 2016-06-16 17:21:25]

Patchset 8 (id:??) landed as
https://crrev.com/db13d18a0372ba0ddf03f7c1901dba898b713528
Cr-Commit-Position: refs/heads/master@{#400185}