Updates to PrerenderingOffliner and PrerenderingLoader interfaces to push PrerenderManager knowledge

Updates to PrerenderingOffliner and PrerenderingLoader interfaces to push
PrerenderManager knowledge down into Loader only. Also adds initial
unit tests for Offliner with mocked Loader. This CL to be followed by
a Loader CL that integrates to PrerenderManager. Fleshing out the Offliner
logic more will come after that.

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

[dougarnett, 2016-05-09 16:22:13]

Description was changed from

==========
Updates to PrerenderingOffliner and PrerenderingLoader interfaces to push
PrerenderManager knowledge down into Loader only. Also adds initial
unit tests for Offliner with mocked Loader. This CL to be followed by
a Loader CL that integrates to PrerenderManager. Fleshing out the Offliner
logic more will come after that.

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

to

==========
Updates to PrerenderingOffliner and PrerenderingLoader interfaces to push
PrerenderManager knowledge down into Loader only. Also adds initial
unit tests for Offliner with mocked Loader. This CL to be followed by
a Loader CL that integrates to PrerenderManager. Fleshing out the Offliner
logic more will come after that.

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

[dougarnett, 2016-05-09 16:22:13]

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

[Pete Williamson, 2016-05-09 16:49:38]

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_offliner.h (right):

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:29:
PrerenderingOffliner(content::BrowserContext* browser_context,
We could probably make content::BrowserContext const here.

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:41:
PrerenderingOffliner(content::BrowserContext* browser_context,
and here...

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:47: void
OnLoadPageDone(Offliner::CompletionStatus load_status,
load_status can also likely be const.  Might be a good idea to check in other
places too, and make const whatever we can.

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

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:16: class
MockPrerenderingLoader : public PrerenderingLoader {
Is there any other class that might need the MockPrerenderingLoader?  I suspect
not, but if so, consider moving this class to its own file.

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:80:
PrerenderingOffliner* offliner_;
How do these get destroyed?

[dougarnett, 2016-05-09 17:42:04]

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_offliner.h (right):

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:29:
PrerenderingOffliner(content::BrowserContext* browser_context,
On 2016/05/09 16:49:38, Pete Williamson wrote:
> We could probably make content::BrowserContext const here.

I don't think so as we want to get Profile from it and this doesn't take const:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

Also, the PrerenderManager ctor does not take const Profile.

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:41:
PrerenderingOffliner(content::BrowserContext* browser_context,
On 2016/05/09 16:49:38, Pete Williamson wrote:
> and here...

ditto

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:47: void
OnLoadPageDone(Offliner::CompletionStatus load_status,
On 2016/05/09 16:49:38, Pete Williamson wrote:
> load_status can also likely be const.  Might be a good idea to check in other
> places too, and make const whatever we can.

Done.

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

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:16: class
MockPrerenderingLoader : public PrerenderingLoader {
On 2016/05/09 16:49:38, Pete Williamson wrote:
> Is there any other class that might need the MockPrerenderingLoader?  I
suspect
> not, but if so, consider moving this class to its own file.

At this point I see it as private to Offliner but we may add some test framework
support to drive testing of loading pages and testing that test framework may
motivate breaking it out. Would prefer to wait until and if we do integrate at
the Loader level though.

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:80:
PrerenderingOffliner* offliner_;
On 2016/05/09 16:49:38, Pete Williamson wrote:
> How do these get destroyed?

Done.

[Pete Williamson, 2016-05-09 17:55:40]

lgtm

[fgorski, 2016-05-09 20:38:32]

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/prerendering_offliner.h (right):

https://codereview.chromium.org/1961083002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:47: void
OnLoadPageDone(Offliner::CompletionStatus load_status,
On 2016/05/09 17:42:03, dougarnett wrote:
> On 2016/05/09 16:49:38, Pete Williamson wrote:
> > load_status can also likely be const.  Might be a good idea to check in
other
> > places too, and make const whatever we can.
> 
> Done.

It is an enum, hence no need to const it.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.h (right):

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:44:
PrerenderingLoader* loader);
Since you are taking ownership of the loader, use unique_ptr to pass it as well,
that way it is clear that you are taking it over.

Also, seeing how the TestPrerenderingLoader is implemented, I think you free up
loader memory twice. Once when destroying PrerenderingOffliner, and once when
completing the test (also held by the test in unique_ptr). I'll comment more in
the test.

There is an alternative to defining this constructor:
A SetLoaderForTesting method, that accepts a unique_ptr.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
(right):

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:18: class
MockPrerenderingLoader : public PrerenderingLoader {
can you put this in the anonymous namespace too?

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:40: class
TestPrerenderingOffliner : public PrerenderingOffliner {
When you have SetLoaderForTesting I don't think this is needed.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:40: class
TestPrerenderingOffliner : public PrerenderingOffliner {
ditto for namespace.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:83:
std::unique_ptr<MockPrerenderingLoader> loader_;
this frees the loader memory or the second time. Perhaps that is why you crash.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:94:
loader_.reset(new MockPrerenderingLoader(nullptr));
simply create a unique ptr and move it into the offliner_

[dougarnett, 2016-05-09 22:25:11]

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.h (right):

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:44:
PrerenderingLoader* loader);
On 2016/05/09 20:38:32, fgorski wrote:
> Since you are taking ownership of the loader, use unique_ptr to pass it as
well,
> that way it is clear that you are taking it over.
> 
> Also, seeing how the TestPrerenderingLoader is implemented, I think you free
up
> loader memory twice. Once when destroying PrerenderingOffliner, and once when
> completing the test (also held by the test in unique_ptr). I'll comment more
in
> the test.
> 
> There is an alternative to defining this constructor:
> A SetLoaderForTesting method, that accepts a unique_ptr.

PTAL - now have lazy creation of Loader with SetLoaderForTesting() method.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
(right):

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:18: class
MockPrerenderingLoader : public PrerenderingLoader {
On 2016/05/09 20:38:32, fgorski wrote:
> can you put this in the anonymous namespace too?

Done.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:40: class
TestPrerenderingOffliner : public PrerenderingOffliner {
On 2016/05/09 20:38:32, fgorski wrote:
> When you have SetLoaderForTesting I don't think this is needed.

Done.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:40: class
TestPrerenderingOffliner : public PrerenderingOffliner {
On 2016/05/09 20:38:32, fgorski wrote:
> ditto for namespace.

Done.

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:83:
std::unique_ptr<MockPrerenderingLoader> loader_;
On 2016/05/09 20:38:32, fgorski wrote:
> this frees the loader memory or the second time. Perhaps that is why you
crash.

reworked

https://codereview.chromium.org/1961083002/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:94:
loader_.reset(new MockPrerenderingLoader(nullptr));
On 2016/05/09 20:38:32, fgorski wrote:
> simply create a unique ptr and move it into the offliner_

Done.

[fgorski, 2016-05-10 03:52:43]

lgtm with comments.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:35: return
(GetOrCreateLoader()->LoadPage(
The outer () are not necessary, and they don't improve readability of this code.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:46: loader_ =
std::unique_ptr<PrerenderingLoader>(
I think a more concise form would be:

loader_.reset(new PrerenderingLoader(browser_context_));

What do you think?

And below in 54, I think assignment is the right choice.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:53:
std::unique_ptr<PrerenderingLoader> loader) {
If you want to prevent accidentally setting the loader twice in your tests you
can add:
DCHECK(!loader_);

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.h (right):

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:34: bool
LoadAndSave(const SavePageRequest& request,
it is common to put a header on a set of overrides.

// Offliner implementation.

And then don't put empty lines between methods.
This sends a clear message where to look for definitions and documentation of
the methods.
It is obviously more useful, when there are more classes you inherit from.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:39: void
SetLoaderForTesting(std::unique_ptr<PrerenderingLoader> loader);
add documentation to that one.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
(right):

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:55:
Offliner::CompletionCallback callback() {
this can be a const function

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:61:
Offliner::CompletionStatus completion_status() { return completion_status_; }
this can be a const function as well

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:69:
MockPrerenderingLoader* loader_;
I think this is OK. You should be able to work with this.

Do you plan to allow inspection of input parameters using mock loader?

[dougarnett, 2016-05-10 15:30:19]

Thanks Filip. Think I covered your comments.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.cc (right):

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:35: return
(GetOrCreateLoader()->LoadPage(
On 2016/05/10 03:52:43, fgorski wrote:
> The outer () are not necessary, and they don't improve readability of this
code.

Done.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.cc:53:
std::unique_ptr<PrerenderingLoader> loader) {
On 2016/05/10 03:52:43, fgorski wrote:
> If you want to prevent accidentally setting the loader twice in your tests you
> can add:
> DCHECK(!loader_);

Done.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner.h (right):

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:34: bool
LoadAndSave(const SavePageRequest& request,
On 2016/05/10 03:52:43, fgorski wrote:
> it is common to put a header on a set of overrides.
> 
> // Offliner implementation.
> 
> And then don't put empty lines between methods.
> This sends a clear message where to look for definitions and documentation of
> the methods.
> It is obviously more useful, when there are more classes you inherit from.

Done.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:39: void
SetLoaderForTesting(std::unique_ptr<PrerenderingLoader> loader);
On 2016/05/10 03:52:43, fgorski wrote:
> add documentation to that one.

Done.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
(right):

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:55:
Offliner::CompletionCallback callback() {
On 2016/05/10 03:52:43, fgorski wrote:
> this can be a const function

Done.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:61:
Offliner::CompletionStatus completion_status() { return completion_status_; }
On 2016/05/10 03:52:43, fgorski wrote:
> this can be a const function as well

Done.

https://codereview.chromium.org/1961083002/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:69:
MockPrerenderingLoader* loader_;
On 2016/05/10 03:52:43, fgorski wrote:
> I think this is OK. You should be able to work with this.
> 
> Do you plan to allow inspection of input parameters using mock loader?

I plan to access the LoadPageCallback but wasn't that concerned about verifying
the same url is passed through but would be easy enough to do.

[dougarnett, 2016-05-10 16:03:51]

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

[commit-bot: I haz the power, 2016-05-10 16:04:10]

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

[commit-bot: I haz the power, 2016-05-10 16:31:49]

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

[commit-bot: I haz the power, 2016-05-10 16:31:50]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[dougarnett, 2016-05-10 16:44:42]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-05-10 16:44:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1961083002/#ps100001
(title: "Removed the unnecessary const on enum accessor as it made a trybot
unhappy.")

[commit-bot: I haz the power, 2016-05-10 16:45:03]

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

[commit-bot: I haz the power, 2016-05-10 18:14:21]

Description was changed from

==========
Updates to PrerenderingOffliner and PrerenderingLoader interfaces to push
PrerenderManager knowledge down into Loader only. Also adds initial
unit tests for Offliner with mocked Loader. This CL to be followed by
a Loader CL that integrates to PrerenderManager. Fleshing out the Offliner
logic more will come after that.

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

to

==========
Updates to PrerenderingOffliner and PrerenderingLoader interfaces to push
PrerenderManager knowledge down into Loader only. Also adds initial
unit tests for Offliner with mocked Loader. This CL to be followed by
a Loader CL that integrates to PrerenderManager. Fleshing out the Offliner
logic more will come after that.

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

[commit-bot: I haz the power, 2016-05-10 18:14:22]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-10 18:15:33]

Description was changed from

==========
Updates to PrerenderingOffliner and PrerenderingLoader interfaces to push
PrerenderManager knowledge down into Loader only. Also adds initial
unit tests for Offliner with mocked Loader. This CL to be followed by
a Loader CL that integrates to PrerenderManager. Fleshing out the Offliner
logic more will come after that.

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

to

==========
Updates to PrerenderingOffliner and PrerenderingLoader interfaces to push
PrerenderManager knowledge down into Loader only. Also adds initial
unit tests for Offliner with mocked Loader. This CL to be followed by
a Loader CL that integrates to PrerenderManager. Fleshing out the Offliner
logic more will come after that.

BUG=601173

Committed: https://crrev.com/30fe6bc61105ba5c555c735166b362e4986485b5
Cr-Commit-Position: refs/heads/master@{#392646}
==========

[commit-bot: I haz the power, 2016-05-10 18:15:34]

Patchset 6 (id:??) landed as
https://crrev.com/30fe6bc61105ba5c555c735166b362e4986485b5
Cr-Commit-Position: refs/heads/master@{#392646}