[Offline pages]: Implement background loader to save on last retry, and record last retry success U…

chrome/browser/android/offline_pages/background_loader_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/background_loader_offliner.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offliner_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "components/offline_pages/core/background/offliner_policy.h"
 #include "components/offline_pages/core/background/save_page_request.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_user_data.h"
 
 namespace offline_pages {
@@ skipping 42 matching lines @@
       std::abs(error_code));
 }
 }  // namespace
 
 BackgroundLoaderOffliner::BackgroundLoaderOffliner(
     content::BrowserContext* browser_context,
     const OfflinerPolicy* policy,
     OfflinePageModel* offline_page_model)
     : browser_context_(browser_context),
       offline_page_model_(offline_page_model),
+      policy_(policy),
       is_low_end_device_(base::SysInfo::IsLowEndDevice()),
       save_state_(NONE),
       page_load_state_(SUCCESS),
       page_delay_ms_(kOfflinePageDelayMs),
       network_bytes_(0LL),
       weak_ptr_factory_(this) {
   DCHECK(offline_page_model_);
   DCHECK(browser_context_);
 }
 
@@ skipping 105 matching lines @@
     cancel_callback_ = callback;
     return;
   }
 
   int64_t request_id = pending_request_->request_id();
   ResetState();
   callback.Run(request_id);
 }
 
 bool BackgroundLoaderOffliner::HandleTimeout(const SavePageRequest& request) {
-  // TODO(romax): Decide if we want to also take a snapshot on the last timeout
-  // for the background loader offliner. crbug.com/705090
+  if (pending_request_) {
+    DCHECK(request.request_id() == pending_request_->request_id());
+    if (is_low_bar_met_ &&
+        (request.started_attempt_count() + 1 > policy_->GetMaxStartedTries() ||

[fgorski, 2017/04/14 05:15:57]

I am wondering whether we can make all these checks outside of the offliner and
only pass information whether this is_last_retry_ or not. 

That way you have only 2 tests to write. I think we are spilling policy too much
that way.

I looked at HandleTimeout of PrerenderingOffliner and of course there is a minor
difference there, but the code is duplicated otherwise... telling me it belongs
outside of that call. Also, policy_ is used only in this method in case of the
other offliner...

bool PrerenderingOffliner::HandleTimeout(const SavePageRequest& request) {
  if (pending_request_) {
    DCHECK(request.request_id() == pending_request_->request_id());
    if (GetOrCreateLoader()->IsLowbarMet() &&
        (request.started_attempt_count() + 1 >= policy_->GetMaxStartedTries() ||
         request.completed_attempt_count() + 1 >=
             policy_->GetMaxCompletedTries())) {
      GetOrCreateLoader()->StartSnapshot();
      return true;
    }
  }
  return false;
}

[chili, 2017/04/15 00:34:58]

I agree with this, but can we make the change in a separate CL? I filed
crbug/711842 to track.

The move will require updates in a number of unittests, and it might be better
to do that separately.

[fgorski, 2017/04/17 17:48:59]

Acknowledged.

+         request.completed_attempt_count() + 1 >=
+             policy_->GetMaxCompletedTries())) {
+      did_snapshot_on_last_retry_ = true;
+      StartSnapshot();
+      return true;
+    }
+  }
   return false;
 }
 
 void BackgroundLoaderOffliner::DocumentAvailableInMainFrame() {
   snapshot_controller_->DocumentAvailableInMainFrame();
+  is_low_bar_met_ = true;
 }
 
 void BackgroundLoaderOffliner::DocumentOnLoadCompletedInMainFrame() {
   if (!pending_request_.get()) {
     DVLOG(1) << "DidStopLoading called even though no pending request.";
     return;
   }
 
   snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
 }
@@ skipping 111 matching lines @@
       base::Bind(&BackgroundLoaderOffliner::OnPageSaved,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void BackgroundLoaderOffliner::OnPageSaved(SavePageResult save_result,
                                            int64_t offline_id) {
   if (!pending_request_)
     return;
 
   SavePageRequest request(*pending_request_.get());
+  bool did_snapshot_on_last_retry = did_snapshot_on_last_retry_;
   ResetState();
 
   if (save_state_ == DELETE_AFTER_SAVE) {
     save_state_ = NONE;
     cancel_callback_.Run(request.request_id());
     return;
   }
 
   save_state_ = NONE;
 
   Offliner::RequestStatus save_status;
-  if (save_result == SavePageResult::SUCCESS)
-    save_status = RequestStatus::SAVED;
-  else
+  if (save_result == SavePageResult::SUCCESS) {
+    if (did_snapshot_on_last_retry)
+      save_status = RequestStatus::SAVED_ON_LAST_RETRY;
+    else
+      save_status = RequestStatus::SAVED;
+  } else {
     save_status = RequestStatus::SAVE_FAILED;
+  }
 
   completion_callback_.Run(request, save_status);
 }
 
 void BackgroundLoaderOffliner::ResetState() {
   pending_request_.reset();
   snapshot_controller_.reset();
   page_load_state_ = SUCCESS;
   network_bytes_ = 0LL;
+  is_low_bar_met_ = false;
+  did_snapshot_on_last_retry_ = false;
   // TODO(chili): Remove after RequestCoordinator can handle multiple offliners.
   // We reset the loader and observer after completion so loaders
   // will not be re-used across different requests/tries. This is a temporary
   // solution while there exists assumptions about the number of offliners
   // there are.
   loader_.reset(
       new background_loader::BackgroundLoaderContents(browser_context_));
   content::WebContents* contents = loader_->web_contents();
   content::WebContentsObserver::Observe(contents);
   OfflinerData::AddToWebContents(contents, this);
@@ skipping 19 matching lines @@
     int64_t offline_id) {
   // If for some reason the request was reset during while waiting for callback
   // ignore the completion callback.
   if (pending_request_ && pending_request_->request_id() != offline_id)
     return;
   completion_callback_.Run(request, RequestStatus::FOREGROUND_CANCELED);
 }
 }  // namespace offline_pages
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinerData);