Improve the page download:

chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc

Index: chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
diff --git a/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc b/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
index c6227668269a875c5938e3f501c7820814f00979..c4c171606ab976d14ec6d777b1a76b189c66716a 100644
--- a/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
+++ b/chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc
@@ -66,70 +66,51 @@ content::WebContents* GetWebContentsFromJavaTab(
   return tab->web_contents();
 }
 
-void SavePageIfNavigatedToURL(const GURL& query_url,
-                              const ScopedJavaGlobalRef<jobject>& j_tab_ref) {
+void SavePageIfStillOnSamePage(const GURL& original_url,

[Pete Williamson, 2016/10/12 17:33:36]

I thought our bridges were supposed to be thin pass throughs - this seems like a
lot of logic in the bridge (to be fair, it was also a lot of logic before you
change).

[Dmitry Titov, 2016/10/12 23:11:14]

Indeed. Bridges ideally should be dumb translators between languages.
Unfortunately, this complexity was implemented before this patch. I think most
of this should actually be in RequestCoordinator, maybe once it becomes
PageCaptureJobController or something like that :) One of the reason this logic
goes in foo_bridge.cc is because there is no C++ side object that would
implement the 'capturing' action end-to-end. We have OfflinePageModel,
RequestCoordinator, TabHelper - and there should be something coordinating all
these things but we don't have it. Good future refactor.

+                               const ScopedJavaGlobalRef<jobject>& j_tab_ref) {
   content::WebContents* web_contents = GetWebContentsFromJavaTab(j_tab_ref);
   if (!web_contents)
     return;
 
-  // This doesn't detect navigations to the same URL, only that we are looking
-  // at a completely different page.
+  // This ignores fragment differences in URLs, bails out only if tab has
+  // navigated to a completely different page.
   GURL url = web_contents->GetLastCommittedURL();
-  if (!OfflinePageUtils::EqualsIgnoringFragment(url, query_url))
+  if (!OfflinePageUtils::EqualsIgnoringFragment(url, original_url))
     return;
 
   offline_pages::ClientId client_id;
   client_id.name_space = offline_pages::kDownloadNamespace;
   client_id.id = base::GenerateGUID();
+  int64_t request_id = 0;
 
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext())
-          ->GetOriginalProfile();
-
-  OfflinePageNotificationBridge notification_bridge;
-
-  // If the page is not loaded enough to be captured, submit a background loader
-  // request instead.
-  offline_pages::RecentTabHelper* tab_helper =
-      RecentTabHelper::FromWebContents(web_contents);
-  if (tab_helper && !tab_helper->is_page_ready_for_snapshot() &&
-      offline_pages::IsBackgroundLoaderForDownloadsEnabled()) {
-    // TODO(dimich): Improve this to wait for the page load if it is still going
-    // on. Pre-submit the request and if the load finishes and capture happens,
-    // remove request.
+  if (offline_pages::IsBackgroundLoaderForDownloadsEnabled()) {
+    // Post disabled request before passing the download task to the tab helper.
+    // This will keep the request persisted in case Chrome is evicted from RAM
+    // or closed by the user.
     offline_pages::RequestCoordinator* request_coordinator =
-        offline_pages::RequestCoordinatorFactory::GetForBrowserContext(profile);
-    request_coordinator->SavePageLater(
+        offline_pages::RequestCoordinatorFactory::GetForBrowserContext(
+            web_contents->GetBrowserContext());
+    request_id = request_coordinator->SavePageLater(
         url, client_id, true,
-        RequestCoordinator::RequestAvailability::ENABLED_FOR_OFFLINER);
-
-    notification_bridge.ShowDownloadingToast();
-    return;
+        RequestCoordinator::RequestAvailability::DISABLED_FOR_OFFLINER);
   }
 
-  // Page is ready, capture it right from the tab.
-  offline_pages::OfflinePageModel* offline_page_model =
-      OfflinePageModelFactory::GetForBrowserContext(profile);
-  if (!offline_page_model)
+  // Pass request_id to the current tab's helper to attempt download right from
+  // the tab. If unsuccessful, it'll enable the already-queued request for
+  // background offliner. Same will happen if Chrome is terminated since
+  // 'disabled' status of the request is RAM-stored info.
+  offline_pages::RecentTabHelper* tab_helper =
+      RecentTabHelper::FromWebContents(web_contents);
+  if (!tab_helper)
     return;
+  tab_helper->ObserveAndDownloadCurrentPage(client_id, request_id);
 
-  auto archiver =
-      base::MakeUnique<offline_pages::OfflinePageMHTMLArchiver>(web_contents);
-
-  DownloadUIItem item;
-  item.guid = client_id.id;
-  item.url = url;
-
-  OfflinePageNotificationBridge bridge;
-  bridge.NotifyDownloadProgress(item);
-
+  OfflinePageNotificationBridge notification_bridge;
   notification_bridge.ShowDownloadingToast();
-  offline_page_model->SavePage(url, client_id, 0l, std::move(archiver),
-                               base::Bind(&SavePageCallback, item));
 }
 
 void RequestQueueDuplicateCheckDone(
-    const GURL& query_url,
+    const GURL& original_url,
     const ScopedJavaGlobalRef<jobject>& j_tab_ref,
     bool has_duplicates) {
   if (has_duplicates) {
@@ -143,10 +124,10 @@ void RequestQueueDuplicateCheckDone(
     return;
   }
 
-  SavePageIfNavigatedToURL(query_url, j_tab_ref);
+  SavePageIfStillOnSamePage(original_url, j_tab_ref);
 }
 
-void ModelDuplicateCheckDone(const GURL& query_url,
+void ModelDuplicateCheckDone(const GURL& original_url,
                              const ScopedJavaGlobalRef<jobject>& j_tab_ref,
                              bool has_duplicates) {
   content::WebContents* web_contents = GetWebContentsFromJavaTab(j_tab_ref);
@@ -155,16 +136,16 @@ void ModelDuplicateCheckDone(const GURL& query_url,
 
   if (has_duplicates) {
     OfflinePageInfoBarDelegate::Create(
-        base::Bind(&SavePageIfNavigatedToURL, query_url, j_tab_ref),
-        query_url.spec(), web_contents);
+        base::Bind(&SavePageIfStillOnSamePage, original_url, j_tab_ref),
+        original_url.spec(), web_contents);
     return;
   }
 
   OfflinePageUtils::CheckExistenceOfRequestsWithURL(
       Profile::FromBrowserContext(web_contents->GetBrowserContext())
           ->GetOriginalProfile(),
-      kDownloadNamespace, query_url,
-      base::Bind(&RequestQueueDuplicateCheckDone, query_url, j_tab_ref));
+      kDownloadNamespace, original_url,
+      base::Bind(&RequestQueueDuplicateCheckDone, original_url, j_tab_ref));
 }
 
 void ToJavaOfflinePageDownloadItemList(
@@ -331,7 +312,6 @@ void OfflinePageDownloadBridge::StartDownload(
     return;
 
   GURL url = web_contents->GetLastCommittedURL();
-
   ScopedJavaGlobalRef<jobject> j_tab_ref(env, j_tab);
 
   OfflinePageUtils::CheckExistenceOfPagesWithURL(