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

chrome/browser/android/offline_pages/prerendering_offliner.cc

Index: chrome/browser/android/offline_pages/prerendering_offliner.cc
diff --git a/chrome/browser/android/offline_pages/prerendering_offliner.cc b/chrome/browser/android/offline_pages/prerendering_offliner.cc
index b654c75a6b5620ebe6bf03e7d525daa88a111cb0..42f12535dbb9a346d4f8f85c3d3e7f6ac7cb7d58 100644
--- a/chrome/browser/android/offline_pages/prerendering_offliner.cc
+++ b/chrome/browser/android/offline_pages/prerendering_offliner.cc
@@ -5,6 +5,7 @@
 #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"
 
@@ -16,28 +17,88 @@ PrerenderingOffliner::PrerenderingOffliner(
     OfflinePageModel* offline_page_model)
     : browser_context_(browser_context),
       offline_page_model_(offline_page_model),
+      pending_request_id_(0),
       weak_ptr_factory_(this) {}
 
 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_)
+    return;
+
+  if (load_status == Offliner::RequestStatus::LOADED) {
+    // The page has successfully loaded so now try to save the page.
+    // After issuing the save request we will wait for either: the save
+    // callback or a CANCELED load callback (triggered because the loaded
+    // WebContents are being destroyed) - whichever callback occurs first.
+    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,
+                        completion_callback));
+  } else {
+    // Clear pending request and then run its completion callback.
+    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;
+
+  ClearPendingRequest();
+  GetOrCreateLoader()->StopLoading();
+
+  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;
+  }
+  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.
+  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));
+  if (!accepted)

[fgorski, 2016/05/24 19:58:28]

I understand that callback will be called if LoadPage returned true.
Is it guaranteed to not call the callback if it returned false?

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

Yes, updated the interface method comment.

+    ClearPendingRequest();
+
+  return accepted;
 }
 
 void PrerenderingOffliner::Cancel() {
+  ClearPendingRequest();
   GetOrCreateLoader()->StopLoading();
+  // TODO(dougarnett): Consider ability to cancel SavePage request.
 }
 
 void PrerenderingOffliner::SetLoaderForTesting(
@@ -46,6 +107,20 @@ void PrerenderingOffliner::SetLoaderForTesting(
   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_));
@@ -53,4 +128,15 @@ PrerenderingLoader* PrerenderingOffliner::GetOrCreateLoader() {
   return loader_.get();
 }
 
+void PrerenderingOffliner::SetPendingRequest(int64_t request_id) {
+  DCHECK_GT(request_id, 0);
+  DCHECK_EQ(pending_request_id_, 0);

[fgorski, 2016/05/24 19:58:28]

we are using the magic number 0 all over this code.

Did you consider extracting it, e.g. kInvalidRequestId.

Also, I am wondering if only allowing positive request IDs is reasonable, just
valid (non-zero) could be a more sound approach. Please consider that.

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

Revised to keep pointer to pending request instead (leverage nullptr instead
special id value)

+  pending_request_id_ = request_id;
+}
+
+void PrerenderingOffliner::ClearPendingRequest() {
+  DCHECK_GT(pending_request_id_, 0);
+  pending_request_id_ = 0;
+}
+
 }  // namespace offline_pages