Field trial removing tiles from NTP if URL is already open - for 1993 clients

chrome/browser/history/top_sites.cc

Index: chrome/browser/history/top_sites.cc
diff --git a/chrome/browser/history/top_sites.cc b/chrome/browser/history/top_sites.cc
index 9de804e1f34059a89f8c8395b4aaa211dcafddda..5ca217ca7ecd237bdc33b8f40f0eb59bb374fa32 100644
--- a/chrome/browser/history/top_sites.cc
+++ b/chrome/browser/history/top_sites.cc
@@ -5,9 +5,13 @@
 #include "chrome/browser/history/top_sites.h"
 
 #include "base/metrics/field_trial.h"
+#include "base/metrics/histogram.h"
 #include "base/strings/string_util.h"
+#include "base/values.h"
 #include "chrome/browser/history/top_sites_impl.h"
 #include "chrome/browser/history/top_sites_likely_impl.h"
+#include "chrome/common/instant_types.h"
+#include "content/public/browser/web_contents.h"
 #include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 #include "grit/locale_settings.h"
@@ -27,6 +31,11 @@ const char kMostVisitedFieldTrialName[] = "MostVisitedTilePlacement";
 const char kOneEightGroupPrefix[] = "OneEight";
 const char kOneFourGroupPrefix[] = "OneFour";
 const char kFlippedSuffix[] = "Flipped";
+const char kTabsGroupName[] = "DontShowOpenTabs";
+
+// Minimum number of Most Visited suggestions required in order for the Most
+// Visited Field Trial to remove a URL already open in the browser.
+const size_t kMinUrlSuggestions = 8;
 
 }  // namespace
 
@@ -69,18 +78,105 @@ void TopSites::MaybeShuffle(MostVisitedURLList* data) {
   // tiles, or the 1st and 8th, or do nothing.
   if (EndsWith(group_name, kFlippedSuffix, true)) {
     size_t index_to_flip = 0;
-    if (StartsWithASCII(group_name, kOneEightGroupPrefix, true) &&
-        data->size() >= 8) {
-      index_to_flip = 7;
-    } else if (StartsWithASCII(group_name, kOneFourGroupPrefix, true) &&
-               data->size() >= 4) {
-      index_to_flip = 3;
+    if (StartsWithASCII(group_name, kOneEightGroupPrefix, true)) {
+      if (data->size() < 8) {
+        UMA_HISTOGRAM_ENUMERATION(
+            "NewTabPage.MostVisitedTilePlacementExperiment",
+            NTP_TILE_EXPERIMENT_ACTION_TOO_FEW_URLS_TILES_1_8,
+            NUM_NTP_TILE_EXPERIMENT_ACTIONS);
+      } else {
+        index_to_flip = 7;
+      }
+    } else if (StartsWithASCII(group_name, kOneFourGroupPrefix, true)) {
+      if (data->size() < 4) {
+        UMA_HISTOGRAM_ENUMERATION(
+            "NewTabPage.MostVisitedTilePlacementExperiment",
+            NTP_TILE_EXPERIMENT_ACTION_TOO_FEW_URLS_TILES_1_4,
+            NUM_NTP_TILE_EXPERIMENT_ACTIONS);
+      } else {
+        index_to_flip = 3;
+      }
     }
 
-    if (data->empty() || (*data)[index_to_flip].url.is_empty())
+    if (data->empty() || (*data)[index_to_flip].url.is_empty()) {
+      UMA_HISTOGRAM_ENUMERATION(
+            "NewTabPage.MostVisitedTilePlacementExperiment",
+            NTP_TILE_EXPERIMENT_ACTION_NO_URL_TO_FLIP,
+            NUM_NTP_TILE_EXPERIMENT_ACTIONS);
       return;
+    }
     std::swap((*data)[0], (*data)[index_to_flip]);
   }
 }
 
+// The following 3 functions are part of an experiment that removes recommended
+// Most Visited URLs if a matching URL is already open in the Browser. Note:
+// the experiment targets only the top-level of sites i.e. if www.foo.com/bar is
+// open in browser, and www.foo.com is a recommended URL, www.foo.com will still
+// appear on the next NTP open. The experiment will not remove a URL if doing so
+// would cause the number of Most Visited recommendations to drop below eight.

[beaudoin, 2013/06/20 21:55:44]

We do not use comments describing >1 function. Put the comment on one of the
function, preferably in the .h, and ensure the other 2 refer to it.

[annark1, 2013/07/04 18:29:15]

Done.

+
+// static
+bool TopSites::IsClientInTabsGroup() {
+  return base::FieldTrialList::FindFullName(kMostVisitedFieldTrialName) ==
+      kTabsGroupName;
+}
+
+// static
+void TopSites::RemoveItemsMatchingOpenTabs(
+    const std::set<std::string>& open_urls,
+    std::vector<InstantMostVisitedItem>* items) {
+  size_t i = 0;
+  while (i < items->size()) {
+    const std::string url = (*items)[i].url.spec();
+    if (open_urls.count(url) != 0) {
+      if (items->size() <= kMinUrlSuggestions) {
+        UMA_HISTOGRAM_ENUMERATION(
+            "NewTabPage.MostVisitedTilePlacementExperiment",
+            NTP_TILE_EXPERIMENT_ACTION_DID_NOT_REMOVE_URL,
+            NUM_NTP_TILE_EXPERIMENT_ACTIONS);
+        ++i;
+      } else {
+        items->erase(items->begin()+i);
+        UMA_HISTOGRAM_ENUMERATION(
+            "NewTabPage.MostVisitedTilePlacementExperiment",
+            NTP_TILE_EXPERIMENT_ACTION_REMOVED_URL,
+            NUM_NTP_TILE_EXPERIMENT_ACTIONS);
+      }
+    } else {
+      ++i;
+    }
+  }
+}
+
+// static
+void TopSites::RemovePageValuesMatchingOpenTabs(
+    const std::set<std::string>& open_urls,
+    base::ListValue* pages_value) {
+  size_t i = 0;
+  while (i < pages_value->GetSize()) {
+    base::DictionaryValue* page_value;
+    std::string url;
+    if (pages_value->GetDictionary(i, &page_value) &&
+        page_value->GetString("url", &url) &&
+        open_urls.count(url) != 0) {
+      if (pages_value->GetSize() <= kMinUrlSuggestions) {
+        UMA_HISTOGRAM_ENUMERATION(
+            "NewTabPage.MostVisitedTilePlacementExperiment",
+            NTP_TILE_EXPERIMENT_ACTION_DID_NOT_REMOVE_URL,
+            NUM_NTP_TILE_EXPERIMENT_ACTIONS);
+        ++i;
+      } else {
+        pages_value->Remove(*page_value, &i);
+        UMA_HISTOGRAM_ENUMERATION(
+            "NewTabPage.MostVisitedTilePlacementExperiment",
+            NTP_TILE_EXPERIMENT_ACTION_REMOVED_URL,
+            NUM_NTP_TILE_EXPERIMENT_ACTIONS);
+      }
+    } else {
+      ++i;
+    }
+  }
+}

[beaudoin, 2013/06/20 21:55:44]

These two functions are really quite similar. We could probably use some
Callback<> and base::Bind() to simplify this. (See doc in
http://src.chromium.org/viewvc/chrome/trunk/src/base/callback.h?content-type=...
)

I'm thinking of something like:

namespace {

const std::string& GetUrlFromVector(std::vector<InstantMostVisitedItem>* items,
size_t i) {
  return (*items)[i].url.spec();
}

const std::string& GetUrlFromPageValue(base::ListValue* pages_value, size_t i) {
  base::DictionaryValue* page_value;
  std::string url;
  if (!pages_value->GetDictionary(i, &page_value) ||
      !page_value->GetString("url", &url))
    return std::string();
  return url;
}

void RemoveUrlFromVector(base::ListValue* pages_value, size_t i) { ... }

void RemoveUrlFromPageValue(base::ListValue* pages_value, size_t i) { ... }


void RemoveUrlsMatchingOpenTabs(
    const std::set<std::string>& open_urls,
    size_t num_urls,
    const base::Callback<const std::string&(size_t)>& getUrl,
    const base::Callback<void(size_t)>& removeUrl) {

  // Your big method here, invoking the callbacks with Run();

}

}  // namespace

void TopSites::RemoveItemsMatchingOpenTabs(
    const std::set<std::string>& open_urls,
    std::vector<InstantMostVisitedItem>* items) {
  RemoveUrlsMatchingOpenTabs(open_urls, items->size(),
                             base::bind(&GetUrlFromVector, open_urls),
                             base::bind(&RemoveUrlFromVector, open_urls));
}

void TopSites::RemovePageValuesMatchingOpenTabs(...) { ... }


Not entirely sure it's worth it. You may want to see what the owner thinks but
if you have some free cycles it's a nice exercise. :)

[annark1, 2013/07/04 18:29:15]

brettw, do you have a preference on how this should be done? 


On 2013/06/20 21:55:44, beaudoin wrote:

+
 }  // namespace history