[Offline Pages] Fix several infobar bugs.

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"
+#include "base/debug/stack_trace.h"

[fgorski, 2016/10/10 18:08:20]

Do we need this or was it for debugging?

[dewittj, 2016/10/10 18:37:02]

Removed.

 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/android/offline_pages/downloads/offline_page_infobar_delegate.h"
 #include "chrome/browser/android/offline_pages/downloads/offline_page_notification_bridge.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
 #include "chrome/browser/android/offline_pages/offline_page_utils.h"
 #include "chrome/browser/android/offline_pages/recent_tab_helper.h"
 #include "chrome/browser/android/offline_pages/request_coordinator_factory.h"
@@ skipping 38 matching lines @@
 content::WebContents* GetWebContentsFromJavaTab(
     const ScopedJavaGlobalRef<jobject>& j_tab_ref) {
   JNIEnv* env = AttachCurrentThread();
   TabAndroid* tab = TabAndroid::GetNativeTab(env, j_tab_ref);
   if (!tab)
     return nullptr;
 
   return tab->web_contents();
 }
 
+OfflinePageModel* GetOfflinePageModelFromJavaTab(

[fgorski, 2016/10/10 18:08:20]

General comment about the code in this file. I think we should move it down to
download_ui_adapter (if that is possible). I didn't think about that with the
previous patch and I don't expect changes in this one, beyond adding a TODO.

[dewittj, 2016/10/10 18:37:02]

Acknowledged.

+    const ScopedJavaGlobalRef<jobject>& j_tab_ref) {
+  content::WebContents* web_contents = GetWebContentsFromJavaTab(j_tab_ref);
+  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) {
   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.
   GURL url = web_contents->GetLastCommittedURL();
   if (!OfflinePageUtils::EqualsIgnoringFragment(url, query_url))
@@ skipping 34 matching lines @@
   if (!offline_page_model)
     return;
 
   auto archiver =
       base::MakeUnique<offline_pages::OfflinePageMHTMLArchiver>(web_contents);
 
   DownloadUIItem item;
   item.guid = client_id.id;
   item.url = url;
 
-  OfflinePageNotificationBridge bridge;
-  bridge.NotifyDownloadProgress(item);
+  notification_bridge.NotifyDownloadProgress(item);

[fgorski, 2016/10/10 18:08:21]

Good catch.

[dewittj, 2016/10/10 18:37:03]

Acknowledged.

 
   notification_bridge.ShowDownloadingToast();
   offline_page_model->SavePage(url, client_id, 0l, std::move(archiver),
                                base::Bind(&SavePageCallback, item));
 }
 
+void OnDeletePagesForInfoBar(const GURL& query_url,
+                             const ScopedJavaGlobalRef<jobject>& j_tab_ref,
+                             DeletePageResult result) {
+  SavePageIfNavigatedToURL(query_url, j_tab_ref);
+}
+
+void OnGetPagesForDeleteDone(const GURL& query_url,

[fgorski, 2016/10/10 18:08:20]

nit: How about naming it something related to what is happening in the method
and not when?

[dewittj, 2016/10/10 18:37:03]

how about DeletePagesForOverwrite?

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

[fgorski, 2016/10/10 18:08:21]

nit: In the spirit of the above nit, why not simply &SavePageIfNavigatedToURL as
continuation? 

[dewittj, 2016/10/10 18:37:02]

Wanted that, but there is an extra parameter and I didn't know how to ignore
that in a bound function.

+}
+
+void OnInfoBarAction(const GURL& query_url,
+                     const ScopedJavaGlobalRef<jobject>& j_tab_ref,
+                     OfflinePageInfoBarDelegate::Action action) {
+  if (action == OfflinePageInfoBarDelegate::Action::CONFIRM) {
+    SavePageIfNavigatedToURL(query_url, j_tab_ref);
+    return;
+  }
+
+  OfflinePageModel* offline_page_model =
+      GetOfflinePageModelFromJavaTab(j_tab_ref);
+  if (!offline_page_model)

[fgorski, 2016/10/10 18:08:21]

we should probably discuss with PMs the fact that if any step here fails, we
don't give any message to the user.

[dewittj, 2016/10/10 18:37:02]

I don't really expect this to be possible, since we are already going for the
original profile.  Can you think of a case where this would be encountered?  If
so I totally agree.

+    return;
+

[fgorski, 2016/10/10 18:08:20]

please add a dcheck that the action is OVERWRITE at this point. 

[dewittj, 2016/10/10 18:37:02]

Done (used switch for better compile-time checking too.)

+  offline_page_model->GetPagesByOnlineURL(
+      query_url, base::Bind(&OnGetPagesForDeleteDone, query_url, j_tab_ref));
+}
+
 void RequestQueueDuplicateCheckDone(
     const GURL& query_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);
 }
 
 void ModelDuplicateCheckDone(const GURL& query_url,
                              const ScopedJavaGlobalRef<jobject>& j_tab_ref,
+                             const std::string& downloads,
                              bool has_duplicates) {
   content::WebContents* web_contents = GetWebContentsFromJavaTab(j_tab_ref);
   if (!web_contents)
     return;
 
   if (has_duplicates) {
     OfflinePageInfoBarDelegate::Create(
-        base::Bind(&SavePageIfNavigatedToURL, query_url, j_tab_ref),
+        base::Bind(&OnInfoBarAction, query_url, j_tab_ref), downloads,
         query_url.spec(), web_contents);
     return;
   }
 
   OfflinePageUtils::CheckExistenceOfRequestsWithURL(
       Profile::FromBrowserContext(web_contents->GetBrowserContext())
           ->GetOriginalProfile(),
       kDownloadNamespace, query_url,
       base::Bind(&RequestQueueDuplicateCheckDone, query_url, j_tab_ref));
 }
@@ skipping 145 matching lines @@
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& j_guid) {
   std::string guid = ConvertJavaStringToUTF8(env, j_guid);
   return download_ui_adapter_->GetOfflineIdByGuid(guid);
 }
 
 void OfflinePageDownloadBridge::StartDownload(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
-    const JavaParamRef<jobject>& j_tab) {
+    const JavaParamRef<jobject>& j_tab,
+    const JavaParamRef<jstring>& j_downloads_string) {
+  std::string downloads = ConvertJavaStringToUTF8(env, j_downloads_string);
   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);
 
   OfflinePageUtils::CheckExistenceOfPagesWithURL(
       tab->GetProfile()->GetOriginalProfile(), kDownloadNamespace, url,
-      base::Bind(&ModelDuplicateCheckDone, url, j_tab_ref));
+      base::Bind(&ModelDuplicateCheckDone, url, j_tab_ref, downloads));
 }
 
 void OfflinePageDownloadBridge::CancelDownload(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     const JavaParamRef<jstring>& j_guid) {
   std::string guid = ConvertJavaStringToUTF8(env, j_guid);
   RequestCoordinator* coordinator =
       RequestCoordinatorFactory::GetForBrowserContext(browser_context_);
 
@@ skipping 83 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