Adds shells of classes (PrerenderingOffliner and PrerenderingLoader) to intergrate from Background …

Adds shells of classes (PrerenderingOffliner and PrerenderingLoader) to intergrate from Background Offliner core to Prerenderer for background loading.

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

[dougarnett, 2016-04-27 18:05:51]

Description was changed from

==========
Adds shells of classes (PrerenderingOffliner and PrerenderingLoader) to
intergrate from Background Offliner core to Prerenderer for background loading.

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

to

==========
Adds shells of classes (PrerenderingOffliner and PrerenderingLoader) to
intergrate from Background Offliner core to Prerenderer for background loading.

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

[dougarnett, 2016-04-27 18:05:51]

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

[dougarnett, 2016-04-27 23:18:37]

[oops, thought I sent this earlier]

[Pete Williamson, 2016-04-27 23:58:28]

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

https://codereview.chromium.org/1915983008/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:23: class
PrerenderingOffliner : public Offliner {
How is this different from the PrerenderingLoader?  I'd like the class comments
to explain that a bit better (either here or in the other class or both)

https://codereview.chromium.org/1915983008/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1915983008/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1559:
'browser/android/offline_pages/recent_tab_helper.cc',
You also need to modify the associated BUILD.gn file.

[dougarnett, 2016-04-28 00:18:03]

Btw, an uncommented motivation for the PrerendingLoader is both a test point
(that need not involve the saving page part).

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

https://codereview.chromium.org/1915983008/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:23: class
PrerenderingOffliner : public Offliner {
On 2016/04/27 23:58:28, Pete Williamson wrote:
> How is this different from the PrerenderingLoader?  I'd like the class
comments
> to explain that a bit better (either here or in the other class or both)

Please see if updated comments are better and good enuf.

https://codereview.chromium.org/1915983008/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1915983008/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1559:
'browser/android/offline_pages/recent_tab_helper.cc',
On 2016/04/27 23:58:28, Pete Williamson wrote:
> You also need to modify the associated BUILD.gn file.

Do we need to add all our files to it then in new target then? 
The local BUILD.gn only has a proto defn currently.

[Pete Williamson, 2016-04-28 00:28:45]

Mostly looks good.  I'd like to see BUILD.gn before I sign off, though.

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

https://codereview.chromium.org/1915983008/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_offliner.h:23: class
PrerenderingOffliner : public Offliner {
On 2016/04/28 00:18:03, dougarnett wrote:
> On 2016/04/27 23:58:28, Pete Williamson wrote:
> > How is this different from the PrerenderingLoader?  I'd like the class
> comments
> > to explain that a bit better (either here or in the other class or both)
> 
> Please see if updated comments are better and good enuf.

Acknowledged.

https://codereview.chromium.org/1915983008/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1915983008/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1559:
'browser/android/offline_pages/recent_tab_helper.cc',
On 2016/04/28 00:18:03, dougarnett wrote:
> On 2016/04/27 23:58:28, Pete Williamson wrote:
> > You also need to modify the associated BUILD.gn file.
> 
> Do we need to add all our files to it then in new target then? 
> The local BUILD.gn only has a proto defn currently.

Yes, we may need to add a new target.  If we do add a new target, we'll need to
modify a higher level BUILD.gn file to include our target.  See my latest issue
1914333002 for an example of what to change in a higher level build file.

[fgorski, 2016-04-28 11:32:21]

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

https://codereview.chromium.org/1915983008/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.h:38: const GURL url,
const ref

https://codereview.chromium.org/1915983008/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.h:43: void StopLoad();
nit: StopLoading? Also Cancel is perfect here.

https://codereview.chromium.org/1915983008/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.h:47: Profile*
profile_;
Not sure if that is doable, but see if you can work with BrowserContext instead.

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

https://codereview.chromium.org/1915983008/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:25: //
TODO(dougarnett): consider not locking to profile (pass in request).
If you are passing profile/browser context, you don't technically need to pass
OPM, as it is a keyed service.
If you can get away with not passing the profile, and pass the right specific
objects that would be best.

[dougarnett, 2016-04-28 16:02:08]

On 2016/04/28 00:28:45, Pete Williamson wrote:
> Mostly looks good.  I'd like to see BUILD.gn before I sign off, though.
> 
>
https://codereview.chromium.org/1915983008/diff/1/chrome/browser/android/offl...
> File chrome/browser/android/offline_pages/prerendering_offliner.h (right):
> 
>
https://codereview.chromium.org/1915983008/diff/1/chrome/browser/android/offl...
> chrome/browser/android/offline_pages/prerendering_offliner.h:23: class
> PrerenderingOffliner : public Offliner {
> On 2016/04/28 00:18:03, dougarnett wrote:
> > On 2016/04/27 23:58:28, Pete Williamson wrote:
> > > How is this different from the PrerenderingLoader?  I'd like the class
> > comments
> > > to explain that a bit better (either here or in the other class or both)
> > 
> > Please see if updated comments are better and good enuf.
> 
> Acknowledged.
> 
> https://codereview.chromium.org/1915983008/diff/1/chrome/chrome_browser.gypi
> File chrome/chrome_browser.gypi (right):
> 
>
https://codereview.chromium.org/1915983008/diff/1/chrome/chrome_browser.gypi#...
> chrome/chrome_browser.gypi:1559:
> 'browser/android/offline_pages/recent_tab_helper.cc',
> On 2016/04/28 00:18:03, dougarnett wrote:
> > On 2016/04/27 23:58:28, Pete Williamson wrote:
> > > You also need to modify the associated BUILD.gn file.
> > 
> > Do we need to add all our files to it then in new target then? 
> > The local BUILD.gn only has a proto defn currently.
> 
> Yes, we may need to add a new target.  If we do add a new target, we'll need
to
> modify a higher level BUILD.gn file to include our target.  See my latest
issue
> 1914333002 for an example of what to change in a higher level build file.

I'm not clear about that. Looks like the chrome/browser/BUILD.gn pulls in the
chrome_browser_offline_page_sources from the chrome/chrome_browser.gypi file
conditionally if android_ui. So this looks different from the components setup.
If we want to change that, then I wonder about landing another CL to do that
for the existing files first (then add the new files to it in this CL).

[dougarnett, 2016-04-28 20:13:13]

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

https://codereview.chromium.org/1915983008/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.h:38: const GURL url,
On 2016/04/28 11:32:20, fgorski wrote:
> const ref

Done.

https://codereview.chromium.org/1915983008/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.h:43: void StopLoad();
On 2016/04/28 11:32:20, fgorski wrote:
> nit: StopLoading? Also Cancel is perfect here.

Done.

https://codereview.chromium.org/1915983008/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.h:47: Profile*
profile_;
On 2016/04/28 11:32:20, fgorski wrote:
> Not sure if that is doable, but see if you can work with BrowserContext
instead.

updated to pass in PrerenderManager instead

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

https://codereview.chromium.org/1915983008/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:25: //
TODO(dougarnett): consider not locking to profile (pass in request).
On 2016/04/28 11:32:20, fgorski wrote:
> If you are passing profile/browser context, you don't technically need to pass
> OPM, as it is a keyed service.
> If you can get away with not passing the profile, and pass the right specific
> objects that would be best.

The right specific object I need would seem to be the PrerenderManager which is
a keyed service tied to BrowserContext/Profile.

[Pete Williamson, 2016-04-28 20:37:15]

lgtm

[fgorski, 2016-04-29 05:31:45]

lgtm

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

https://codereview.chromium.org/1915983008/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:25:
PrerenderingOffliner(
Do you run git cl format? Not sure but maybe this fits in 2 lines.

[dougarnett, 2016-04-29 16:27:23]

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

https://codereview.chromium.org/1915983008/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_offliner.h:25:
PrerenderingOffliner(
On 2016/04/29 05:31:45, fgorski wrote:
> Do you run git cl format? Not sure but maybe this fits in 2 lines.

Done.

[dougarnett, 2016-04-29 17:10:39]

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

[commit-bot: I haz the power, 2016-04-29 17:10:55]

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

[dougarnett, 2016-04-29 18:20:29]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-04-29 18:20:29]

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/1915983008/#ps100001
(title: "Fixes for unused and override checks by trybots")

[commit-bot: I haz the power, 2016-04-29 18:21:22]

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

[commit-bot: I haz the power, 2016-04-29 18:26:38]

Description was changed from

==========
Adds shells of classes (PrerenderingOffliner and PrerenderingLoader) to
intergrate from Background Offliner core to Prerenderer for background loading.

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

to

==========
Adds shells of classes (PrerenderingOffliner and PrerenderingLoader) to
intergrate from Background Offliner core to Prerenderer for background loading.

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

[commit-bot: I haz the power, 2016-04-29 18:26:39]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:27:32]

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

[commit-bot: I haz the power, 2016-04-30 18:01:33]

Description was changed from

==========
Adds shells of classes (PrerenderingOffliner and PrerenderingLoader) to
intergrate from Background Offliner core to Prerenderer for background loading.

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

to

==========
Adds shells of classes (PrerenderingOffliner and PrerenderingLoader) to
intergrate from Background Offliner core to Prerenderer for background loading.

BUG=601173

Committed: https://crrev.com/70ad75b5c1f8208569b642c62bc9e4ce90cb06e5
Cr-Commit-Position: refs/heads/master@{#390704}
==========