Track when the auto-fetch in progress notification should be shown

Watch for navigations and request coordinator changes to determine when
the in-progress notification should be shown.

A follow-up CL will introduce an instrumentation test to supplement the
unit tests here.

Bug: 883486

RELAND: This is a reland for https://chromium-review.googlesource.com/c/1381233
Changes to OfflinePageBridge were removed.

TBR=petewil@chromium.org
  This is the same patch, but without the base CL. The base CL was unintentially
  submitted previously.

Change-Id: I8e6988edcc0e8ca810f99c1961c9cdb9f730ba40
Reviewed-on: https://chromium-review.googlesource.com/c/1406939
Commit-Queue: Dan H <harringtond@google.com>
Reviewed-by: Dan H <harringtond@google.com>
Cr-Commit-Position: refs/heads/master@{#622047}

chrome/browser/offline_pages/android/auto_fetch_page_load_watcher.cc

@@ -5,13 +5,20 @@
 #include "chrome/browser/offline_pages/android/auto_fetch_page_load_watcher.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/android/tab_android.h"
 #include "chrome/browser/offline_pages/android/offline_page_auto_fetcher.h"
 #include "chrome/browser/offline_pages/android/offline_page_auto_fetcher_service.h"
 #include "chrome/browser/offline_pages/android/offline_page_auto_fetcher_service_factory.h"
 #include "chrome/browser/offline_pages/request_coordinator_factory.h"
+#include "chrome/browser/ui/android/tab_model/tab_model.h"
+#include "chrome/browser/ui/android/tab_model/tab_model_list.h"
+#include "components/offline_pages/core/auto_fetch.h"
 #include "components/offline_pages/core/background/request_coordinator.h"
+#include "components/offline_pages/core/background/save_page_request.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
@@ -20,6 +27,48 @@
 #include "content/public/browser/web_contents_user_data.h"
 
 namespace offline_pages {
+using auto_fetch_internal::AndroidTabFinder;
+using auto_fetch_internal::InternalImpl;
+using auto_fetch_internal::MakeRequestInfo;
+using auto_fetch_internal::RequestInfo;
+using auto_fetch_internal::TabInfo;
+
+AndroidTabFinder::~AndroidTabFinder() = default;
+
+TabInfo AnroidTabInfo(const TabAndroid& tab) {
+  return {tab.GetAndroidId(), tab.GetURL()};
+}
+
+std::map<int, TabInfo> AndroidTabFinder::FindAndroidTabs(
+    std::vector<int> android_tab_ids) {
+  std::map<int, TabInfo> result;
+  if (android_tab_ids.empty())
+    return result;
+
+  for (TabModelList::const_iterator i = TabModelList::begin();
+       i != TabModelList::end(); i++) {
+    TabModel* model = *i;
+    if (model->IsOffTheRecord())
+      continue;
+
+    for (int index = 0; index < model->GetTabCount(); ++index) {
+      TabAndroid* tab = model->GetTabAt(index);
+      if (std::find(android_tab_ids.begin(), android_tab_ids.end(),
+                    tab->GetAndroidId()) != android_tab_ids.end()) {
+        result[tab->GetAndroidId()] = AnroidTabInfo(*tab);
+      }
+    }
+  }
+  return result;
+}
+
+base::Optional<TabInfo> AndroidTabFinder::FindNavigationTab(
+    content::WebContents* web_contents) {
+  TabAndroid* tab = TabAndroid::FromWebContents(web_contents);
+  if (!tab)
+    return base::nullopt;
+  return AnroidTabInfo(*tab);
+}
 
 // Observes a WebContents to relay navigation events to
 // AutoFetchPageLoadWatcher.
@@ -30,30 +79,26 @@ class AutoFetchPageLoadWatcher::NavigationObserver
  public:
   explicit NavigationObserver(content::WebContents* web_contents)
       : content::WebContentsObserver(web_contents) {
-    helper_ = OfflinePageAutoFetcherServiceFactory::GetForBrowserContext(
-                  web_contents->GetBrowserContext())
-                  ->page_load_watcher();
-    DCHECK(helper_);
+    page_load_watcher_ =
+        OfflinePageAutoFetcherServiceFactory::GetForBrowserContext(
+            web_contents->GetBrowserContext())
+            ->page_load_watcher();
+    DCHECK(page_load_watcher_);
   }
 
   // content::WebContentsObserver implementation.
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override {
     if (!navigation_handle->IsInMainFrame() ||
-        !navigation_handle->HasCommitted() || navigation_handle->IsErrorPage())
+        !navigation_handle->HasCommitted())
       return;
-
-    // Note: The redirect chain includes the final URL. We consider all URLs
-    // along the redirect chain as successful.
-    for (const auto& u : navigation_handle->GetRedirectChain()) {
-      helper_->HandlePageNavigation(u);
-    }
+    page_load_watcher_->HandleNavigation(navigation_handle);
   }
 
  private:
   friend class content::WebContentsUserData<
       AutoFetchPageLoadWatcher::NavigationObserver>;
-  AutoFetchPageLoadWatcher* helper_;
+  AutoFetchPageLoadWatcher* page_load_watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(NavigationObserver);
   WEB_CONTENTS_USER_DATA_KEY_DECL();
@@ -74,84 +119,253 @@ void AutoFetchPageLoadWatcher::CreateForWebContents(
   }
 }
 
-AutoFetchPageLoadWatcher::AutoFetchPageLoadWatcher(
-    RequestCoordinator* request_coordinator)
-    : request_coordinator_(request_coordinator) {
-  request_coordinator_->AddObserver(this);
-  request_coordinator_->GetAllRequests(
-      base::BindOnce(&AutoFetchPageLoadWatcher::ObserverInitialize,
-                     weak_ptr_factory_.GetWeakPtr()));
+namespace auto_fetch_internal {
+
+base::Optional<RequestInfo> MakeRequestInfo(const SavePageRequest& request) {
+  base::Optional<auto_fetch::ClientIdMetadata> metadata =
+      auto_fetch::ExtractMetadata(request.client_id());
+  if (!metadata)
+    return base::nullopt;
+
+  RequestInfo info;
+  info.request_id = request.request_id();
+  info.url = request.url();
+  info.metadata = metadata.value();
+  info.notification_state = request.auto_fetch_notification_state();
+  return info;
 }
 
-AutoFetchPageLoadWatcher::~AutoFetchPageLoadWatcher() {
-  request_coordinator_->RemoveObserver(this);
+InternalImpl::InternalImpl(AutoFetchNotifier* notifier,
+                           Delegate* delegate,
+                           std::unique_ptr<AndroidTabFinder> tab_finder)
+    : notifier_(notifier),
+      delegate_(delegate),
+      tab_finder_(std::move(tab_finder)) {}
+
+InternalImpl::~InternalImpl() {}
+
+void InternalImpl::RequestListInitialized(std::vector<RequestInfo> request) {
+  DCHECK(!requests_initialized_);
+  requests_initialized_ = true;
+  requests_ = std::move(request);
+
+  for (const GURL& url : pages_loaded_before_observer_ready_) {
+    SuccessfulPageNavigation(url);
+  }
+  pages_loaded_before_observer_ready_.clear();
+
+  // Now that we have the full list of requests, we need to verify that the
+  // notification state is correct. For instance, if a tab was closed or
+  // naviagated away from the request URL, we need to trigger the in-progress
+  // notification.
+
+  // For requests that haven't yet produced an in-progress notification, we need
+  // to find out if the request URL is currently bound to the expected tab. If
+  // not, trigger the in-progress notification.
+  std::vector<int> android_tab_ids;
+  for (const RequestInfo& request : requests_) {
+    if (request.notification_state ==
+        SavePageRequest::AutoFetchNotificationState::kUnknown) {
+      android_tab_ids.push_back(request.metadata.android_tab_id);
+    }
+  }
+
+  const std::map<int, TabInfo> android_tabs =
+      tab_finder_->FindAndroidTabs(android_tab_ids);
+  for (RequestInfo& request : requests_) {
+    if (request.notification_state ==
+        SavePageRequest::AutoFetchNotificationState::kUnknown) {
+      auto tab_iterator = android_tabs.find(request.metadata.android_tab_id);
+      if (tab_iterator == android_tabs.end() ||
+          tab_iterator->second.current_url != request.url) {
+        SetNotificationStateToShown(request.request_id);
+      }
+    }
+  }
 }
 
-void AutoFetchPageLoadWatcher::OnAdded(const SavePageRequest& request) {
-  if (!observer_ready_)
+void InternalImpl::RequestAdded(RequestInfo request) {
+  if (!requests_initialized_)
     return;
-  if (request.client_id().name_space == kAutoAsyncNamespace) {
-    live_auto_fetch_requests_[request.url()].push_back(request.request_id());
-  }
+
+  requests_.push_back(request);
+  // Because interaction with RequestCoordinator is asynchronous, we need to
+  // check if the request is no longer tied to a tab, and issue the in-progress
+  // notification.
+  if (request.notification_state ==
+      SavePageRequest::AutoFetchNotificationState::kShown)
+    return;
+
+  const std::map<int, TabInfo> android_tabs =
+      tab_finder_->FindAndroidTabs({request.metadata.android_tab_id});
+  if (android_tabs.empty())
+    delegate_->SetNotificationStateToShown(request.request_id);
+
+  // TODO(harringtond): it's also possible that the request should be removed
+  // because a successful navigation happened before the request could be added
+  // to the database. We might be able to catch this case by remembering some
+  // set of previous successful navigations along with timestamps, but even that
+  // isn't perfect.
+  // The upshot is that we risk auto-fetching a page and notifying the user even
+  // after they've already loaded it.
 }
 
-void AutoFetchPageLoadWatcher::OnCompleted(
-    const SavePageRequest& request,
-    RequestNotifier::BackgroundSavePageResult status) {
-  if (!observer_ready_)
+void InternalImpl::RequestRemoved(RequestInfo request) {
+  if (!requests_initialized_)
     return;
-  // A SavePageRequest is complete, remove our bookeeping of it in
-  // |live_auto_fetch_requests_|.
-  auto iter = live_auto_fetch_requests_.find(request.url());
-  if (iter != live_auto_fetch_requests_.end()) {
-    std::vector<int64_t>& id_list = iter->second;
-    auto id_iter =
-        std::find(id_list.begin(), id_list.end(), request.request_id());
-    if (id_iter != id_list.end()) {
-      id_list.erase(id_iter);
-      if (id_list.empty())
-        live_auto_fetch_requests_.erase(iter);
-    }
+
+  for (size_t i = 0; i < requests_.size(); ++i) {
+    RequestInfo info = requests_[i];
+    if (info.request_id == request.request_id)
+      requests_.erase(requests_.begin() + i);
   }
+  notifier_->InProgressCountChanged(requests_.size());
 }
 
-void AutoFetchPageLoadWatcher::RemoveRequests(
-    const std::vector<int64_t>& request_ids) {
-  request_coordinator_->RemoveRequests(request_ids, base::DoNothing());
+void InternalImpl::SetNotificationStateComplete(int64_t request_id,
+                                                bool success) {
+  if (!success)
+    return;
+
+  notifier_->NotifyInProgress(requests_.size());
 }
 
-void AutoFetchPageLoadWatcher::HandlePageNavigation(const GURL& url) {
+// Called when a successful navigation to |url| happens.
+// If URL is loaded successfully on tab, cancel the auto-fetch request.
+void InternalImpl::SuccessfulPageNavigation(const GURL& url) {
   // Early exit for the common-case.
-  if (observer_ready_ && live_auto_fetch_requests_.empty()) {
+  if (requests_initialized_ && requests_.empty())
     return;
-  }
 
-  // Note: It is possible that this method is called before
-  // ObserverInitialized, in which case we have to defer handling of the
+  // If the request list isn't yet initialized, we have to defer handling of the
   // event. Never accumulate more than a few, so we can't have a boundless
-  // array if ObserverInitialize is never called.
-  if (!observer_ready_) {
+  // array. This means we will fail to cancel an auto-fetch request if too many
+  // navigations occur before |RequestListInitialized|.
+  if (!requests_initialized_) {
     if (pages_loaded_before_observer_ready_.size() < 10)
       pages_loaded_before_observer_ready_.push_back(url);
     return;
   }
 
-  auto iter = live_auto_fetch_requests_.find(url);
-  if (iter == live_auto_fetch_requests_.end())
+  std::vector<int64_t> remove_ids;
+  for (const RequestInfo& request : requests_) {
+    if (request.url == url)
+      remove_ids.push_back(request.request_id);
+  }
+  if (!remove_ids.empty())
+    delegate_->RemoveRequests(remove_ids);
+}
+
+void InternalImpl::NavigationFrom(const GURL& previous_url,
+                                  content::WebContents* web_contents) {
+  // Early exit for the common-case. We can ignore events from before the
+  // request list is initialized because we reconcile things in
+  // |RequestListInitialized|.
+  if (!requests_initialized_ || requests_.empty())
     return;
-  RemoveRequests(iter->second);
+
+  // Find requests that haven't yet been notified, and that match the
+  // navigated-from URL.
+  for (RequestInfo& request : requests_) {
+    if (request.url == previous_url &&
+        request.notification_state ==
+            SavePageRequest::AutoFetchNotificationState::kUnknown) {
+      // Check that the navigation is happening on the tab from which the
+      // request came.
+      base::Optional<TabInfo> tab =
+          tab_finder_->FindNavigationTab(web_contents);
+      if (tab && tab->android_tab_id == request.metadata.android_tab_id)
+        SetNotificationStateToShown(request.request_id);
+    }
+  }
 }
 
-void AutoFetchPageLoadWatcher::ObserverInitialize(
-    std::vector<std::unique_ptr<SavePageRequest>> all_requests) {
-  observer_ready_ = true;
-  for (const std::unique_ptr<SavePageRequest>& request : all_requests) {
-    OnAdded(*request);
+void InternalImpl::SetNotificationStateToShown(int64_t request_id) {
+  const auto kShown = SavePageRequest::AutoFetchNotificationState::kShown;
+  for (RequestInfo& request : requests_) {
+    if (request.request_id == request_id)
+      request.notification_state = kShown;
   }
-  for (const GURL& url : pages_loaded_before_observer_ready_) {
-    HandlePageNavigation(url);
+  delegate_->SetNotificationStateToShown(request_id);
+}
+
+}  // namespace auto_fetch_internal
+
+AutoFetchPageLoadWatcher::AutoFetchPageLoadWatcher(
+    AutoFetchNotifier* notifier,
+    RequestCoordinator* request_coordinator,
+    std::unique_ptr<AndroidTabFinder> tab_finder)
+    : request_coordinator_(request_coordinator),
+      impl_(notifier, this, std::move(tab_finder)) {
+  request_coordinator_->AddObserver(this);
+  request_coordinator_->GetAllRequests(base::BindOnce(
+      &AutoFetchPageLoadWatcher::InitializeRequestList, GetWeakPtr()));
+}
+
+AutoFetchPageLoadWatcher::~AutoFetchPageLoadWatcher() {
+  request_coordinator_->RemoveObserver(this);
+}
+
+void AutoFetchPageLoadWatcher::RemoveRequests(
+    const std::vector<int64_t>& request_ids) {
+  request_coordinator_->RemoveRequests(request_ids, base::DoNothing());
+}
+
+void AutoFetchPageLoadWatcher::HandleNavigation(
+    content::NavigationHandle* navigation_handle) {
+  // First, call HandleSuccessfulPageNavigation() if this is a successful
+  // navigation.
+  if (!navigation_handle->IsErrorPage()) {
+    // Note: The redirect chain includes the final URL. We consider all URLs
+    // along the redirect chain as successful.
+    for (const auto& url : navigation_handle->GetRedirectChain()) {
+      impl_.SuccessfulPageNavigation(url);
+    }
   }
-  pages_loaded_before_observer_ready_.clear();
+
+  // Ignore if the URL didn't change.
+  const GURL& previous_url = navigation_handle->GetPreviousURL();
+  if (navigation_handle->GetURL() == previous_url)
+    return;
+
+  impl_.NavigationFrom(previous_url, navigation_handle->GetWebContents());
+}
+
+void AutoFetchPageLoadWatcher::SetNotificationStateToShown(int64_t request_id) {
+  request_coordinator_->SetAutoFetchNotificationState(
+      request_id, SavePageRequest::AutoFetchNotificationState::kShown,
+      base::BindOnce(&InternalImpl::SetNotificationStateComplete,
+                     impl_.GetWeakPtr(), request_id));
+}
+
+void AutoFetchPageLoadWatcher::OnAdded(const SavePageRequest& request) {
+  base::Optional<RequestInfo> info = MakeRequestInfo(request);
+  if (!info)
+    return;
+
+  impl_.RequestAdded(std::move(info.value()));
+}
+
+void AutoFetchPageLoadWatcher::OnCompleted(
+    const SavePageRequest& request,
+    RequestNotifier::BackgroundSavePageResult status) {
+  base::Optional<RequestInfo> info = MakeRequestInfo(request);
+  if (!info)
+    return;
+
+  impl_.RequestRemoved(std::move(info.value()));
+}
+
+void AutoFetchPageLoadWatcher::InitializeRequestList(
+    std::vector<std::unique_ptr<SavePageRequest>> requests) {
+  std::vector<RequestInfo> request_infos;
+  for (const auto& request : requests) {
+    base::Optional<RequestInfo> info = MakeRequestInfo(*request);
+    if (!info)
+      continue;
+    request_infos.push_back(info.value());
+  }
+  impl_.RequestListInitialized(std::move(request_infos));
 }
 
 }  // namespace offline_pages

chrome/browser/offline_pages/android/auto_fetch_page_load_watcher.h

@@ -12,60 +12,175 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "components/offline_pages/core/auto_fetch.h"
 #include "components/offline_pages/core/background/request_coordinator.h"
 #include "components/offline_pages/core/client_id.h"
 #include "url/gurl.h"
 
 namespace content {
 class WebContents;
+class NavigationHandle;
 }  // namespace content
 
 namespace offline_pages {
 class SavePageRequest;
 class RequestCoordinator;
 
-// Cancels active auto fetch requests if a page loads.
-class AutoFetchPageLoadWatcher : public RequestCoordinator::Observer {
+// Manages showing the in-progress notification.
+class AutoFetchNotifier {
  public:
-  static void CreateForWebContents(content::WebContents* web_contents);
+  virtual ~AutoFetchNotifier() {}
+  // Ensures that the in-progress notification is showing with the appropriate
+  // request count.
+  virtual void NotifyInProgress(int in_flight_count) {}
+  // Update the request count if the in-progress notification is already
+  // showing. This won't trigger showing the notification if it's not already
+  // shown. If |in_flight_count| is 0, the notification will be hidden.
+  virtual void InProgressCountChanged(int in_flight_count) {}
+};
 
-  explicit AutoFetchPageLoadWatcher(RequestCoordinator* request_coordinator);
-  ~AutoFetchPageLoadWatcher() override;
+// Types and functions internal to AutoFetchPageLoadWatcher. Included in the
+// header for testing.
+namespace auto_fetch_internal {
 
-  // RequestCoordinator::Observer methods. These keep
-  // |live_auto_fetch_requests_| in sync with the state of RequestCoordinator.
-  void OnAdded(const SavePageRequest& request) override;
-  void OnCompleted(const SavePageRequest& request,
-                   RequestNotifier::BackgroundSavePageResult status) override;
-  void OnChanged(const SavePageRequest& request) override {}
-  void OnNetworkProgress(const SavePageRequest& request,
-                         int64_t received_bytes) override {}
+// Information about an Android browser tab.
+struct TabInfo {
+  int android_tab_id = 0;
+  GURL current_url;
+};
 
- protected:
-  class NavigationObserver;
+// Interface to Android tabs used by |AutoFetchPageLoadWatcher|. This is the
+// real implementation, methods are virtual for testing only.
+class AndroidTabFinder {
+ public:
+  virtual ~AndroidTabFinder();
+  // Returns a mapping of Android tab ID to TabInfo.
+  virtual std::map<int, TabInfo> FindAndroidTabs(
+      std::vector<int> android_tab_ids);
+  virtual base::Optional<TabInfo> FindNavigationTab(
+      content::WebContents* web_contents);
+};
+
+// Information about an auto-fetch |SavePageRequest|.
+struct RequestInfo {
+  int64_t request_id;
+  GURL url;
+  SavePageRequest::AutoFetchNotificationState notification_state;
+  auto_fetch::ClientIdMetadata metadata;
+};
+
+base::Optional<RequestInfo> MakeRequestInfo(const SavePageRequest& request);
+
+// |AutoFetchPageLoadWatcher|'s more unit-testable internal implementation.
+// This class was designed to have few dependencies to make testing more
+// tractable. Events are communicated to |InternalImpl| through its public
+// member functions, and functions are injected through |AutoFetchNotifier|,
+// |Delegate|, and |AndroidTabFinder|.
+class InternalImpl {
+ public:
+  // Injected functions, implemented by |AutoFetchPageLoadWatcher|.
+  // We need this because we can't call these functions directly.
+  class Delegate {
+   public:
+    virtual ~Delegate() {}
+    // Sets the notification state of a request to
+    // |SavePageRequest::AutoFetchNotificationState::kShown|. Results in a call
+    // to |SetNotificationStateComplete|.
+    virtual void SetNotificationStateToShown(int64_t request_id) {}
+    // Removes all |SavePageRequest|s with the given IDs.
+    virtual void RemoveRequests(const std::vector<int64_t>& request_ids) {}
+  };
+
+  InternalImpl(AutoFetchNotifier* notifier,
+               Delegate* delegate,
+               std::unique_ptr<AndroidTabFinder> tab_finder);
+  ~InternalImpl();
+
+  // Request state change methods.
+  void RequestListInitialized(std::vector<RequestInfo> requests);
+  void RequestAdded(RequestInfo info);
+  void RequestRemoved(RequestInfo info);
+  void SetNotificationStateComplete(int64_t request_id, bool success);
+
+  // Navigation methods.
+  void SuccessfulPageNavigation(const GURL& url);
+  void NavigationFrom(const GURL& previous_url,
+                      content::WebContents* web_contents);
+
+  base::WeakPtr<InternalImpl> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+ private:
+  void SetNotificationStateToShown(int64_t request_id);
+
+  AutoFetchNotifier* notifier_;
+  Delegate* delegate_;
+  std::unique_ptr<AndroidTabFinder> tab_finder_;
+  std::vector<RequestInfo> requests_;
+  // Tracks whether |RequestListInitialized| has been called. If false,
+  // |RequestAdded| and |RequestRemoved| should be ignored, as per the
+  // documentation in |RequestCoordinator::Observer|.
+  bool requests_initialized_ = false;
+  std::vector<GURL> pages_loaded_before_observer_ready_;
 
-  // Virtual for testing only.
-  virtual void RemoveRequests(const std::vector<int64_t>& request_ids);
+  base::WeakPtrFactory<InternalImpl> weak_ptr_factory_{this};
+};
 
-  // Called when navigation completes. Triggers cancellation of any
-  // in-flight auto fetch requests that match a successful navigation.
-  void HandlePageNavigation(const GURL& url);
+}  // namespace auto_fetch_internal
+
+// Watches for page loads and |RequestCoordinator| requests.
+// Given an active auto-fetch request for <tab, URL>:
+// - If URL is loaded successfully on tab, cancel the auto-fetch request.
+// - If a different URL is loaded successfully on tab, trigger the in-progress
+//   notification.
+// - If an auto-fetch request is removed, update the in-progress notification's
+//   displayed request count.
+//
+// Implementation note:
+// This class simply observes events and passes them down to |InternalImpl|
+// for processing. All code here is run on the UI thread.
+class AutoFetchPageLoadWatcher
+    : public RequestCoordinator::Observer,
+      public auto_fetch_internal::InternalImpl::Delegate {
+ public:
+  using AndroidTabFinder = auto_fetch_internal::AndroidTabFinder;
 
-  void ObserverInitialize(
-      std::vector<std::unique_ptr<SavePageRequest>> all_requests);
+  static void CreateForWebContents(content::WebContents* web_contents);
 
-  offline_pages::RequestCoordinator* GetRequestCoordinator();
+  AutoFetchPageLoadWatcher(AutoFetchNotifier* notifier,
+                           RequestCoordinator* request_coordinator,
+                           std::unique_ptr<AndroidTabFinder> tab_finder);
 
-  // Whether ObserverInitialize has been called.
-  bool observer_ready_ = false;
+  ~AutoFetchPageLoadWatcher() override;
 
-  RequestCoordinator* request_coordinator_;
-  std::vector<GURL> pages_loaded_before_observer_ready_;
+  // Called when navigation completes, even on errors. This is only called
+  // once per navigation.
+  void HandleNavigation(content::NavigationHandle* navigation_handle);
+
+ private:
+  class NavigationObserver;
+  base::WeakPtr<AutoFetchPageLoadWatcher> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
 
-  // This represents the set of auto fetch request IDs currently in the
-  // RequestCoordinator's queue, mapped by URL.
-  std::map<GURL, std::vector<int64_t>> live_auto_fetch_requests_;
+  void InitializeRequestList(
+      std::vector<std::unique_ptr<SavePageRequest>> requests);
+
+  // InternalImpl::Delegate.
+  void SetNotificationStateToShown(int64_t request_id) override;
+  void RemoveRequests(const std::vector<int64_t>& request_ids) override;
+
+  // RequestCoordinator::Observer.
+  void OnAdded(const SavePageRequest& request) override;
+  void OnCompleted(const SavePageRequest& request,
+                   RequestNotifier::BackgroundSavePageResult status) override;
+  void OnChanged(const SavePageRequest& request) override {}
+  void OnNetworkProgress(const SavePageRequest& request,
+                         int64_t received_bytes) override {}
 
+  RequestCoordinator* request_coordinator_;  // Not owned.
+  auto_fetch_internal::InternalImpl impl_;
   base::WeakPtrFactory<AutoFetchPageLoadWatcher> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(AutoFetchPageLoadWatcher);

chrome/browser/offline_pages/android/auto_fetch_page_load_watcher_unittest.cc

@@ -8,253 +8,238 @@
 #include <string>
 #include <utility>
 
-#include "base/test/bind_test_util.h"
-#include "base/test/test_simple_task_runner.h"
-#include "base/threading/thread_task_runner_handle.h"
-#include "chrome/browser/offline_pages/request_coordinator_factory.h"
-#include "chrome/test/base/chrome_render_view_host_test_harness.h"
-#include "chrome/test/base/testing_profile.h"
-#include "components/offline_pages/core/background/request_coordinator_stub_taco.h"
+#include "chrome/browser/offline_pages/android/offline_page_auto_fetcher_service.h"
+#include "chrome/browser/offline_pages/android/offline_page_auto_fetcher_service_factory.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
-#include "content/public/test/navigation_simulator.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 namespace {
-using content::NavigationSimulator;
+using ::offline_pages::auto_fetch_internal::AndroidTabFinder;
+using ::offline_pages::auto_fetch_internal::InternalImpl;
+using ::offline_pages::auto_fetch_internal::RequestInfo;
+using ::offline_pages::auto_fetch_internal::TabInfo;
+using ::testing::_;
+
+const int kDefaultTabId = 123;
+const base::Time kEpoch = base::Time::FromDoubleT(1.0e6);
 
 GURL TestURL() {
   return GURL("http://www.url.com");
 }
+GURL OtherURL() {
+  return GURL("http://other.com");
+}
 
-SavePageRequest TestRequest(int64_t id, const GURL& url = TestURL()) {
-  return SavePageRequest(id, url,
-                         ClientId(kAutoAsyncNamespace, std::to_string(id)),
-                         base::Time::Now(), false);
+RequestInfo TestInfo(
+    int64_t id,
+    const GURL& url = TestURL(),
+    SavePageRequest::AutoFetchNotificationState notification_state =
+        SavePageRequest::AutoFetchNotificationState::kUnknown) {
+  RequestInfo info;
+  info.request_id = id;
+  info.url = url;
+  info.notification_state = notification_state;
+  info.metadata.android_tab_id = kDefaultTabId;
+  return info;
 }
 
-// For access to protected methods.
-class TestAutoFetchPageLoadWatcher : public AutoFetchPageLoadWatcher {
+class MockAutoFetchNotifier : public AutoFetchNotifier {
  public:
-  explicit TestAutoFetchPageLoadWatcher(RequestCoordinator* request_coordinator)
-      : AutoFetchPageLoadWatcher(request_coordinator) {}
-  void RemoveRequests(const std::vector<int64_t>& request_ids) override {
-    removed_ids_ = request_ids;
-  }
-  std::vector<int64_t> removed_ids() const { return removed_ids_; }
-
-  std::map<GURL, std::vector<int64_t>>* live_auto_fetch_requests() {
-    return &live_auto_fetch_requests_;
-  }
-
-  using AutoFetchPageLoadWatcher::HandlePageNavigation;
-  using AutoFetchPageLoadWatcher::ObserverInitialize;
-
- private:
-  std::vector<int64_t> removed_ids_;
+  MOCK_METHOD1(NotifyInProgress, void(int in_flight_count));
+  MOCK_METHOD1(InProgressCountChanged, void(int in_flight_count));
 };
 
-// Tests AutoFetchPageLoadWatcher in a realistic way by simulating navigations.
-class AutoFetchPageLoadWatcherNavigationTest
-    : public ChromeRenderViewHostTestHarness {
+class FakeInternalImplDelegate : public InternalImpl::Delegate {
  public:
-  AutoFetchPageLoadWatcherNavigationTest() = default;
-  ~AutoFetchPageLoadWatcherNavigationTest() override = default;
-
-  void SetUp() override {
-    ChromeRenderViewHostTestHarness::SetUp();
-    RequestCoordinatorFactory::GetInstance()->SetTestingFactoryAndUse(
-        profile(), request_coordinator_taco_.FactoryFunction());
-
-    AutoFetchPageLoadWatcher::CreateForWebContents(web_contents());
+  ~FakeInternalImplDelegate() override {}
+  void SetNotificationStateToShown(int64_t request_id) override {
+    set_notification_state_requests.push_back(request_id);
   }
-
-  std::unique_ptr<NavigationSimulator> CreateNavigation(const GURL& url) {
-    return NavigationSimulator::CreateRendererInitiated(url, main_rfh());
+  void RemoveRequests(const std::vector<int64_t>& request_ids) override {
+    removed_requests.insert(removed_requests.end(), request_ids.begin(),
+                            request_ids.end());
   }
 
-  RequestCoordinator* request_coordinator() {
-    return request_coordinator_taco_.request_coordinator();
-  }
+  std::vector<int64_t> removed_requests;
+  std::vector<int64_t> set_notification_state_requests;
+};
 
-  void AddAutoSavePageRequest() {
-    RequestCoordinator::SavePageLaterParams params;
-    params.url = TestURL();
-    params.client_id = ClientId(kAutoAsyncNamespace, "request1");
-    request_coordinator()->SavePageLater(params, base::DoNothing());
+// Note that TabAndroid doesn't work in unit tests, so this stubs out access to
+// tab information.
+class StubTabFinder : public AutoFetchPageLoadWatcher::AndroidTabFinder {
+ public:
+  ~StubTabFinder() override {}
+
+  // AutoFetchPageLoadWatcher::AndroidTabFinder.
+  std::map<int, TabInfo> FindAndroidTabs(
+      std::vector<int> android_tab_ids) override {
+    std::map<int, TabInfo> result;
+    for (const int tab_id : android_tab_ids) {
+      if (tabs_.count(tab_id)) {
+        result[tab_id] = TabInfo{tab_id, tabs_[tab_id]};
+      }
+    }
+    return result;
   }
 
-  std::vector<GURL> RequestsInQueue() {
-    std::vector<GURL> request_urls;
-    request_coordinator()->GetAllRequests(base::BindLambdaForTesting(
-        [&](std::vector<std::unique_ptr<SavePageRequest>> requests) {
-          for (const auto& request : requests) {
-            request_urls.push_back(request->url());
-          }
-        }));
-    RunUntilIdle();
-    return request_urls;
+  base::Optional<TabInfo> FindNavigationTab(
+      content::WebContents* web_contents) override {
+    if (!tabs_.count(current_tab_id_))
+      return base::nullopt;
+    return TabInfo{current_tab_id_, tabs_[current_tab_id_]};
   }
 
-  void RunUntilIdle() { thread_bundle()->RunUntilIdle(); }
+  // Methods to alter stub behavior.
+  void SetTabs(std::map<int, GURL> tab_urls) { tabs_ = std::move(tab_urls); }
+  void SetCurrentTabId(int tab_id) { current_tab_id_ = tab_id; }
 
  private:
-  RequestCoordinatorStubTaco request_coordinator_taco_;
+  std::map<int, GURL> tabs_;
+  // ID of the current tab to return from FindNavigationTab.
+  int current_tab_id_ = kDefaultTabId;
+};
 
-  DISALLOW_COPY_AND_ASSIGN(AutoFetchPageLoadWatcherNavigationTest);
+// Note: This unittest doesn't attempt to directly test
+// |AutoFetchPageLoadWatcher| because the set-up is difficult, especially for
+// testing various call orderings. Additionally, TabAndroid can't be tested
+// in a unit test. Instead, see OfflinePageAutoFetchTest.java for coverage.
+class AutoFetchInternalImplTest : public testing::Test {
+ public:
+ protected:
+  // A WebContents* is needed for some |InternalImpl| methods, but nullptr is
+  // sufficient because |StubTabFinder| doesn't inspect the value.
+  content::WebContents* const web_contents_ = nullptr;
+  MockAutoFetchNotifier notifier_;
+  FakeInternalImplDelegate delegate_;
+  StubTabFinder* tab_finder_ = new StubTabFinder;
+  InternalImpl impl_{&notifier_, &delegate_, base::WrapUnique(tab_finder_)};
 };
 
-// Navigation results in an error page, and has no effect on
-// AutoFetchPageLoadWatcher.
-TEST_F(AutoFetchPageLoadWatcherNavigationTest, NavigateToErrorPage) {
-  AddAutoSavePageRequest();
-  RunUntilIdle();
+TEST_F(AutoFetchInternalImplTest, NoInitialization) {
+  // Just verify there is no crash.
+  impl_.SuccessfulPageNavigation(TestURL());
+  impl_.NavigationFrom(TestURL(), web_contents_);
+}
 
-  std::unique_ptr<NavigationSimulator> simulator = CreateNavigation(TestURL());
-  simulator->Start();
-  simulator->Fail(net::ERR_TIMED_OUT);
-  simulator->CommitErrorPage();
+TEST_F(AutoFetchInternalImplTest, RemoveRequestOnSuccessfulNavigation) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, TestURL()}});
+  impl_.RequestListInitialized(
+      std::vector<RequestInfo>{TestInfo(1, TestURL())});
+  impl_.SuccessfulPageNavigation(TestURL());
 
-  std::vector<GURL> expected_requests{TestURL()};
-  EXPECT_EQ(expected_requests, RequestsInQueue());
+  EXPECT_EQ(std::vector<int64_t>({1}), delegate_.removed_requests);
 }
 
-// Successful navigation results in cancellation of request.
-TEST_F(AutoFetchPageLoadWatcherNavigationTest, NavigateAndCancel) {
-  AddAutoSavePageRequest();
-  RunUntilIdle();
-
-  std::unique_ptr<NavigationSimulator> simulator = CreateNavigation(TestURL());
-  simulator->Start();
-  simulator->Commit();
+TEST_F(AutoFetchInternalImplTest,
+       RemoveRequestOnSuccessfulNavigationBeforeInitialization) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, TestURL()}});
+  impl_.SuccessfulPageNavigation(TestURL());
+  impl_.RequestListInitialized(
+      std::vector<RequestInfo>{TestInfo(1, TestURL())});
 
-  std::vector<GURL> expected_requests{};
-  EXPECT_EQ(expected_requests, RequestsInQueue());
+  EXPECT_EQ(std::vector<int64_t>({1}), delegate_.removed_requests);
 }
 
-TEST_F(AutoFetchPageLoadWatcherNavigationTest, NavigateToDifferentURL) {
-  AddAutoSavePageRequest();
-  RunUntilIdle();
+// Successful navigation to a URL that we are not auto fetching should not
+// remove any requests from the list of in progress auto fetches.
+TEST_F(AutoFetchInternalImplTest, SuccessfulNavigationToOtherURL) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, TestURL()}});
+  impl_.RequestListInitialized(
+      std::vector<RequestInfo>{TestInfo(1, TestURL())});
+  impl_.SuccessfulPageNavigation(OtherURL());
 
-  std::unique_ptr<NavigationSimulator> simulator =
-      CreateNavigation(GURL("http://www.different.com"));
-  simulator->Start();
-  simulator->Commit();
-
-  std::vector<GURL> expected_requests{TestURL()};
-  EXPECT_EQ(expected_requests, RequestsInQueue());
+  EXPECT_EQ(std::vector<int64_t>(), delegate_.removed_requests);
 }
 
-TEST_F(AutoFetchPageLoadWatcherNavigationTest, RedirectToAndCancel) {
-  AddAutoSavePageRequest();
-  RunUntilIdle();
-
-  std::unique_ptr<NavigationSimulator> simulator =
-      CreateNavigation(GURL("http://different.com"));
-  simulator->Start();
-  simulator->Redirect(TestURL());
-  simulator->Commit();
+TEST_F(AutoFetchInternalImplTest,
+       SuccessfulNavigationToOtherURLBeforeInitialization) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, TestURL()}});
+  impl_.SuccessfulPageNavigation(OtherURL());
+  impl_.RequestListInitialized(
+      std::vector<RequestInfo>{TestInfo(1, TestURL())});
 
-  std::vector<GURL> expected_requests{};
-  EXPECT_EQ(expected_requests, RequestsInQueue());
+  EXPECT_EQ(std::vector<int64_t>(), delegate_.removed_requests);
 }
 
-TEST_F(AutoFetchPageLoadWatcherNavigationTest, RedirectFromAndCancel) {
-  AddAutoSavePageRequest();
-  RunUntilIdle();
+TEST_F(AutoFetchInternalImplTest, NavigatingFromNotifies) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, TestURL()}});
+  impl_.RequestListInitialized(
+      std::vector<RequestInfo>{TestInfo(1, TestURL())});
 
-  std::unique_ptr<NavigationSimulator> simulator = CreateNavigation(TestURL());
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, OtherURL()}});
+  impl_.NavigationFrom(TestURL(), web_contents_);
+  EXPECT_EQ(std::vector<int64_t>({1}),
+            delegate_.set_notification_state_requests);
 
-  simulator->Start();
-  simulator->Redirect(GURL("http://different.com"));
-  simulator->Commit();
-
-  std::vector<GURL> expected_requests{};
-  EXPECT_EQ(expected_requests, RequestsInQueue());
+  EXPECT_CALL(notifier_, NotifyInProgress(1));
+  impl_.SetNotificationStateComplete(1, true);
 }
 
-// Tests some details of AutoFetchPageLoadWatcher
-class AutoFetchPageLoadWatcherTest : public testing::Test {
- public:
-  AutoFetchPageLoadWatcherTest() {}
-  ~AutoFetchPageLoadWatcherTest() override {}
-
-  void SetUp() override {
-    testing::Test::SetUp();
-
-    taco_.CreateRequestCoordinator();
-  }
+TEST_F(AutoFetchInternalImplTest, CompletedRequestUpdatesInProgressCount) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, TestURL()}});
+  impl_.RequestListInitialized(
+      std::vector<RequestInfo>{TestInfo(1, TestURL())});
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, OtherURL()}});
+  impl_.NavigationFrom(TestURL(), web_contents_);
 
-  RequestCoordinator* request_coordinator() {
-    return taco_.request_coordinator();
-  }
+  EXPECT_CALL(notifier_, InProgressCountChanged(0));
+  impl_.RequestRemoved(TestInfo(1, TestURL()));
+}
 
- private:
-  scoped_refptr<base::TestSimpleTaskRunner> task_runner_ =
-      base::MakeRefCounted<base::TestSimpleTaskRunner>();
-  base::ThreadTaskRunnerHandle handle_{task_runner_};
+TEST_F(AutoFetchInternalImplTest, NavigatingFromNotifiesTwoRequests) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, TestURL()}});
+  impl_.RequestListInitialized(
+      {TestInfo(1, TestURL()), TestInfo(2, TestURL())});
 
-  RequestCoordinatorStubTaco taco_;
-};
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, OtherURL()}});
+  impl_.NavigationFrom(TestURL(), web_contents_);
+  EXPECT_EQ(std::vector<int64_t>({1, 2}),
+            delegate_.set_notification_state_requests);
 
-// Simulate navigation to a URL prior to ObserverInitialize. Verify the
-// RemoveRequests is called.
-TEST_F(AutoFetchPageLoadWatcherTest, NavigateBeforeObserverInitialize) {
-  TestAutoFetchPageLoadWatcher tab_helper(request_coordinator());
-  tab_helper.HandlePageNavigation(TestURL());
-  std::vector<std::unique_ptr<SavePageRequest>> all_requests;
-  all_requests.push_back(std::make_unique<SavePageRequest>(TestRequest(1)));
-  all_requests.push_back(std::make_unique<SavePageRequest>(
-      TestRequest(2, GURL("http://different.com"))));
-  tab_helper.ObserverInitialize(std::move(all_requests));
-
-  std::vector<int64_t> expected_requests{1};
-  EXPECT_EQ(expected_requests, tab_helper.removed_ids());
+  EXPECT_CALL(notifier_, NotifyInProgress(2)).Times(2);
+  impl_.SetNotificationStateComplete(1, true);
+  impl_.SetNotificationStateComplete(2, true);
 }
 
-TEST_F(AutoFetchPageLoadWatcherTest, OnCompletedNoRequest) {
-  TestAutoFetchPageLoadWatcher tab_helper(request_coordinator());
-  tab_helper.ObserverInitialize({});
+TEST_F(AutoFetchInternalImplTest, NavigatingFromBeforeInitialization) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, OtherURL()}});
+  impl_.RequestListInitialized(
+      {TestInfo(1, TestURL()), TestInfo(2, TestURL())});
 
-  tab_helper.OnCompleted(TestRequest(1),
-                         RequestNotifier::BackgroundSavePageResult());
+  EXPECT_EQ(std::vector<int64_t>({1, 2}),
+            delegate_.set_notification_state_requests);
 
-  // Nothing happens, just verify there is no crash.
-  SUCCEED();
+  EXPECT_CALL(notifier_, NotifyInProgress(2)).Times(2);
+  impl_.SetNotificationStateComplete(1, true);
+  impl_.SetNotificationStateComplete(2, true);
 }
 
-TEST_F(AutoFetchPageLoadWatcherTest, OnCompletedOneRequestWithURL) {
-  TestAutoFetchPageLoadWatcher tab_helper(request_coordinator());
-  tab_helper.ObserverInitialize({});
-  std::map<GURL, std::vector<int64_t>>* requests =
-      tab_helper.live_auto_fetch_requests();
-
-  tab_helper.OnAdded(TestRequest(1));
-  ASSERT_EQ(1ul, requests->count(TestURL()));
+TEST_F(AutoFetchInternalImplTest, TabCloseNotifies) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId + 1, TestURL()}});
+  impl_.RequestListInitialized(
+      std::vector<RequestInfo>{TestInfo(1, TestURL())});
 
-  tab_helper.OnCompleted(TestRequest(1),
-                         RequestNotifier::BackgroundSavePageResult());
+  EXPECT_EQ(std::vector<int64_t>({1}),
+            delegate_.set_notification_state_requests);
 
-  EXPECT_EQ(0ul, requests->count(TestURL()));
+  EXPECT_CALL(notifier_, NotifyInProgress(1)).Times(1);
+  impl_.SetNotificationStateComplete(1, true);
 }
 
-// Verify multiple requests with the same URL are handled as expected.
-TEST_F(AutoFetchPageLoadWatcherTest, OnCompletedMultipleRequestsWithURL) {
-  TestAutoFetchPageLoadWatcher tab_helper(request_coordinator());
-  tab_helper.ObserverInitialize({});
+TEST_F(AutoFetchInternalImplTest, RequestRemovedWhileSettingNotificationState) {
+  tab_finder_->SetTabs(std::map<int, GURL>{{kDefaultTabId, OtherURL()}});
+  impl_.RequestListInitialized(
+      std::vector<RequestInfo>{TestInfo(1, TestURL())});
 
-  // Three requests with the same URL.
-  tab_helper.OnAdded(TestRequest(1));
-  tab_helper.OnAdded(TestRequest(2));
-  tab_helper.OnAdded(TestRequest(3));
+  EXPECT_EQ(std::vector<int64_t>({1}),
+            delegate_.set_notification_state_requests);
 
-  // Only one is completed.
-  tab_helper.OnCompleted(TestRequest(2),
-                         RequestNotifier::BackgroundSavePageResult());
+  impl_.RequestRemoved({TestInfo(1, TestURL())});
 
-  std::vector<int64_t> expected_requests = {1, 3};
-  EXPECT_EQ(expected_requests,
-            (*tab_helper.live_auto_fetch_requests())[TestURL()]);
+  EXPECT_CALL(notifier_, NotifyInProgress(1)).Times(0);
+  impl_.SetNotificationStateComplete(1, false);
 }
 
 }  // namespace

chrome/browser/offline_pages/android/offline_page_auto_fetcher_service.cc

@@ -7,12 +7,9 @@
 #include <string>
 #include <utility>
 
-#include "base/android/jni_android.h"
-#include "base/android/jni_string.h"
 #include "base/optional.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "chrome/browser/offline_pages/request_coordinator_factory.h"
 #include "components/offline_pages/core/auto_fetch.h"
@@ -77,7 +74,11 @@ OfflinePageAutoFetcherService::OfflinePageAutoFetcherService(
     RequestCoordinator* request_coordinator,
     OfflinePageModel* offline_page_model,
     Delegate* delegate)
-    : page_load_watcher_(request_coordinator),
+    : notifier_(std::make_unique<AutoFetchNotifier>()),
+      page_load_watcher_(
+          notifier_.get(),
+          request_coordinator,
+          std::make_unique<AutoFetchPageLoadWatcher::AndroidTabFinder>()),
       request_coordinator_(request_coordinator),
       offline_page_model_(offline_page_model),
       delegate_(delegate) {
@@ -159,6 +160,7 @@ void OfflinePageAutoFetcherService::TryScheduleStep2(
       return;
     }
   }
+
   // Finally, schedule a new request, and proceed to step 3.
   RequestCoordinator::SavePageLaterParams params;
   params.url = url;

chrome/browser/offline_pages/android/offline_page_auto_fetcher_service.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_OFFLINE_PAGES_ANDROID_OFFLINE_PAGE_AUTO_FETCHER_SERVICE_H_
 
 #include <memory>
+#include <queue>
 #include <utility>
 #include <vector>
 
@@ -146,6 +147,7 @@ class OfflinePageAutoFetcherService : public KeyedService,
 
   void AutoFetchComplete(const OfflinePageItem* page);
 
+  std::unique_ptr<AutoFetchNotifier> notifier_;
   AutoFetchPageLoadWatcher page_load_watcher_;
   RequestCoordinator* request_coordinator_;
   OfflinePageModel* offline_page_model_;