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 fd9e2c436345dc7205668bdcbff0f86d90a2f07b..47717aec2450ee4bc92d0abe9b1876040987d088 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
@@ -81,74 +81,56 @@ OfflinePageModel* GetOfflinePageModelFromJavaTab(
   return OfflinePageModelFactory::GetForBrowserContext(profile);
 }
 
-void SavePageIfNavigatedToURL(const GURL& query_url,
-                              const ScopedJavaGlobalRef<jobject>& j_tab_ref) {
+void SavePageIfStillOnSamePage(const GURL& original_url,

[dewittj, 2016/10/12 03:27:27]

Page is not really right here, that's why I referred to URL previously.  So I
still prefer the "url" in this name and in the comments because it's closer to
the truth.

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

"SamePage" is more established than "SameURL". For example, there is
NavigationHandle::IsSamePage() that we use in TabHelper. Also, considering the
fragment can change URL but not page and this is what we actually check here, it
is more precise to say "same page" rather than "same URL"...

Attempted to find a better name. LMKWDYT.

[dewittj, 2016/10/13 17:25:37]

Acknowledged.

+                               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.

[dewittj, 2016/10/12 03:27:27]

My comment here is not internally consistent, my preference is to s/page/URL/
here too :)

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

Same as above.

+  // 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;

[dewittj, 2016/10/12 03:27:27]

this default is suspicious.  Needs at least a comment.  If it has to be zero,
perhaps making it a named constant in the request coordinator or tab helper
would be nice for self-documentation.

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

Done.

 
-  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);

[dewittj, 2016/10/12 03:27:27]

The comment above doesn't mention that the request is persisted but the disabled
status is not persisted, might be useful context here.

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

Done.

   }
 
-  // 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)

[dewittj, 2016/10/12 03:27:27]

If no tab helper then perhaps this code should re-enable the save page later
request.

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

Done.

     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;
-
-  notification_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 OnDeletePagesForInfoBar(const GURL& query_url,
+void OnDeletePagesForInfoBar(const GURL& original_url,
                              const ScopedJavaGlobalRef<jobject>& j_tab_ref,
                              DeletePageResult result) {
-  SavePageIfNavigatedToURL(query_url, j_tab_ref);
+  SavePageIfStillOnSamePage(original_url, j_tab_ref);
 }
 
-void DeletePagesForOverwrite(const GURL& query_url,
+void DeletePagesForOverwrite(const GURL& original_url,
                              const ScopedJavaGlobalRef<jobject>& j_tab_ref,
                              const MultipleOfflinePageItemResult& pages) {
   OfflinePageModel* model = GetOfflinePageModelFromJavaTab(j_tab_ref);
@@ -164,15 +146,16 @@ void DeletePagesForOverwrite(const GURL& query_url,
   }
 
   model->DeletePagesByOfflineId(
-      offline_ids, base::Bind(&OnDeletePagesForInfoBar, query_url, j_tab_ref));
+      offline_ids, base::Bind(
+          &OnDeletePagesForInfoBar, original_url, j_tab_ref));
 }
 
-void OnInfoBarAction(const GURL& query_url,
+void OnInfoBarAction(const GURL& original_url,
                      const ScopedJavaGlobalRef<jobject>& j_tab_ref,
                      OfflinePageInfoBarDelegate::Action action) {
   switch (action) {
     case OfflinePageInfoBarDelegate::Action::CREATE_NEW:
-      SavePageIfNavigatedToURL(query_url, j_tab_ref);
+      SavePageIfStillOnSamePage(original_url, j_tab_ref);
       break;
     case OfflinePageInfoBarDelegate::Action::OVERWRITE:
       OfflinePageModel* offline_page_model =
@@ -181,14 +164,14 @@ void OnInfoBarAction(const GURL& query_url,
         return;
 
       offline_page_model->GetPagesByOnlineURL(
-          query_url,
-          base::Bind(&DeletePagesForOverwrite, query_url, j_tab_ref));
+          original_url,
+          base::Bind(&DeletePagesForOverwrite, original_url, j_tab_ref));
       break;
   }
 }
 
 void RequestQueueDuplicateCheckDone(
-    const GURL& query_url,
+    const GURL& original_url,
     const ScopedJavaGlobalRef<jobject>& j_tab_ref,
     bool has_duplicates) {
   if (has_duplicates) {
@@ -202,10 +185,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,
                              const std::string& downloads_label,
                              bool has_duplicates) {
@@ -215,16 +198,16 @@ void ModelDuplicateCheckDone(const GURL& query_url,
 
   if (has_duplicates) {
     OfflinePageInfoBarDelegate::Create(
-        base::Bind(&OnInfoBarAction, query_url, j_tab_ref), downloads_label,
-        query_url.spec(), web_contents);
+        base::Bind(&OnInfoBarAction, original_url, j_tab_ref), downloads_label,
+        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(
@@ -393,7 +376,6 @@ void OfflinePageDownloadBridge::StartDownload(
     return;
 
   GURL url = web_contents->GetLastCommittedURL();
-
   ScopedJavaGlobalRef<jobject> j_tab_ref(env, j_tab);
 

[dewittj, 2016/10/12 03:27:27]

if you're deleting blank lines may as well also delete this one :)

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

Restoring blank line, wasnt' deliberate.

   OfflinePageUtils::CheckExistenceOfPagesWithURL(