Improve the page download:

chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.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/downloads/offline_page_download_bridge.h"
 
 #include <vector>
 
 #include "base/android/jni_string.h"
 #include "base/bind.h"
@@ skipping 63 matching lines @@
   if (!web_contents)
     return nullptr;
 
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext())
           ->GetOriginalProfile();
 
   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();
+  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(
+            web_contents->GetBrowserContext());
+    request_id = request_coordinator->SavePageLater(
+        url, client_id, true,
+        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.

+  }
+
+  // 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);
 
   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.
-    offline_pages::RequestCoordinator* request_coordinator =
-        offline_pages::RequestCoordinatorFactory::GetForBrowserContext(profile);
-    request_coordinator->SavePageLater(
-        url, client_id, true,
-        RequestCoordinator::RequestAvailability::ENABLED_FOR_OFFLINER);
-
-    notification_bridge.ShowDownloadingToast();
-    return;
-  }
-
-  // Page is ready, capture it right from the tab.
-  offline_pages::OfflinePageModel* offline_page_model =
-      OfflinePageModelFactory::GetForBrowserContext(profile);
-  if (!offline_page_model)
-    return;
-
-  auto archiver =
-      base::MakeUnique<offline_pages::OfflinePageMHTMLArchiver>(web_contents);
-
-  DownloadUIItem item;
-  item.guid = client_id.id;
-  item.url = url;
-
-  notification_bridge.NotifyDownloadProgress(item);
-
   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);
   if (!model)
     return;
 
   std::vector<int64_t> offline_ids;
   for (auto& page : pages) {
     if (page.client_id.name_space == kDownloadNamespace ||
         page.client_id.name_space == kAsyncNamespace) {
       offline_ids.emplace_back(page.offline_id);
     }
   }
 
   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 =
           GetOfflinePageModelFromJavaTab(j_tab_ref);
       if (!offline_page_model)
         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) {
     // TODO(fgorski): Additionally we could update existing request's expiration
     // period, as it is still important. Alternative would be to actually take a
     // snapshot on the spot, but that would only work if the page is loaded
     // enough.
     // This simply toasts that the item is downloading.
     OfflinePageNotificationBridge notification_bridge;
     notification_bridge.ShowDownloadingToast();
     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) {
   content::WebContents* web_contents = GetWebContentsFromJavaTab(j_tab_ref);
   if (!web_contents)
     return;
 
   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(
     JNIEnv* env,
     jobject j_result_obj,
     const std::vector<const DownloadUIItem*>& items) {
   for (const auto item : items) {
     Java_OfflinePageDownloadBridge_createDownloadItemAndAddToList(
         env, j_result_obj, ConvertUTF8ToJavaString(env, item->guid),
         ConvertUTF8ToJavaString(env, item->url.spec()),
@@ skipping 148 matching lines @@
   std::string downloads_label = ConvertJavaStringToUTF8(env, j_downloads_label);
   TabAndroid* tab = TabAndroid::GetNativeTab(env, j_tab);
   if (!tab)
     return;
 
   content::WebContents* web_contents = tab->web_contents();
   if (!web_contents)
     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(
       tab->GetProfile()->GetOriginalProfile(), kDownloadNamespace, url,
       base::Bind(&ModelDuplicateCheckDone, url, j_tab_ref, downloads_label));
 }
 
 void OfflinePageDownloadBridge::CancelDownload(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& j_guid) {
   std::string guid = ConvertJavaStringToUTF8(env, j_guid);
@@ skipping 86 matching lines @@
 
   DownloadUIAdapter* adapter =
       DownloadUIAdapter::FromOfflinePageModel(offline_page_model);
 
   return reinterpret_cast<jlong>(
       new OfflinePageDownloadBridge(env, obj, adapter, browser_context));
 }
 
 }  // namespace android
 }  // namespace offline_pages