Fixes for turning on offliner request.

Fixes for turning on offliner request.

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

[dougarnett, 2016-06-08 02:17:21]

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

[Pete Williamson, 2016-06-08 17:32:16]

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

https://codereview.chromium.org/2048923002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter.cc:81: delete
handle_to_destroy;
I didn't see any definitions of handle_to_destroy or OnPrerenderStopLoading in
this changelist.  I didn't find handle_to_destroy with code search either.  Is a
file missing from this changelist?

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

https://codereview.chromium.org/2048923002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_loader.h:49: virtual bool
LoadPage(const GURL& url, LoadPageCallback callback);
Generally speaking, we like to pass classes by pointer or reference unless they
are small (std::String is the typical example of a small class, with only one
pointer member).  I can understand if your code cannot hold onto a reference,
but would a pointer work?  If so, be sure to use unique_ptr if transferring
ownership, or a bald pointer if ownership is managed elsewhere (and document
that it is unowned).

[dougarnett, 2016-06-08 20:02:31]

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

https://codereview.chromium.org/2048923002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter.cc:81: delete
handle_to_destroy;
On 2016/06/08 17:32:16, Pete Williamson wrote:
> I didn't see any definitions of handle_to_destroy or OnPrerenderStopLoading in
> this changelist.  I didn't find handle_to_destroy with code search either.  Is
a
> file missing from this changelist?

? handle_to_destroy is defined 2 lines above and the commented OnPrerenderStop
method is 4 methods below. This fixes and issue where the Prerender stack is
calling OnPrerenderStop() directly in call stack from the OnCancel() call here
and before we had reset active_handle_

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

https://codereview.chromium.org/2048923002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerendering_loader.h:49: virtual bool
LoadPage(const GURL& url, LoadPageCallback callback);
On 2016/06/08 17:32:16, Pete Williamson wrote:
> Generally speaking, we like to pass classes by pointer or reference unless
they
> are small (std::String is the typical example of a small class, with only one
> pointer member).  I can understand if your code cannot hold onto a reference,
> but would a pointer work?  If so, be sure to use unique_ptr if transferring
> ownership, or a bald pointer if ownership is managed elsewhere (and document
> that it is unowned).

Done - holding copy of callback, fixed param signature

[Pete Williamson, 2016-06-08 20:09:40]

lgtm

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

https://codereview.chromium.org/2048923002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/prerender_adapter.cc:81: delete
handle_to_destroy;
On 2016/06/08 20:02:31, dougarnett wrote:
> On 2016/06/08 17:32:16, Pete Williamson wrote:
> > I didn't see any definitions of handle_to_destroy or OnPrerenderStopLoading
in
> > this changelist.  I didn't find handle_to_destroy with code search either. 
Is
> a
> > file missing from this changelist?
> 
> ? handle_to_destroy is defined 2 lines above and the commented OnPrerenderStop
> method is 4 methods below. This fixes and issue where the Prerender stack is
> calling OnPrerenderStop() directly in call stack from the OnCancel() call here
> and before we had reset active_handle_ 

Acknowledged.  My bad, I saw the assignment on line 79, and missed the fact that
the assignment was also a declaration somehow.

[dougarnett, 2016-06-08 20:18:43]

The CQ bit was checked by dougarnett@chromium.org

[commit-bot: I haz the power, 2016-06-08 20:19:48]

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

[commit-bot: I haz the power, 2016-06-08 21:00:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-08 21:03:02]

Description was changed from

==========
Fixes for turning on offliner request.

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

to

==========
Fixes for turning on offliner request.

BUG=601173

Committed: https://crrev.com/856dc4052804930a76e22cf18463bb68640aa146
Cr-Commit-Position: refs/heads/master@{#398669}
==========

[commit-bot: I haz the power, 2016-06-08 21:03:03]

Patchset 2 (id:??) landed as
https://crrev.com/856dc4052804930a76e22cf18463bb68640aa146
Cr-Commit-Position: refs/heads/master@{#398669}