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

chrome/browser/favicon/favicon_tab_helper.cc

 // Copyright (c) 2012 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/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"
 #include "chrome/browser/favicon/chrome_favicon_client_factory.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/url_constants.h"
 #include "components/favicon/content/favicon_url_util.h"
 #include "components/favicon/core/favicon_handler.h"
 #include "components/favicon/core/favicon_service.h"
 #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"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_delegate.h"
 #include "content/public/common/favicon_url.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_rep.h"
 
 using content::FaviconStatus;
 using content::NavigationController;
 using content::NavigationEntry;
 using content::WebContents;
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(FaviconTabHelper);
 
 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
 
 FaviconTabHelper::FaviconTabHelper(WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())) {
   client_ = ChromeFaviconClientFactory::GetForProfile(profile_);
 #if defined(OS_ANDROID) || defined(OS_IOS)
   bool download_largest_icon = true;
@@ skipping 254 matching lines @@
       id, image_url, bitmaps, original_bitmap_sizes);
   if (touch_icon_handler_.get()) {
     touch_icon_handler_->OnDidDownloadFavicon(
         id, image_url, bitmaps, original_bitmap_sizes);
   }
   if (large_icon_handler_.get()) {
     large_icon_handler_->OnDidDownloadFavicon(
         id, image_url, bitmaps, original_bitmap_sizes);
   }
 }