Add SiteIsolation.IsolateExtensions metrics and tests.

chrome/browser/site_details.cc

Index: chrome/browser/site_details.cc
diff --git a/chrome/browser/site_details.cc b/chrome/browser/site_details.cc
index af38bc525c30bc9d2fb1079ada273103ad23c29c..2675fc7df6e31da44d0fc58540bb7d4f9a69627d 100644
--- a/chrome/browser/site_details.cc
+++ b/chrome/browser/site_details.cc
@@ -7,13 +7,50 @@
 #include "base/metrics/histogram.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
+#include "extensions/common/constants.h"
 
 using content::BrowserThread;
 using content::RenderProcessHost;
 using content::SiteInstance;
 using content::WebContents;
 
-SiteData::SiteData() {}
+namespace {
+
+bool ShouldIsolate(IsolationScenarioType policy, const GURL& site) {
+  switch (policy) {
+    case ISOLATE_ALL_SITES:
+      return true;
+    case ISOLATE_HTTPS_SITES:
+      // Note: For estimation purposes "isolate https sites" is really
+      // implemented as "isolate non-http sites". This means that, for example,
+      // the New Tab Page gets counted as two processes under this policy, and
+      // extensions are isolated as well.
+      return !site.SchemeIs(url::kHttpScheme);
+    case ISOLATE_EXTENSIONS:
+      return site.SchemeIs(extensions::kExtensionScheme);
+  }
+  NOTREACHED();
+  return true;
+}
+
+}  // namespace
+
+IsolationScenario::IsolationScenario() : policy(ISOLATE_ALL_SITES) {}
+
+IsolationScenario::~IsolationScenario() {}
+
+void IsolationScenario::CollectSiteInfoForScenario(SiteInstance* primary,
+                                                   const GURL& site) {
+  const GURL& isolated = ShouldIsolate(policy, site) ? site : GURL("http://");
+  sites.insert(isolated);
+  browsing_instance_site_map[primary->GetId()].insert(isolated);
+}
+
+SiteData::SiteData() {
+  scenarios[ISOLATE_ALL_SITES].policy = ISOLATE_ALL_SITES;
+  scenarios[ISOLATE_HTTPS_SITES].policy = ISOLATE_HTTPS_SITES;
+  scenarios[ISOLATE_EXTENSIONS].policy = ISOLATE_EXTENSIONS;
+}
 
 SiteData::~SiteData() {}
 
@@ -30,9 +67,9 @@ void SiteDetails::CollectSiteInfo(WebContents* contents,
   // the "primary" SiteInstances of each BrowsingInstance we've seen so far.
   SiteInstance* instance = contents->GetSiteInstance();
   SiteInstance* primary = NULL;
-  for (size_t i = 0; i < site_data->instances.size(); ++i) {
-    if (instance->IsRelatedSiteInstance(site_data->instances[i])) {
-      primary = site_data->instances[i];
+  for (SiteInstance* already_collected_instance : site_data->instances) {
+    if (instance->IsRelatedSiteInstance(already_collected_instance)) {
+      primary = already_collected_instance;
       break;
     }
   }
@@ -44,29 +81,19 @@ void SiteDetails::CollectSiteInfo(WebContents* contents,
 
   // Now keep track of how many sites we have in this BrowsingInstance (and
   // overall), including sites in iframes.
-  std::set<GURL> sites_in_tab = contents->GetSitesInTab();
-  for (std::set<GURL>::iterator iter = sites_in_tab.begin();
-       iter != sites_in_tab.end(); ++iter) {
-    // Skip about:blank, since we won't usually give it its own process.
-    // Because about:blank has no host, its site URL will be blank.
-    if (iter->is_empty())
-      continue;
-
-    // Make sure we don't overcount process-per-site sites, like the NTP.
-    if (RenderProcessHost::ShouldUseProcessPerSite(browser_context, *iter) &&
-        site_data->sites.find(*iter) != site_data->sites.end()) {
+  for (const GURL& site : contents->GetSitesInTab()) {
+    // Make sure we don't overcount process-per-site sites, like the NTP or
+    // extensions, by skipping over them if they're already logged for
+    // ISOLATE_ALL_SITES.
+    if (RenderProcessHost::ShouldUseProcessPerSite(browser_context, site) &&
+        site_data->scenarios[ISOLATE_ALL_SITES].sites.find(site) !=
+            site_data->scenarios[ISOLATE_ALL_SITES].sites.end()) {
       continue;
     }
 
-    site_data->sites.insert(*iter);
-    site_data->instance_site_map[primary->GetId()].insert(*iter);
-
-    // Also keep track of how things would look if we only isolated HTTPS sites.
-    // In this model, all HTTP sites are grouped into one "http://" site.  HTTPS
-    // and other schemes (e.g., chrome:) are still isolated.
-    GURL https_site = iter->SchemeIs("http") ? GURL("http://") : *iter;
-    site_data->https_sites.insert(https_site);
-    site_data->instance_https_site_map[primary->GetId()].insert(https_site);
+    for (IsolationScenario& scenario : site_data->scenarios) {

[nasko, 2015/09/04 22:19:43]

nit: no need for {}

[ncarter (slow), 2015/09/10 19:08:58]

The style guide allows both ways, right? I usually prefer the {'s for loops,
especially like here when we're at the end of an outer block.

[nasko, 2015/09/11 18:14:11]

Acknowledged.

+      scenario.CollectSiteInfoForScenario(primary, site);
+    }
   }
 }
 
@@ -78,39 +105,33 @@ void SiteDetails::UpdateHistograms(
   int process_limit = RenderProcessHost::GetMaxRendererProcessCount();
 
   // Sum the number of sites and SiteInstances in each BrowserContext.
-  int num_sites = 0;
-  int num_https_sites = 0;
+  int num_sites[ISOLATION_SCENARIO_LAST + 1] = {};
+  int num_isolated_site_instances[ISOLATION_SCENARIO_LAST + 1] = {};
   int num_browsing_instances = 0;
-  int num_isolated_site_instances = 0;
-  int num_isolated_https_site_instances = 0;
   for (BrowserContextSiteDataMap::const_iterator i = site_data_map.begin();
        i != site_data_map.end(); ++i) {
-    num_sites += i->second.sites.size();
-    num_https_sites += i->second.https_sites.size();
-    num_browsing_instances += i->second.instance_site_map.size();
-    for (BrowsingInstanceSiteMap::const_iterator iter =
-             i->second.instance_site_map.begin();
-         iter != i->second.instance_site_map.end(); ++iter) {
-      num_isolated_site_instances += iter->second.size();
-    }
-    for (BrowsingInstanceSiteMap::const_iterator iter =
-             i->second.instance_https_site_map.begin();
-         iter != i->second.instance_https_site_map.end(); ++iter) {
-      num_isolated_https_site_instances += iter->second.size();
+    for (const IsolationScenario& scenario : i->second.scenarios) {
+      num_sites[scenario.policy] += scenario.sites.size();
+      for (auto& browsing_instance : scenario.browsing_instance_site_map) {
+        num_isolated_site_instances[scenario.policy] +=
+            browsing_instance.second.size();
+      }
     }
+    num_browsing_instances += i->second.scenarios[ISOLATE_ALL_SITES]
+                                  .browsing_instance_site_map.size();
   }
 
-  // Predict the number of processes needed when isolating all sites and when
-  // isolating only HTTPS sites.
-  int process_count_lower_bound = num_sites;
-  int process_count_upper_bound = num_sites + process_limit - 1;
-  int process_count_estimate = std::min(
-      num_isolated_site_instances, process_count_upper_bound);
-
-  int process_count_https_lower_bound = num_https_sites;
-  int process_count_https_upper_bound = num_https_sites + process_limit - 1;
-  int process_count_https_estimate = std::min(
-      num_isolated_https_site_instances, process_count_https_upper_bound);
+  // Predict the number of processes needed when isolating all sites, when
+  // isolating only HTTPS sites, and when isolating

[nasko, 2015/09/04 22:19:43]

This sentence doesn't seem complete. Did you miss "extensions"?

[ncarter (slow), 2015/09/10 19:08:58]

Done.

+  int process_count_lower_bound[ISOLATION_SCENARIO_LAST + 1];
+  int process_count_upper_bound[ISOLATION_SCENARIO_LAST + 1];
+  int process_count_estimate[ISOLATION_SCENARIO_LAST + 1];
+  for (int policy = 0; policy <= ISOLATION_SCENARIO_LAST; policy++) {
+    process_count_lower_bound[policy] = num_sites[policy];
+    process_count_upper_bound[policy] = num_sites[policy] + process_limit - 1;
+    process_count_estimate[policy] = std::min(
+        num_isolated_site_instances[policy], process_count_upper_bound[policy]);
+  }
 
   // Just renderer process count:
   UMA_HISTOGRAM_COUNTS_100("SiteIsolation.CurrentRendererProcessCount",
@@ -118,30 +139,40 @@ void SiteDetails::UpdateHistograms(
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.BrowsingInstanceCount",
       num_browsing_instances);
-  UMA_HISTOGRAM_COUNTS_100(
-      "SiteIsolation.IsolateAllSitesProcessCountNoLimit",
-      num_isolated_site_instances);
+  UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateAllSitesProcessCountNoLimit",
+                           num_isolated_site_instances[ISOLATE_ALL_SITES]);
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateAllSitesProcessCountLowerBound",
-      process_count_lower_bound);
-  UMA_HISTOGRAM_COUNTS_100(
-      "SiteIsolation.IsolateAllSitesProcessCountEstimate",
-      process_count_estimate);
-  UMA_HISTOGRAM_COUNTS_100(
-      "SiteIsolation.IsolateHttpsSitesProcessCountNoLimit",
-      num_isolated_https_site_instances);
+      process_count_lower_bound[ISOLATE_ALL_SITES]);
+  UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateAllSitesProcessCountEstimate",
+                           process_count_estimate[ISOLATE_ALL_SITES]);
+
+  UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateHttpsSitesProcessCountNoLimit",
+                           num_isolated_site_instances[ISOLATE_HTTPS_SITES]);
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateHttpsSitesProcessCountLowerBound",
-      process_count_https_lower_bound);
+      process_count_lower_bound[ISOLATE_HTTPS_SITES]);
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateHttpsSitesProcessCountEstimate",
-      process_count_https_estimate);
+      process_count_estimate[ISOLATE_HTTPS_SITES]);
+
+  UMA_HISTOGRAM_COUNTS_100("SiteIsolation.IsolateExtensionsProcessCountNoLimit",
+                           num_isolated_site_instances[ISOLATE_EXTENSIONS]);
+  UMA_HISTOGRAM_COUNTS_100(
+      "SiteIsolation.IsolateExtensionsProcessCountLowerBound",
+      process_count_lower_bound[ISOLATE_EXTENSIONS]);
+  UMA_HISTOGRAM_COUNTS_100(
+      "SiteIsolation.IsolateExtensionsProcessCountEstimate",
+      process_count_estimate[ISOLATE_EXTENSIONS]);
 
   // Total process count:
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateAllSitesTotalProcessCountEstimate",
-      process_count_estimate + non_renderer_process_count);
+      process_count_estimate[ISOLATE_ALL_SITES] + non_renderer_process_count);
   UMA_HISTOGRAM_COUNTS_100(
       "SiteIsolation.IsolateHttpsSitesTotalProcessCountEstimate",
-      process_count_https_estimate + non_renderer_process_count);
+      process_count_estimate[ISOLATE_HTTPS_SITES] + non_renderer_process_count);
+  UMA_HISTOGRAM_COUNTS_100(
+      "SiteIsolation.IsolateExtensionsTotalProcessCountEstimate",
+      process_count_estimate[ISOLATE_EXTENSIONS] + non_renderer_process_count);
 }