Adds unit tests for Offliner SavePage implementation.

Adds unit tests for Offliner SavePage implementation.

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

[dougarnett, 2016-05-27 18:06:09]

Description was changed from

==========
Adds unit tests for Offliner SavePage implementation.

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

to

==========
Adds unit tests for Offliner SavePage implementation.

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

[dougarnett, 2016-05-27 18:06:09]

dougarnett@chromium.org changed reviewers:
+ fgorski@chromium.org, petewil@chromium.org

[dougarnett, 2016-05-27 18:11:35]

2nd part of Offliner impl change => unit tests.

For the moment, this is extending the Offliner class in order to mock the
SavePage() call. Once OfflinePageModel change to be an interface
lands(https://codereview.chromium.org/2011763005), this part can be reworked.
But tests can be reviewed in the meantime as you have time.

[Pete Williamson, 2016-05-27 18:32:09]

New tests look good.

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

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:98: //
TODO(dougarnett): Remove this subclass once OfflinePageModel is split out
I'd prefer to delay checking in this changelist until the new interface has
landed and we can remove this inheritance.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:213:
EXPECT_FALSE(loading());
Which loading() function is being called here, the one on the test or the one on
the base class?  I didn't see one on the test, and we probably shouldn't be
calling the base class this way, instead we should be getting a pointer to it
from the test class.  We'll have to make that change when we move towards an
interface, might as well make it now.  Same comment for saving().

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:251:
EXPECT_FALSE(callback_called());
I'm a bit confused that callback_called() is returning false given that you just
called CallbackForLoaded().  Perhaps it is a different callback, and naming the
callback_called() function to show which callback would help.  Maybe
"save_callback_called()"?

[fgorski, 2016-05-27 18:43:11]

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

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:65:
mock_loading_ = false;
Just a consideration:
* Might mock_loading_ and mock_loaded_ a a tri-state enum really?
* Is there ever a case when they are both true?

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:115: void
CallbackForSaved() {
Can you consider this to be mirror for the  method above. E.g.
CallbackForSaveSucceeded()

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:123: bool
mock_saving() { return mock_saving_; }
I believe this method can be const.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:249:
loader()->CallbackForLoaded();
I am thinking whether there is a better name here.
How does loader()->CompleteLoad[ing]WithSuccess();  // or something like that
sound?

This would apply to saving as well.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:259:
EXPECT_TRUE(saving());
Could you add a comment and explain why saving is still in progress after
cancel?

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:290:
TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedAndSaved) {
How about: LoadAndSaveSuccessful

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:328:
EXPECT_FALSE(loaded());
Is there an assertion related to saving missing here?

[dougarnett, 2016-05-27 20:39:18]

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

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:65:
mock_loading_ = false;
On 2016/05/27 18:43:11, fgorski wrote:
> Just a consideration:
> * Might mock_loading_ and mock_loaded_ a a tri-state enum really?
> * Is there ever a case when they are both true?

True - was hoping two bools might be simpler to see here that the state_ and
enum values which are currently private (but other way to go would be to make
them protected and use here). 

No longer exposing these outside of the mock which should help with any
confusion in test case assertions anyway.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:98: //
TODO(dougarnett): Remove this subclass once OfflinePageModel is split out
On 2016/05/27 18:32:09, Pete Williamson wrote:
> I'd prefer to delay checking in this changelist until the new interface has
> landed and we can remove this inheritance.

I do too - hopefully that review can land soon. I'm slightly concerned its scope
might get expanded to cover more cleanups though.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:115: void
CallbackForSaved() {
On 2016/05/27 18:43:11, fgorski wrote:
> Can you consider this to be mirror for the  method above. E.g.
> CallbackForSaveSucceeded()

Went with the later Complete* naming suggested below

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:123: bool
mock_saving() { return mock_saving_; }
On 2016/05/27 18:43:11, fgorski wrote:
> I believe this method can be const.

Done.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:213:
EXPECT_FALSE(loading());
On 2016/05/27 18:32:09, Pete Williamson wrote:
> Which loading() function is being called here, the one on the test or the one
on
> the base class?  I didn't see one on the test, and we probably shouldn't be
> calling the base class this way, instead we should be getting a pointer to it
> from the test class.  We'll have to make that change when we move towards an
> interface, might as well make it now.  Same comment for saving().

Reworked to use IsIdle() and IsLoaded() methods on Loader instead.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:249:
loader()->CallbackForLoaded();
On 2016/05/27 18:43:11, fgorski wrote:
> I am thinking whether there is a better name here.
> How does loader()->CompleteLoad[ing]WithSuccess();  // or something like that
> sound?
> 
> This would apply to saving as well.

Done - trying new naming scheme

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:251:
EXPECT_FALSE(callback_called());
On 2016/05/27 18:32:09, Pete Williamson wrote:
> I'm a bit confused that callback_called() is returning false given that you
just
> called CallbackForLoaded().  Perhaps it is a different callback, and naming
the
> callback_called() function to show which callback would help.  Maybe
> "save_callback_called()"?

changed to completion_callback_called()

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:259:
EXPECT_TRUE(saving());
On 2016/05/27 18:43:11, fgorski wrote:
> Could you add a comment and explain why saving is still in progress after
> cancel?

Done.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:290:
TEST_F(PrerenderingOfflinerTest, LoadAndSaveLoadedAndSaved) {
On 2016/05/27 18:43:11, fgorski wrote:
> How about: LoadAndSaveSuccessful

Done.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:328:
EXPECT_FALSE(loaded());
On 2016/05/27 18:43:11, fgorski wrote:
> Is there an assertion related to saving missing here?

Done.

[Pete Williamson, 2016-05-27 21:06:46]

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

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:98: //
TODO(dougarnett): Remove this subclass once OfflinePageModel is split out
On 2016/05/27 20:39:18, dougarnett wrote:
> On 2016/05/27 18:32:09, Pete Williamson wrote:
> > I'd prefer to delay checking in this changelist until the new interface has
> > landed and we can remove this inheritance.
> 
> I do too - hopefully that review can land soon. I'm slightly concerned its
scope
> might get expanded to cover more cleanups though.

OK.  Still waiting for this one.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:213:
EXPECT_FALSE(loading());
On 2016/05/27 20:39:18, dougarnett wrote:
> On 2016/05/27 18:32:09, Pete Williamson wrote:
> > Which loading() function is being called here, the one on the test or the
one
> on
> > the base class?  I didn't see one on the test, and we probably shouldn't be
> > calling the base class this way, instead we should be getting a pointer to
it
> > from the test class.  We'll have to make that change when we move towards an
> > interface, might as well make it now.  Same comment for saving().
> 
> Reworked to use IsIdle() and IsLoaded() methods on Loader instead.

Acknowledged.

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:251:
EXPECT_FALSE(callback_called());
On 2016/05/27 20:39:18, dougarnett wrote:
> On 2016/05/27 18:32:09, Pete Williamson wrote:
> > I'm a bit confused that callback_called() is returning false given that you
> just
> > called CallbackForLoaded().  Perhaps it is a different callback, and naming
> the
> > callback_called() function to show which callback would help.  Maybe
> > "save_callback_called()"?
> 
> changed to completion_callback_called()

Acknowledged.

[fgorski, 2016-05-31 20:06:12]

lgtm

[dougarnett, 2016-06-01 17:51:10]

Now have mocked OfflinePageModel instead of extending offliner class. 

Added a FakeOfflinePageModel empty implementation to base such mocks classes on
(so they need only mock the methods their tests use). The Fake is just in a .h
file and I imagine I should have .cc file although I've been in Java land too
long to know why so please advise on that.

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

https://codereview.chromium.org/2016313003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:98: //
TODO(dougarnett): Remove this subclass once OfflinePageModel is split out
On 2016/05/27 21:06:46, Pete Williamson wrote:
> On 2016/05/27 20:39:18, dougarnett wrote:
> > On 2016/05/27 18:32:09, Pete Williamson wrote:
> > > I'd prefer to delay checking in this changelist until the new interface
has
> > > landed and we can remove this inheritance.
> > 
> > I do too - hopefully that review can land soon. I'm slightly concerned its
> scope
> > might get expanded to cover more cleanups though.
> 
> OK.  Still waiting for this one.

Done.

[fgorski, 2016-06-01 18:04:01]

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
components/offline_pages/BUILD.gn:81: "fake_offline_page_model.h",
I think this belongs in test_support

and components/offline_pages.gypi:offline_pages_test_support

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
File components/offline_pages/fake_offline_page_model.h (right):

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
components/offline_pages/fake_offline_page_model.h:19: class
FakeOfflinePageModel : public OfflinePageModel {
Did you consider naming it differently, e.g. TestOfflinePageModel,
OfflinePageTestModel (either one is good in case you want to add functionality
to this one). Or StubOfflinePageModel in case you keep it empty forever.

[Pete Williamson, 2016-06-01 19:47:47]

LGTM with comments.

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
File components/offline_pages/fake_offline_page_model.h (right):

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
components/offline_pages/fake_offline_page_model.h:19: class
FakeOfflinePageModel : public OfflinePageModel {
On 2016/06/01 18:04:00, fgorski wrote:
> Did you consider naming it differently, e.g. TestOfflinePageModel,
> OfflinePageTestModel (either one is good in case you want to add functionality
> to this one). Or StubOfflinePageModel in case you keep it empty forever.

I agree that StubOfflinePageModel is good.  I don't think that stubs need to be
completely empty, just to have no "real" implementation, they only do enough to
let the tests pass.

[dougarnett, 2016-06-01 20:28:28]

Stub it is!

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
File components/offline_pages/BUILD.gn (right):

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
components/offline_pages/BUILD.gn:81: "fake_offline_page_model.h",
On 2016/06/01 18:04:00, fgorski wrote:
> I think this belongs in test_support
> 
> and components/offline_pages.gypi:offline_pages_test_support

Thanks - Done

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
File components/offline_pages/fake_offline_page_model.h (right):

https://codereview.chromium.org/2016313003/diff/60001/components/offline_page...
components/offline_pages/fake_offline_page_model.h:19: class
FakeOfflinePageModel : public OfflinePageModel {
Done

[dougarnett, 2016-06-01 23:26:42]

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

[commit-bot: I haz the power, 2016-06-01 23:27:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2016313003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2016313003/80001

[commit-bot: I haz the power, 2016-06-01 23:42:37]

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

[commit-bot: I haz the power, 2016-06-01 23:42:38]

Dry run: This issue passed the CQ dry run.

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

The CQ bit was checked by dougarnett@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, petewil@chromium.org
Link to the patchset: https://codereview.chromium.org/2016313003/#ps80001
(title: "Renamed Fake* to Stub* and added files to proper build target")

[commit-bot: I haz the power, 2016-06-02 15:18:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2016313003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2016313003/80001

[commit-bot: I haz the power, 2016-06-02 15:24:01]

Description was changed from

==========
Adds unit tests for Offliner SavePage implementation.

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

to

==========
Adds unit tests for Offliner SavePage implementation.

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

[commit-bot: I haz the power, 2016-06-02 15:24:02]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-02 15:25:49]

Description was changed from

==========
Adds unit tests for Offliner SavePage implementation.

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

to

==========
Adds unit tests for Offliner SavePage implementation.

BUG=601173

Committed: https://crrev.com/8c5b5db77e83c7ebb5878123358375771287cde7
Cr-Commit-Position: refs/heads/master@{#397411}
==========

[commit-bot: I haz the power, 2016-06-02 15:25:50]

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