[Offline pages] Filter out pages cached by different tabs on tab helper

chrome/browser/android/offline_pages/offline_page_tab_helper.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/offline_page_tab_helper.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.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/tab_android.h"
+#include "components/offline_pages/client_namespace_constants.h"
 #include "components/offline_pages/offline_page_model.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/net_errors.h"
 #include "net/base/network_change_notifier.h"
 #include "ui/base/page_transition_types.h"
@@ skipping 42 matching lines @@
   // which are not at the head of the stack.
   const content::NavigationController& controller =
       web_contents()->GetController();
   if (controller.GetEntryCount() > 0 &&
       controller.GetCurrentEntryIndex() != -1 &&
       controller.GetCurrentEntryIndex() < controller.GetEntryCount() - 1) {
     return;
   }
 
   content::BrowserContext* context = web_contents()->GetBrowserContext();
+  if (net::NetworkChangeNotifier::IsOffline()) {
+    GetBestPageAndTryRedirect(navigated_url,
+                              RedirectReason::DISCONNECTED_NETWORK);
+    return;
+  }
+
   OfflinePageModel* offline_page_model =
       OfflinePageModelFactory::GetForBrowserContext(context);
   if (!offline_page_model)
     return;
 
-  if (net::NetworkChangeNotifier::IsOffline()) {
-    offline_page_model->GetBestPageForOnlineURL(
-        navigated_url,
-        base::Bind(&OfflinePageTabHelper::TryRedirectToOffline,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   RedirectReason::DISCONNECTED_NETWORK, navigated_url));
-  } else {
-    offline_page_model->GetPageByOfflineURL(
-        navigated_url,
-        base::Bind(&OfflinePageTabHelper::RedirectToOnline,
-                   weak_ptr_factory_.GetWeakPtr(), navigated_url));
-  }
+  offline_page_model->GetPageByOfflineURL(
+      navigated_url,
+      base::Bind(&OfflinePageTabHelper::RedirectToOnline,
+                 weak_ptr_factory_.GetWeakPtr(), navigated_url));
 }
 
 void OfflinePageTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   // Skips non-main frame.
   if (!navigation_handle->IsInMainFrame())
     return;
 
   GURL navigated_url = navigation_handle->GetURL();
   net::Error error_code = navigation_handle->GetNetErrorCode();
@@ skipping 15 matching lines @@
   // navigation will eventually fail and we want to redirect to offline copy
   // in this case. If error code doesn't match this list, then we still show
   // the error page and not an offline page, so do nothing.
   if (error_code != net::ERR_INTERNET_DISCONNECTED &&
       error_code != net::ERR_NAME_NOT_RESOLVED &&
       error_code != net::ERR_ADDRESS_UNREACHABLE &&
       error_code != net::ERR_PROXY_CONNECTION_FAILED) {
     return;
   }
 
-  OfflinePageModel* offline_page_model =
-      OfflinePageModelFactory::GetForBrowserContext(browser_context);
-  if (!offline_page_model)
-    return;
-
   // Otherwise, get the offline URL for this url, and attempt a redirect if
   // necessary.
   RedirectReason reason =
       ui::PageTransitionTypeIncludingQualifiersIs(
           navigation_handle->GetPageTransition(),
           ui::PAGE_TRANSITION_FORWARD_BACK)
           ? RedirectReason::FLAKY_NETWORK_FORWARD_BACK
           : RedirectReason::FLAKY_NETWORK;
-  offline_page_model->GetBestPageForOnlineURL(
-      navigated_url,
-      base::Bind(&OfflinePageTabHelper::TryRedirectToOffline,
-                 weak_ptr_factory_.GetWeakPtr(), reason, navigated_url));
+  GetBestPageAndTryRedirect(navigated_url, reason);
 }
 
 void OfflinePageTabHelper::RedirectToOnline(
     const GURL& navigated_url,
     const OfflinePageItem* offline_page) {
   // Bails out if no redirection is needed.
   if (!offline_page)
     return;
 
   GURL redirect_url = offline_page->url;
@@ skipping 11 matching lines @@
   Redirect(navigated_url, redirect_url);
   // Clear the offline page since we are redirecting to online.
   offline_page_ = nullptr;
 
   UMA_HISTOGRAM_COUNTS("OfflinePages.RedirectToOnlineCount", 1);
 }
 
 void OfflinePageTabHelper::TryRedirectToOffline(
     RedirectReason redirect_reason,
     const GURL& from_url,
     const OfflinePageItem* offline_page) {

[dewittj, 2016/06/27 21:52:50]

nit: const OfflinePageItem& if there is no case where it's null

[fgorski, 2016/06/27 22:40:09]

Done.

-  if (!offline_page)
-    return;
-
   GURL redirect_url = offline_page->GetOfflineURL();
-
   if (!redirect_url.is_valid())
     return;
 
   if (redirect_reason == RedirectReason::FLAKY_NETWORK ||
       redirect_reason == RedirectReason::FLAKY_NETWORK_FORWARD_BACK) {
     UMA_HISTOGRAM_BOOLEAN("OfflinePages.ShowOfflinePageOnBadNetwork",
                           redirect_reason == RedirectReason::FLAKY_NETWORK);
     // Don't actually want to redirect on a forward/back nav.
     if (redirect_reason == RedirectReason::FLAKY_NETWORK_FORWARD_BACK)
       return;
@@ skipping 14 matching lines @@
   UMA_HISTOGRAM_COUNTS("OfflinePages.RedirectToOfflineCount", 1);
 }
 
 void OfflinePageTabHelper::Redirect(const GURL& from_url, const GURL& to_url) {
   content::NavigationController::LoadURLParams load_params(to_url);
   load_params.transition_type = ui::PAGE_TRANSITION_CLIENT_REDIRECT;
   load_params.redirect_chain.push_back(from_url);
   web_contents()->GetController().LoadURLWithParams(load_params);
 }
 
+void OfflinePageTabHelper::GetBestPageAndTryRedirect(

[dewittj, 2016/06/27 21:52:50]

nit: pass in the model here so we only have to get it in one place.

[fgorski, 2016/06/27 22:40:09]

We call the method in 2 places.

+    const GURL& online_url,
+    RedirectReason reason) {
+  OfflinePageModel* offline_page_model =
+      OfflinePageModelFactory::GetForBrowserContext(
+          web_contents()->GetBrowserContext());
+  if (!offline_page_model)
+    return;
+
+ offline_page_model->GetPagesByOnlineURL(
+      online_url,
+      base::Bind(&OfflinePageTabHelper::SelectBestPageAndTryRedirect,
+                 weak_ptr_factory_.GetWeakPtr(), reason, online_url));
+}
+
+void OfflinePageTabHelper::SelectBestPageAndTryRedirect(
+    RedirectReason reason,
+    const GURL& online_url,
+    const MultipleOfflinePageItemResult& pages) {
+  TabAndroid* tab_android = TabAndroid::FromWebContents(web_contents());
+  std::string tab_id;
+  if (tab_android)

[Dmitry Titov, 2016/06/27 22:06:03]

How about just early return here:
If (!tab_android)
  return;

[fgorski, 2016/06/27 22:40:09]

Good point.

+    tab_id = base::IntToString(tab_android->GetAndroidId());
+

[dewittj, 2016/06/27 21:52:50]

Need to check for a bad android ID?

[fgorski, 2016/06/27 22:40:09]

Dmitry's comment pretty much resolved the problem.

+  const OfflinePageItem* selected_page = nullptr;
+  for (const auto& offline_page : pages) {
+    if (offline_page.client_id.name_space != kLastNNamespace ||

[Dmitry Titov, 2016/06/27 22:06:03]

shouldn't it be:
if (namespace == kLastN && id == tab_id)
?

[fgorski, 2016/06/27 22:40:09]

Updated to explicit check for bookmark namespace and last n

+        offline_page.client_id.id == tab_id) {
+      if (!selected_page ||
+          offline_page.creation_time > selected_page->creation_time)
+        selected_page = &offline_page;
+    }
+  }
+
+  if (!selected_page)
+    return;
+
+  TryRedirectToOffline(reason, online_url, selected_page);
+}
+
 }  // namespace offline_pages