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

chrome/browser/ui/search/instant_controller.cc

Index: chrome/browser/ui/search/instant_controller.cc
diff --git a/chrome/browser/ui/search/instant_controller.cc b/chrome/browser/ui/search/instant_controller.cc
index 6203445396b17b0b82f1642480a14a91f19d87be..afae94db244bf33222c90b5d2578f2d62d8ae14d 100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -7,9 +7,15 @@
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/autocomplete/autocomplete_provider.h"
+#include "chrome/browser/autocomplete/autocomplete_result.h"
+#include "chrome/browser/autocomplete/search_provider.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/content_settings/content_settings_provider.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
+#include "chrome/browser/history/most_visited_tiles_experiment.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/instant_service.h"
@@ -22,6 +28,7 @@
 #include "chrome/browser/ui/search/instant_ntp.h"
 #include "chrome/browser/ui/search/instant_tab.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/content_settings_types.h"
 #include "chrome/common/pref_names.h"
@@ -112,6 +119,17 @@ void DeletePageSoon(scoped_ptr<T> page) {
   base::MessageLoop::current()->DeleteSoon(FROM_HERE, page.release());
 }
 
+// Creates a set containing the canonical URLs of the currently open tabs.
+void GetOpenUrls(const TabStripModel& tabs,
+                 const history::TopSites& top_sites,
+                 std::set<std::string>* urls) {
+  for (int i = 0; i < tabs.count(); ++i) {
+    content::WebContents* web_contents = tabs.GetWebContentsAt(i);
+    if (web_contents)
+      urls->insert(top_sites.GetCanonicalURLString(web_contents->GetURL()));
+  }
+}
+
 }  // namespace
 
 InstantController::InstantController(BrowserInstantController* browser,
@@ -318,10 +336,13 @@ void InstantController::ClearDebugEvents() {
 
 void InstantController::MostVisitedItemsChanged(
     const std::vector<InstantMostVisitedItem>& items) {
+  std::vector<InstantMostVisitedItem> items_copy(items);
+  MaybeRemoveMostVisitedItems(&items_copy);
+
   if (ntp_)
-    ntp_->sender()->SendMostVisitedItems(items);
+    ntp_->sender()->SendMostVisitedItems(items_copy);
   if (instant_tab_)
-    instant_tab_->sender()->SendMostVisitedItems(items);
+    instant_tab_->sender()->SendMostVisitedItems(items_copy);
 
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_INSTANT_SENT_MOST_VISITED_ITEMS,
@@ -674,3 +695,27 @@ bool InstantController::InStartup() const {
 InstantService* InstantController::GetInstantService() const {
   return InstantServiceFactory::GetForProfile(profile());
 }
+
+void InstantController::MaybeRemoveMostVisitedItems(
+    std::vector<InstantMostVisitedItem>* items) {
+// The following #if is due to the facts that tabstripmodel cannot be accessed
+// in the same way, that chrome::FindBrowserWithWebContents is undefined in
+// Android and, moreover, that this experiment is not designed to run on Android
+// devices due to different NTP presentation.
+#if !defined(OS_ANDROID)
+  if (!history::MostVisitedTilesExperiment::IsDontShowOpenURLsEnabled())

[Alexei Svitkine (slow), 2013/07/22 21:14:10]

This function seems to have identical logic between this impl and the one in
MostVisitedHandler::MaybeRemovePageValues().

Can you refactor them to share the implementation? Perhaps putting the new
function on MostVisitedTilesExperiment.

You could pass in tab_strip_model and top_sites, which is the difference between
the two functions in how they're obtained.

Then, you also don't need to duplicate GetOpenUrls().

[annark1, 2013/07/23 19:52:21]

That's how I had originally tried to do this, but it turned out it's not
possible because I can't include the UI component tab_strip_model in history/*,
where the experiment logic is, which is there due to the different places the
code is for 1993 and pre-1993 clients.
On 2013/07/22 21:14:10, Alexei Svitkine wrote:

[Alexei Svitkine (slow), 2013/07/23 20:10:51]

Then it sounds like the helper function should be in a file somewhere under ui/.

[annark1, 2013/07/24 01:23:58]

I was able to consolidate the two GetOpenURLs functions into
tab_strip_model_utils.cc. I've discussed with Philippe for the rest of the two
functions that are so similar, and given the fact that they don't share an
interface, we don't see how I can combine them any further.

On 2013/07/23 20:10:51, Alexei Svitkine wrote:

+    return;
+
+  const TabStripModel* tab_strip_model = browser_->tab_strip_model();
+  history::TopSites* top_sites = browser_->profile()->GetTopSites();
+  if (!tab_strip_model || !top_sites) {
+    NOTREACHED();
+    return;
+  }
+
+  std::set<std::string> open_urls;
+  GetOpenUrls(*tab_strip_model, *top_sites, &open_urls);
+  history::MostVisitedTilesExperiment::RemoveItemsMatchingOpenTabs(
+      open_urls, items);
+#endif
+}