[TooManyTabs] Add TabNavigationThrottle

chrome/browser/resource_coordinator/tab_navigation_throttle.cc

Index: chrome/browser/resource_coordinator/tab_navigation_throttle.cc
diff --git a/chrome/browser/resource_coordinator/tab_navigation_throttle.cc b/chrome/browser/resource_coordinator/tab_navigation_throttle.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ed0dd949b8e1bc706a42d8f616c77f243538e06d
--- /dev/null
+++ b/chrome/browser/resource_coordinator/tab_navigation_throttle.cc
@@ -0,0 +1,56 @@
+// Copyright 2017 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/resource_coordinator/tab_navigation_throttle.h"
+
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/resource_coordinator/tab_manager.h"
+#include "chrome/browser/search/search.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/web_contents.h"
+
+namespace resource_coordinator {
+
+// static
+std::unique_ptr<TabNavigationThrottle>
+TabNavigationThrottle::MaybeCreateThrottleFor(
+    content::NavigationHandle* navigation_handle) {
+  // Only consider throttling common URLs, i.e., HTTP/HTTPS and common
+  // chrome:// UI URLs, for main frames.

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

I'm curious, what's the motivation for this?

We've found that SchemeIsHTTPOrHTTPS is often the wrong thing to be asking-- it
excludes a lot of related cases like filesystem: and blob: URLs, view-source:,
file:, etc.  What sort of things are you intending to exclude, and why?

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

My actual intention is to exclude all non-tab navigations, e.g., extension. But
I am not sure if filtering extensions:// is enough. Do you know what is the
correct filter for tab navigation?

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

Ping.

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

I'm not sure I understand what you mean.  chrome-extension:// URLs can load in
normal tabs as well, such as Options pages. 
 Conversely, I think you can also have SharedWorkers from HTTPS URLs that load
in background WebContents and aren't in normal tabs.  A URL check is not the
right way to determine if it's loading in a normal tab.

Someone with more knowledge of the chrome/ layer can probably tell you the right
way to determine that.  I think there might be some UserData you could look for?
 (Maybe ask sky@ or avi@?)

+  if (!navigation_handle->IsInMainFrame() ||
+      (!navigation_handle->GetURL().SchemeIsHTTPOrHTTPS() &&
+       !TabManager::IsInternalPage(navigation_handle->GetURL()))) {
+    return nullptr;
+  }
+
+  // Do not delay the NTP, which in some cases has an HTTPS URL.
+  if (search::IsNTPURL(
+          navigation_handle->GetURL(),
+          Profile::FromBrowserContext(
+              navigation_handle->GetWebContents()->GetBrowserContext()))) {
+    return nullptr;
+  }
+
+  return base::MakeUnique<TabNavigationThrottle>(navigation_handle);

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

It seems like we're creating this throttle in a lot more cases than necessary. 
Right now it looks like almost all main frame navigations (with a few exceptions
based on destination URL), but my impression is that it should only be for the
first navigation in a newly created tab, in a previously unused SiteInstance,
with no opener.  That was my understanding from the design doc, anyway.

Would it make sense to narrow down to some of these criteria, using
navigation_handle->GetWebContents()?  You could ask the WebContents about
GetOpener() and GetController().IsInitialNavigation().  We might be able to
check SiteInstanceImpl::active_frame_count() as well.

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

Thanks for the pointers! Added restriction on first navigation and opener case.

Why do we check SiteInstanceImpl::active_frame_count()? What is this use case?

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

SiteInstanceImpl::active_frame_count() says how many frames might be able to
reach and script this page (i.e., are in the same process and could have
references to each other).  It would be similar to checking the opener of the
WebContents, but covers more scenarios (e.g., frames that could find each other
by name).  If it's easy to add to the list of checks, maybe alongside the
HasOpener() call, I'd suggest including it.

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

active_frame_count is not exposed as a public content API, i.e., it is on
content::SiteInstanceImpl, but not content::SiteInstance. Can I actually use it
in //chrome/browser?

[Charlie Reis, 2017/07/07 22:07:55]

Ah, I'd forgotten we're not in content here.  Nevermind, we can skip it.

+}
+
+const char* TabNavigationThrottle::GetNameForLogging() {
+  return "TabNavigationThrottle";
+}
+
+TabNavigationThrottle::TabNavigationThrottle(
+    content::NavigationHandle* navigation_handle)
+    : content::NavigationThrottle(navigation_handle) {}
+
+TabNavigationThrottle::~TabNavigationThrottle() {}
+
+content::NavigationThrottle::ThrottleCheckResult
+TabNavigationThrottle::WillStartRequest() {
+  return g_browser_process->GetTabManager()->MaybeThrottleNavigation(
+      navigation_handle());
+}
+
+}  // namespace resource_coordinator