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

[Icons NTP] Use GetVariationParamValue instead of experiment prefix to determine when large icons are enabled.

This is the standard pattern for adding a forcing flags in about://flags.

This also matches a google3 change that just landed which declares that parameter.

BUG=467712
Committed: https://crrev.com/accd94964315b34fe738166b173992477965b88c
Cr-Commit-Position: refs/heads/master@{#322456}

[beaudoin, 2015-03-25 21:24:03]

beaudoin@chromium.org changed reviewers:
+ huangs@chromium.org, pkotwicz@chromium.org

[beaudoin, 2015-03-25 21:24:03]

beaudoin@chromium.org changed required reviewers:
+ pkotwicz@chromium.org

[pkotwicz1, 2015-03-25 22:25:41]

pkotwicz@google.com changed reviewers:
+ pkotwicz@google.com

[pkotwicz1, 2015-03-25 22:25:42]

LGTM with nit

https://codereview.chromium.org/1032073003/diff/1/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1032073003/diff/1/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper.cc:7: #include
"base/metrics/field_trial.h"
You can remove this include?

[beaudoin, 2015-03-26 14:07:08]

https://codereview.chromium.org/1032073003/diff/1/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1032073003/diff/1/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper.cc:7: #include
"base/metrics/field_trial.h"
On 2015/03/25 22:25:41, pkotwicz22 wrote:
> You can remove this include?

Good catch! Thanks.

Done.

[huangs, 2015-03-26 14:53:40]

Also change
src/chrome/browser/search/local_ntp_source.cc
src/chrome/renderer/searchbox/searchbox_extension.cc

?

[beaudoin, 2015-03-26 15:13:54]

Why I couldn't find these files by searching is beyond me. Thanks for pointing
them out. PTAL.

[beaudoin, 2015-03-26 15:15:01]

beaudoin@chromium.org changed reviewers:
+ kmadhusu@chromium.org

[beaudoin, 2015-03-26 15:15:01]

beaudoin@chromium.org changed required reviewers:
+ kmadhusu@chromium.org

[beaudoin, 2015-03-26 15:15:20]

R+kmadhusu for OWNER on the two new files.

[huangs, 2015-03-26 15:16:46]

lgtm

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

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.cc:49: bool IsIconNTPEnabled() {
This helper function is used by both browser and renderer code. Do you want to
move this to a common place (instant_types.cc)?

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.cc:50: return
variations::GetVariationParamValue("IconNTP", "state") == "enabled";
You are checking for "enabled" instead of "Enabled" state. Is this intentional?

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

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.cc:49: bool IsIconNTPEnabled() {
On 2015/03/26 16:25:11, kmadhusu wrote:
> This helper function is used by both browser and renderer code. Do you want to
> move this to a common place (instant_types.cc)?

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?

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.cc:50: return
variations::GetVariationParamValue("IconNTP", "state") == "enabled";
On 2015/03/26 16:25:11, kmadhusu wrote:
> You are checking for "enabled" instead of "Enabled" state. Is this
intentional? 

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]

lgtm

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.cc:49: bool IsIconNTPEnabled() {
On 2015/03/26 16:50:08, beaudoin wrote:
> On 2015/03/26 16:25:11, kmadhusu wrote:
> > This helper function is used by both browser and renderer code. Do you want
to
> > move this to a common place (instant_types.cc)?
> 
> 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?
> 
> 

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

https://codereview.chromium.org/1032073003/diff/40001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.cc:50: return
variations::GetVariationParamValue("IconNTP", "state") == "enabled";
On 2015/03/26 16:50:10, beaudoin wrote:
> On 2015/03/26 16:25:11, kmadhusu wrote:
> > You are checking for "enabled" instead of "Enabled" state. Is this
> intentional? 
> 
> Yes, this seems to be the pattern used by other Finch experiments that rely on
> the "state" parameters to trigger.

Acknowledged.

[beaudoin, 2015-03-26 19:32:50]

The CQ bit was checked by beaudoin@chromium.org

[beaudoin, 2015-03-26 19:32:51]

The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@google.com
Link to the patchset: https://codereview.chromium.org/1032073003/#ps40001
(title: "Fixed in files Sam recommended.")

[commit-bot: I haz the power, 2015-03-26 19:32:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1032073003/40001

[commit-bot: I haz the power, 2015-03-26 19:33:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-26 19:33:02]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[beaudoin, 2015-03-26 19:36:55]

beaudoin@chromium.org changed reviewers:
- pkotwicz@chromium.org

[beaudoin, 2015-03-26 19:36:55]

beaudoin@chromium.org changed required reviewers:
- pkotwicz@chromium.org

[beaudoin, 2015-03-26 19:37:03]

The CQ bit was checked by beaudoin@chromium.org

[commit-bot: I haz the power, 2015-03-26 19:37:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1032073003/40001

[pkotwicz, 2015-03-26 19:42:43]

LGTM

[commit-bot: I haz the power, 2015-03-26 20:22:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-26 20:23:19]

Patchset 3 (id:??) landed as
https://crrev.com/accd94964315b34fe738166b173992477965b88c
Cr-Commit-Position: refs/heads/master@{#322456}