Adds PrerenderingOffliner implementation for handling Loader callbacks and then performing SavePage…

chrome/browser/android/offline_pages/prerendering_offliner.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/offline_pages/prerendering_offliner.h"
 
 #include "base/bind.h"
+#include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "content/public/browser/browser_context.h"
 
 namespace offline_pages {
 
 PrerenderingOffliner::PrerenderingOffliner(
     content::BrowserContext* browser_context,
     const OfflinerPolicy* policy,
     OfflinePageModel* offline_page_model)
     : browser_context_(browser_context),
       offline_page_model_(offline_page_model),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this),
+      pending_request_id_(0) {}
 
 PrerenderingOffliner::~PrerenderingOffliner() {}
 
-void PrerenderingOffliner::OnLoadPageDone(Offliner::RequestStatus load_status,
-                                          content::WebContents* contents) {
-  // TODO(dougarnett): Implement save attempt and running CompletionCallback.
+void PrerenderingOffliner::OnLoadPageDone(
+    const SavePageRequest& request,
+    const CompletionCallback& completion_callback,
+    Offliner::RequestStatus load_status,
+    content::WebContents* web_contents) {
+  // Check if request is still pending receiving a callback.
+  // Note: it is possible to get a loaded page, start the save operation,
+  // and then get another callback from the Loader (eg, if its loaded
+  // WebContents is being destroyed for some resource reclamation).
+  if (request.request_id() != pending_request_id_)

[Pete Williamson, 2016/05/24 00:40:07]

So if this happens, it looks like the request is ignored.  Isn't that a problem?

[dougarnett, 2016/05/24 19:07:26]

The callback is ignored because the request has already had some save
resolution. 
The scenario (race condition) is that the prerenderer stops rendering (its
WebContents get destroyed) and triggers this callback through the Loader but in
the meantime we have gotten a save callback (I see now that I am missing the
StopLoading() call and adding it).

[Pete Williamson, 2016/05/24 21:18:41]

OK, per our discussion, maybe add more DCHECKs to document.

[dougarnett, 2016/05/25 18:04:19]

Reworked to keep ptr to pending request - hopefully easier to follow.

+    return;
+
+  if (load_status == Offliner::RequestStatus::LOADED) {
+    // Page is loaded so now attempt to save it. We next are waiting for
+    // callback to this save request or may get cancel callback from the
+    // Loader (if this loaded web_contents is being reclaimed).

[Pete Williamson, 2016/05/24 00:40:07]

This sentence is a bit strangely worded, please clarify it some.

[dougarnett, 2016/05/24 19:07:26]

I agree - please check new wording

[Pete Williamson, 2016/05/24 21:18:42]

New wording is OK.

[dougarnett, 2016/05/25 18:04:19]

Acknowledged.

+    DCHECK(web_contents);
+    std::unique_ptr<OfflinePageArchiver> archiver(
+        new OfflinePageMHTMLArchiver(web_contents));
+    SavePage(request.url(), request.client_id(), std::move(archiver),
+             base::Bind(&PrerenderingOffliner::OnSavePageDone,
+                        weak_ptr_factory_.GetWeakPtr(), request,

[Pete Williamson, 2016/05/24 00:40:07]

I normally just have the class inherit publicly from
base::SupportsWeakPtr<RequestCoordinator>, then I use AsWeakPtr() where you used
weak_ptr_factory_.GetWeakPtr().  It makes the code easier to read at the point
of the bind, and you can get rid of the weak_ptr_factory_.  You will need to
#include "base/memory/weak_ptr.h".

[dewittj, 2016/05/24 17:56:09]

It's generally considered bad form to use SupportsWeakPtr, since the destruction
order is important and prone to bugs.  SupportsWeakPtr is generally fine in
tests where you don't care so much about test lifetimes, but in production code
a WeakPtrFactory is more appropriate.

[dougarnett, 2016/05/24 19:07:26]

Are we transitioning to that pattern? The GetWeakPtr() pattern seems to be the
most common in the existing offline_pages code base - eg, besides unittests,
only RequestCoordinator using AsWeakPtr at this point.

I'm happy to follow whichever pattern is preferred. 

[Pete Williamson, 2016/05/24 21:18:41]

OK, then we should do as Justin suggests.

[dougarnett, 2016/05/25 18:04:19]

Acknowledged.

+                        completion_callback));
+  } else {
+    // Clear pending request and then run its completion callback.

[Pete Williamson, 2016/05/24 00:40:07]

So does this mean we only get one callback for success?  If we are using a
RightMomentDetector kind of strategy, I could see us getting several, maybe one
"ok 5 seconds have gone by with no onPageLoaded event, and one "ok the page is
loaded", and one, "Ok, the page loaded and 2 seconds have passed."

[dougarnett, 2016/05/24 19:07:26]

Yes, only supporting one save attempt initially. I favor gathering empirical
evidence/motivation before supporting multiple ones. 

[Pete Williamson, 2016/05/24 21:18:42]

Ack.

+    ClearPendingRequest();
+    completion_callback.Run(request, load_status);
+  }
+}
+
+void PrerenderingOffliner::OnSavePageDone(
+    const SavePageRequest& request,
+    const CompletionCallback& completion_callback,
+    SavePageResult save_result,
+    int64_t offline_id) {
+  // Check if request is still pending receiving a callback.
+  if (request.request_id() != pending_request_id_)
+    return;
+
+  Offliner::RequestStatus save_status;
+  if (save_result == SavePageResult::SUCCESS) {
+    save_status = RequestStatus::SAVED;
+  } else {
+    // TODO(dougarnett): Consider reflecting some recommendation to retry the
+    // request based on specific save error cases.
+    save_status = RequestStatus::FAILED_SAVE;
+  }
+  ClearPendingRequest();
+  completion_callback.Run(request, save_status);
 }
 
 bool PrerenderingOffliner::LoadAndSave(const SavePageRequest& request,
                                        const CompletionCallback& callback) {
-  if (!offline_page_model_->CanSavePage(request.url()))
+  if (!CanSavePage(request.url()))
     return false;
 
+  // Track pending request for callback handling. This also ensures
+  // there is no current pending request.

[Pete Williamson, 2016/05/24 00:40:07]

But it only makes sure of that while debugging.  Do we need a stronger guarantee
at runtime?

[dougarnett, 2016/05/24 19:07:26]

Backing off the comment for now. Debug time might be sufficient since we own the
caller (Coordinator). Maybe we can discuss this afternoon.

[Pete Williamson, 2016/05/24 21:18:41]

ok

[dougarnett, 2016/05/25 18:04:19]

Revised to return false if already have pending request.

+  SetPendingRequest(request.request_id());
+
   // Kick off load page attempt.
-  return GetOrCreateLoader()->LoadPage(
-      request.url(), base::Bind(&PrerenderingOffliner::OnLoadPageDone,
-                                weak_ptr_factory_.GetWeakPtr()));
+  bool accepted = GetOrCreateLoader()->LoadPage(
+      request.url(),
+      base::Bind(&PrerenderingOffliner::OnLoadPageDone,
+                 weak_ptr_factory_.GetWeakPtr(), request, callback));

[Pete Williamson, 2016/05/24 00:40:07]

You can use AsWeakPtr() here too.

[Pete Williamson, 2016/05/24 21:18:42]

Per Justin's comment above, you can ignore this.

+  if (!accepted)
+    ClearPendingRequest();
+
+  return accepted;
 }
 
 void PrerenderingOffliner::Cancel() {
+  ClearPendingRequest();
   GetOrCreateLoader()->StopLoading();
+  // TODO(dougarnett): Consider ability to cancel SavePage request.
 }
 
 void PrerenderingOffliner::SetLoaderForTesting(
     std::unique_ptr<PrerenderingLoader> loader) {
   DCHECK(!loader_);
   loader_ = std::move(loader);
 }
 
+bool PrerenderingOffliner::CanSavePage(const GURL& url) {
+  DCHECK(offline_page_model_);
+  return offline_page_model_->CanSavePage(url);
+}
+
+void PrerenderingOffliner::SavePage(
+    const GURL& url,
+    const ClientId& client_id,
+    std::unique_ptr<OfflinePageArchiver> archiver,
+    const SavePageCallback& callback) {
+  DCHECK(offline_page_model_);
+  offline_page_model_->SavePage(url, client_id, std::move(archiver), callback);
+}
+
 PrerenderingLoader* PrerenderingOffliner::GetOrCreateLoader() {
   if (!loader_) {
     loader_.reset(new PrerenderingLoader(browser_context_));
   }
   return loader_.get();
 }
 
+void PrerenderingOffliner::SetPendingRequest(int64_t request_id) {
+  DCHECK_GT(request_id, 0);
+  DCHECK_EQ(pending_request_id_, 0);
+  pending_request_id_ = request_id;
+}
+
+void PrerenderingOffliner::ClearPendingRequest() {
+  DCHECK_GT(pending_request_id_, 0);
+  pending_request_id_ = 0;
+}
+
 }  // namespace offline_pages