[Icons NTP] Use GetVariationParamValue instead of experiment prefix to determine when large icons a…

chrome/browser/favicon/favicon_tab_helper.cc

Index: chrome/browser/favicon/favicon_tab_helper.cc
diff --git a/chrome/browser/favicon/favicon_tab_helper.cc b/chrome/browser/favicon/favicon_tab_helper.cc
index 80940039538ea403011162e71effdb0e8acd1c79..6d9c9dc3fb5b2879f826e1fa8ceb065a86075afc 100644
--- a/chrome/browser/favicon/favicon_tab_helper.cc
+++ b/chrome/browser/favicon/favicon_tab_helper.cc
@@ -4,7 +4,6 @@
 
 #include "chrome/browser/favicon/favicon_tab_helper.h"
 
-#include "base/metrics/field_trial.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/chrome_favicon_client.h"
@@ -21,6 +20,7 @@
 #include "components/favicon/core/favicon_tab_helper_observer.h"
 #include "components/favicon_base/favicon_types.h"
 #include "components/history/core/browser/history_service.h"
+#include "components/variations/variations_associated_data.h"
 #include "content/public/browser/favicon_status.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/navigation_controller.h"
@@ -47,8 +47,7 @@ namespace {
 
 // Returns whether icon NTP is enabled.
 bool IsIconNTPEnabled() {

[kmadhusu, 2015/03/26 16:25:11]

This helper function is used by both browser and renderer code. Do you want to
move this to a common place (instant_types.cc)?

[beaudoin, 2015/03/26 16:50:08]

Agreed it's not optimal, but I'd propose leaving it like this since it's meant
to be temporary code that will be removed when the experiment launches. Also, I
can't find a good place to put it in common/ (I don't like instant_types since
it contains only types at the moment, and it's not an instant feature per-se.)

Are you OK with this?

[kmadhusu, 2015/03/26 17:10:25]

I am fine with the current state. I suggested instant_types.cc because it
already has "NTP" related code :).

-  return StartsWithASCII(base::FieldTrialList::FindFullName("IconNTP"),
-                         "Enabled", true);
+  return variations::GetVariationParamValue("IconNTP", "state") == "enabled";

[kmadhusu, 2015/03/26 16:25:11]

You are checking for "enabled" instead of "Enabled" state. Is this intentional? 

[beaudoin, 2015/03/26 16:50:10]

Yes, this seems to be the pattern used by other Finch experiments that rely on
the "state" parameters to trigger.

[kmadhusu, 2015/03/26 17:10:25]

Acknowledged.

 }
 
 }  // namespace