[TooManyTabs] Add TabNavigationThrottle

chrome/browser/resource_coordinator/tab_manager.cc

Index: chrome/browser/resource_coordinator/tab_manager.cc
diff --git a/chrome/browser/resource_coordinator/tab_manager.cc b/chrome/browser/resource_coordinator/tab_manager.cc
index c2b5150b795c23e21d1e97cbe43d51ba2ba9316e..1612e2f3cb7c89535d0d317feb3cf3dc60109a1d 100644
--- a/chrome/browser/resource_coordinator/tab_manager.cc
+++ b/chrome/browser/resource_coordinator/tab_manager.cc
@@ -49,6 +49,7 @@
 #include "components/variations/variations_associated_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/page_importance_signals.h"
@@ -111,6 +112,7 @@ TabManager::TabManager()
 #endif
       browser_tab_strip_tracker_(this, nullptr, nullptr),
       test_tick_clock_(nullptr),
+      loading_contents_(nullptr),
       weak_ptr_factory_(this) {
 #if defined(OS_CHROMEOS)
   delegate_.reset(new TabManagerDelegate(weak_ptr_factory_.GetWeakPtr()));
@@ -786,6 +788,8 @@ void TabManager::ActiveTabChanged(content::WebContents* old_contents,
     GetWebContentsData(old_contents)
         ->set_time_to_purge(GetTimeToPurge(min_time_to_purge_));
   }
+
+  ResumeTabNavigationIfNeeded(new_contents);
 }
 
 void TabManager::TabInsertedAt(TabStripModel* tab_strip_model,
@@ -875,4 +879,67 @@ bool TabManager::CanOnlyDiscardOnce() const {
 #endif
 }
 
+content::NavigationThrottle::ThrottleCheckResult
+TabManager::MaybeThrottleNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (!ShouldThrottleNavigation(navigation_handle)) {
+    loading_contents_ = navigation_handle->GetWebContents();
+    return content::NavigationThrottle::PROCEED;
+  }
+
+  pending_navigations_.push_back(navigation_handle);
+  return content::NavigationThrottle::DEFER;
+}
+
+bool TabManager::ShouldThrottleNavigation(
+    content::NavigationHandle* navigation_handle) const {
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kStaggeredBackgroundTabOpen)) {

[Charlie Reis, 2017/06/10 00:53:36]

We shouldn't be creating the throttle if this flag isn't present.

[Zhen Wang, 2017/06/13 23:33:20]

Done.

+    return false;
+  }
+
+  if (!navigation_handle)

[Charlie Reis, 2017/06/10 00:53:36]

Is this possible?

[Zhen Wang, 2017/06/13 23:33:20]

I am not sure if this is ever possible. That's why I check it here. Should I use
a DCHECK?

[Zhen Wang, 2017/06/19 23:00:12]

Ping

[Charlie Reis, 2017/07/06 23:50:30]

Let's remove it if we don't know that it's needed.  Adding unnecessary null
checks makes it harder to reason about how the code is meant to be used.

[Zhen Wang, 2017/07/07 18:06:38]

Done.

+    return false;
+
+  // It only throttle background tabs. Never throttle foreground tabs.
+  if (navigation_handle->GetWebContents()->IsVisible())

[Charlie Reis, 2017/06/10 00:53:36]

This also seems like it could be checked before creating the throttle.

[Zhen Wang, 2017/06/13 23:33:20]

Done.

+    return false;
+
+  // Do not throttle the navigation if no tab is currently loading.
+  if (!loading_contents_) {

[nasko, 2017/06/12 23:22:02]

Wouldn't it be simpler to just check the pending_navigations_ size for 0? Or is
the goal to keep background WebContents delayed until they are made visible? If
we just delay them, so that we don't overload the system, but then resume them
once we have resources, we might not need to keep track of the currently loading
WebContents.

[Zhen Wang, 2017/06/13 23:33:20]

|pending_navigations_| tracks which tabs has not sent out requests.
|loading_contents_| is the tab which has sent out request and started loading.

There could be a case where |pending_navigations_| is empty and there is some
tab loading. In this case, we want to delay the next tab's navigation.

A smarter way is to allow loading the next background tab when all loading tabs
have finished their work. I added a TODO to |loading_contents_| in the header
file. This CL tries to focus on getting the navigation part correct. We will
have follow-up CLs to make the algorithm smarter.

+    return false;
+  }
+
+  return true;
+}
+
+void TabManager::HasFinishedLoadingTab(content::WebContents* contents) {
+  loading_contents_ = nullptr;
+
+  while (!pending_navigations_.empty()) {
+    content::NavigationHandle* navigation_handle = pending_navigations_.front();
+    pending_navigations_.erase(pending_navigations_.begin());
+
+    if (navigation_handle) {
+      loading_contents_ = navigation_handle->GetWebContents();
+      navigation_handle->Resume();
+      break;
+    }
+  }
+}
+
+void TabManager::ResumeTabNavigationIfNeeded(content::WebContents* contents) {
+  auto it = pending_navigations_.begin();
+  while (it != pending_navigations_.end()) {
+    content::NavigationHandle* navigation_handle = *it;
+    if (navigation_handle && navigation_handle->GetWebContents() == contents) {

[nasko, 2017/06/12 23:22:02]

We better ensure we don't put nullptr into the pending_navigations_ list. It is
much easier to skip those null checks.

[Zhen Wang, 2017/06/13 23:33:20]

When a navigation gets cancelled, will the corresponding navigation handle be
deleted? Or should I explicitly track that when the NavigationThrottle is
destructed and notify TabManager to clean up?

[nasko, 2017/06/15 20:01:58]

The WebContentsObserver::DidFinishNavigation will be dispatched, indicating that
the navigation didn't commit. Since NavigationHandle owns all the throttles,
they will be destroyed after the DidFinishNavigation notification is delivered.
Your code should be equipped to handle that case. You already listen to
DidStartNavigation, so I would suggest adding a DidFinishNavigation
implementation that removes the handle from the pending_navigations_ list. The
implementation might do various checks based on success/failure of the
navigation, but unconditionally should remove the handle from the list.

[Zhen Wang, 2017/06/19 23:00:12]

Thanks! Now observing DidFinishNavigation.

+      loading_contents_ = navigation_handle->GetWebContents();

[Charlie Reis, 2017/06/10 00:53:36]

Keep in mind that there may have been an existing loading tab, and we aren't
keeping track of it after this.  That means that the next deferred tab will
start loading as soon as |contents| finishes, even if the previous
|loading_contents_| is still loading (for an arbitrary number of previous tabs,
if the user cycles through a few tabs).

We'd obviously want to stop deferring any tab the user visits, but it does mean
that we'll start loading additional background tabs earlier than expected. 
Maybe that's ok, for the sake of simplicity?

[Zhen Wang, 2017/06/13 23:33:20]

You are right. I think the best way is to know how many tabs are in loading
state. And do not start loading any background tab until all loading tabs are
finished.

We are currently enhancing TabManager to know the loading states of a tab
(https://crrev.com/2936003002/). This CL tries to just use some simple algorithm
and focus on navigation part. We will have follow-up CLs to make the algorithm
smarter. I added a TODO to loading_contents_ in the header file.

+      navigation_handle->Resume();
+      pending_navigations_.erase(it);
+      break;
+    }
+    it++;
+  }
+}
+
 }  // namespace resource_coordinator