Add a feature flag for the security verbose

Add a feature flag for the security verbose

BUG=658379
Committed: https://crrev.com/95bbc94d79f726cfecf15f47d8a487f6d42ac8ad
Cr-Commit-Position: refs/heads/master@{#427526}

[spqchan, 2016-10-21 19:46:55]

Description was changed from

==========
Experiment Finch flags for the security verbose


BUG=
==========

to

==========
Add feature flags for the security verbose

BUG=658379
==========

[spqchan, 2016-10-21 19:47:10]

Description was changed from

==========
Add feature flags for the security verbose

BUG=658379
==========

to

==========
Add a feature flag for the security verbose

BUG=658379
==========

[spqchan, 2016-10-21 20:12:08]

spqchan@chromium.org changed reviewers:
+ estark@chromium.org

[spqchan, 2016-10-21 20:12:08]

Hey estark,

Do you mind looking over this? I'm not too sure if I'm combining the use of the
switches and feature flags properly.

Thanks!

[estark, 2016-10-21 20:32:27]

Thanks, lgtm! Just a few nits inline.

There's one more thing you'll probably need to do, which is to add a section to
testing/variations/fieldtrial_testing_config.json so that the features get
tested on the waterfall. I think it should look something like this:

"SecurityChips": [
  {
    "platforms": [...],
    "experiments": [
      {
        "name": "SecurityChips",
        "enable_features": ["SecurityChipFeature"],
        "params": {
          "SecurityChipVisibility": "show-all",
          "SecurityChipAnimation": "animate-all"
        }
      }
    ]
  }
]

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:142: extern const base::Feature
kSecurityChipFeature{
nit: I don't think you need `extern` here (only in the .h)

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:142: extern const base::Feature
kSecurityChipFeature{
nit: this should have a comment above explaining it, something along the lines
of:

// Enables or disables security chips in the omnibox that show verbose
explanations for security states.

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
chrome/common/chrome_features.h:89: extern const base::Feature
kSecurityChipFeature;
nit: for consistency with the other features, I think you could probably just
name this `kSecurityChip`

[spqchan, 2016-10-21 21:57:06]

Thanks! I'm currently making the changes for json file. I just need to figure
out what experiments we need.

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:142: extern const base::Feature
kSecurityChipFeature{
On 2016/10/21 20:32:27, estark wrote:
> nit: this should have a comment above explaining it, something along the lines
> of:
> 
> // Enables or disables security chips in the omnibox that show verbose
> explanations for security states.

Done.

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
chrome/common/chrome_features.cc:142: extern const base::Feature
kSecurityChipFeature{
On 2016/10/21 20:32:27, estark wrote:
> nit: I don't think you need `extern` here (only in the .h)

Done.

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2430923010/diff/80001/chrome/common/chrome_fe...
chrome/common/chrome_features.h:89: extern const base::Feature
kSecurityChipFeature;
On 2016/10/21 20:32:27, estark wrote:
> nit: for consistency with the other features, I think you could probably just
> name this `kSecurityChip`

Done.

[spqchan, 2016-10-22 00:02:53]

spqchan@chromium.org changed reviewers:
+ rsesek@chromium.org

[spqchan, 2016-10-22 00:02:53]

+rsesek for Mac review

[Robert Sesek, 2016-10-24 15:17:43]

rslgtm

[Robert Sesek, 2016-10-24 15:17:51]

On 2016/10/24 15:17:43, Robert Sesek wrote:
> rs lgtm

[Robert Sesek, 2016-10-24 15:17:57]

rs lgtm

[spqchan, 2016-10-24 17:38:25]

spqchan@chromium.org changed reviewers:
+ pkasting@chromium.org

[spqchan, 2016-10-24 17:38:25]

thanks!

+pkasting for Views

[Peter Kasting, 2016-10-24 21:08:17]

LGTM

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/loca...
File chrome/browser/ui/location_bar/location_bar.cc (right):

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/loca...
chrome/browser/ui/location_bar/location_bar.cc:15: 
Nit: No blank line

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:158: std::string
security_chip;
Nit: |security_chip_visibility| to match constant name

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:171: }
Nit: Shorter:

  should_show_secure_state_ = security_chip == switches::kSecurityChipShowAll;
  should_show_nonsecure_state_ =
      should_show_secure_state_ ||
      (security_chip == switches::kSecurityChipShowNonSecureOnly);

A similar comment applies to the animation block below.

[spqchan, 2016-10-24 22:44:55]

spqchan@chromium.org changed reviewers:
+ isherman@chromium.org

[spqchan, 2016-10-24 22:44:56]

Thanks!

+isherman for testing/variations/fieldtrial_testing_config.json ownership

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/loca...
File chrome/browser/ui/location_bar/location_bar.cc (right):

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/loca...
chrome/browser/ui/location_bar/location_bar.cc:15: 
On 2016/10/24 21:08:16, Peter Kasting wrote:
> Nit: No blank line

Done.

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:158: std::string
security_chip;
On 2016/10/24 21:08:16, Peter Kasting wrote:
> Nit: |security_chip_visibility| to match constant name

Done.

https://codereview.chromium.org/2430923010/diff/100002/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:171: }
On 2016/10/24 21:08:17, Peter Kasting wrote:
> Nit: Shorter:
> 
>   should_show_secure_state_ = security_chip == switches::kSecurityChipShowAll;
>   should_show_nonsecure_state_ =
>       should_show_secure_state_ ||
>       (security_chip == switches::kSecurityChipShowNonSecureOnly);
> 
> A similar comment applies to the animation block below.

Done.

[Ilya Sherman, 2016-10-24 23:37:01]

https://codereview.chromium.org/2430923010/diff/130001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config.json (right):

https://codereview.chromium.org/2430923010/diff/130001/testing/variations/fie...
testing/variations/fieldtrial_testing_config.json:1793: ],
Is there an active field trial (or a soon-to-be-active field trial) for this
experiment?  If not, then there's no need to land this config yet.  I didn't
find one via code search, but LGTM if this matches a config that you're about to
land.

[estark, 2016-10-25 17:02:02]

https://codereview.chromium.org/2430923010/diff/130001/testing/variations/fie...
File testing/variations/fieldtrial_testing_config.json (right):

https://codereview.chromium.org/2430923010/diff/130001/testing/variations/fie...
testing/variations/fieldtrial_testing_config.json:1793: ],
On 2016/10/24 23:37:01, Ilya Sherman wrote:
> Is there an active field trial (or a soon-to-be-active field trial) for this
> experiment?  If not, then there's no need to land this config yet.  I didn't
> find one via code search, but LGTM if this matches a config that you're about
to
> land.

I think the plan is to create an active field trial as soon as this lands. Does
that sound right to you, spqchan?

[spqchan, 2016-10-25 17:24:56]

On 2016/10/25 17:02:02, estark wrote:
>
https://codereview.chromium.org/2430923010/diff/130001/testing/variations/fie...
> File testing/variations/fieldtrial_testing_config.json (right):
> 
>
https://codereview.chromium.org/2430923010/diff/130001/testing/variations/fie...
> testing/variations/fieldtrial_testing_config.json:1793: ],
> On 2016/10/24 23:37:01, Ilya Sherman wrote:
> > Is there an active field trial (or a soon-to-be-active field trial) for this
> > experiment?  If not, then there's no need to land this config yet.  I didn't
> > find one via code search, but LGTM if this matches a config that you're
about
> to
> > land.
> 
> I think the plan is to create an active field trial as soon as this lands.
Does
> that sound right to you, spqchan?

SGTM, thanks!

[spqchan, 2016-10-25 17:25:00]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-10-25 17:25:01]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org,
rsesek@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2430923010/#ps130001
(title: "Fix for pkasting")

[commit-bot: I haz the power, 2016-10-25 17:25:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 18:34:19]

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

[commit-bot: I haz the power, 2016-10-25 18:34:20]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[spqchan, 2016-10-25 22:38:47]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2016-10-25 22:39:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 23:08:54]

Description was changed from

==========
Add a feature flag for the security verbose

BUG=658379
==========

to

==========
Add a feature flag for the security verbose

BUG=658379
==========

[commit-bot: I haz the power, 2016-10-25 23:08:55]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-10-25 23:10:40]

Description was changed from

==========
Add a feature flag for the security verbose

BUG=658379
==========

to

==========
Add a feature flag for the security verbose

BUG=658379

Committed: https://crrev.com/95bbc94d79f726cfecf15f47d8a487f6d42ac8ad
Cr-Commit-Position: refs/heads/master@{#427526}
==========

[commit-bot: I haz the power, 2016-10-25 23:10:41]

Patchset 8 (id:??) landed as
https://crrev.com/95bbc94d79f726cfecf15f47d8a487f6d42ac8ad
Cr-Commit-Position: refs/heads/master@{#427526}