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

chrome/browser/android/offline_pages/offline_page_tab_helper.cc

Index: chrome/browser/android/offline_pages/offline_page_tab_helper.cc
diff --git a/chrome/browser/android/offline_pages/offline_page_tab_helper.cc b/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
index 405ce16a6badac47b4a54b1344aa27f74a5a7ef0..a94e6da46346a8635e92c2747fe0d4043a08e84e 100644
--- a/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/offline_page_tab_helper.cc
@@ -8,9 +8,12 @@
 #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"
@@ -73,23 +76,21 @@ void OfflinePageTabHelper::DidStartNavigation(
   }
 
   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(
@@ -125,11 +126,6 @@ void OfflinePageTabHelper::DidFinishNavigation(
     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 =
@@ -138,10 +134,7 @@ void OfflinePageTabHelper::DidFinishNavigation(
           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(
@@ -174,11 +167,7 @@ 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;
 
@@ -213,4 +202,44 @@ void OfflinePageTabHelper::Redirect(const GURL& from_url, const GURL& to_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