Make form-not-secure controllable as its own separate Finch feature

Make form-not-secure controllable as its own separate Finch feature

Previously, form-not-secure was enabled as its own value of the
--mark-non-secure-as flag. That value also enabled the "Not secure" omnibox
warning. Also, the autofill code that checked for the command-line flag
didn't allow us to enable it via Finch.

This CL moves the form-not-secure warning into its own Finch feature and
corresponding chrome://flags flag. This lets us turn it on and off via Finch,
independently from the "Not secure" omnibox warning that is controlled by
--mark-non-secure-as.

BUG=674320
Committed: https://crrev.com/04c3307c17d85118389afd8e3811daf4d60f823e
Cr-Commit-Position: refs/heads/master@{#439753}

[estark, 2016-12-19 19:35:53]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 19:36:40]

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

[estark, 2016-12-19 19:38:32]

Description was changed from

==========
Make form-not-secure controllable as its own separate Finch feature

This lets us control the form-not-secure warning via a Finch feature and
independently from the omnibox "Not secure" warning.

BUG=674320
==========

to

==========
Make form-not-secure controllable as its own separate Finch feature

Previously, form-not-secure was enabled as its own value of the
--mark-non-secure-as flag. That value also enabled the "Not secure" omnibox
warning. Also, the autofill code that checked for the command-line flag
didn't allow us to enable it via Finch.

This CL moves the form-not-secure warning into its own Finch feature and
corresponding chrome://flags flag. This lets us turn it on and off via Finch,
independently from the "Not secure" omnibox warning that is controlled by
--mark-non-secure-as.

BUG=674320
==========

[estark, 2016-12-19 19:40:38]

estark@chromium.org changed reviewers:
+ elawrence@chromium.org, engedy@chromium.org, estade@chromium.org

[estark, 2016-12-19 19:40:39]

+elawrence for components/security_state and general review
+estade for components/autofill
+engedy for components/password_manager
Thanks, all!

[estark, 2016-12-19 19:41:19]

By the way, I'll note that the chrome://flags value that I'm removing in this CL
hasn't been advertised widely yet so I don't feel too sad about removing it.

[elawrence, 2016-12-19 20:11:46]

https://codereview.chromium.org/2588133002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2588133002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5365: +        Shows an in-form warning when
password or credit card fields are detected on an HTTP page
The wording "in-form warnings" seems somewhat more clear than "form warnings"
but we don't use that wording in the name, only in the description? 

"Shows an in-form warning when password or credit card fields are detected on an
HTTP page" -- Are the warnings in the |form| even if the field is not, or are
the warnings added to the field itself, such that an invisible field does not
show the warning? If the latter, maybe "Attaches a warning UI to any password or
credit card fields detected on a non-secure page"?

The phrasing `HTTP Page` feels a bit problematic, insofar as a common pattern is
to embed a HTTPS page containing a sensitive field, in a frame on a non-secure
outer page.

https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
File components/security_state/core/security_state.h (right):

https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:29: // and credit cards fields
on HTTP pages.
Is this about "HTTP pages" or "non-secure contexts"? E.g. if I have an
FTP-served page, what happens? What about a HTTPS page with active mixed
content?

https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:224: // Returns true if an
experimental form warning about HTTP passwords and
Should we be explicit that this is a "warning UI" as opposed to some other
method of warning (e.g. dev console)?

https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:226: //
--mark-non-secure-as=show-non-secure-passwords-cc-chip-and-form-warning
Is the "show-non-secure-passwords-cc-chip-and-form-warning" value still
supported? The change in the subsequent file
(https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...)
and switches.h makes me think it's not?

[Mathieu, 2016-12-19 20:14:01]

mathp@chromium.org changed reviewers:
+ mathp@google.com
- estade@chromium.org

[Mathieu, 2016-12-19 20:14:16]

mathp@chromium.org changed reviewers:
+ mathp@chromium.org
- mathp@google.com

[Mathieu, 2016-12-19 20:15:49]

https://codereview.chromium.org/2588133002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/2588133002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_experiments.cc:231: bool
IsCreditCardAutofillHttpWarningEnabled() {
I think it's fine to remove this function and call security_state::IsHttp... in
the autofill code directly.

[commit-bot: I haz the power, 2016-12-19 20:41:38]

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

[commit-bot: I haz the power, 2016-12-19 20:41:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[estark, 2016-12-19 20:49:29]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[estark, 2016-12-19 20:49:46]

https://codereview.chromium.org/2588133002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2588133002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:5365: +        Shows an in-form warning when
password or credit card fields are detected on an HTTP page
On 2016/12/19 20:11:46, elawrence wrote:
> The wording "in-form warnings" seems somewhat more clear than "form warnings"
> but we don't use that wording in the name, only in the description? 

Done.

> 
> "Shows an in-form warning when password or credit card fields are detected on
an
> HTTP page" -- Are the warnings in the |form| even if the field is not, or are
> the warnings added to the field itself, such that an invisible field does not
> show the warning? If the latter, maybe "Attaches a warning UI to any password
or
> credit card fields detected on a non-secure page"?

Done.

> 
> The phrasing `HTTP Page` feels a bit problematic, insofar as a common pattern
is
> to embed a HTTPS page containing a sensitive field, in a frame on a non-secure
> outer page.

Changed to "when the top-level page is not HTTPS" which is kind of wordy, but
honestly I've kinda given up hope on using great terminology for these things.
Everything I can think of is confusing and/or inaccurate in one way or another.

https://codereview.chromium.org/2588133002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/2588133002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_experiments.cc:231: bool
IsCreditCardAutofillHttpWarningEnabled() {
On 2016/12/19 20:15:49, Mathieu (OOO Dec 21-Jan 4) wrote:
> I think it's fine to remove this function and call security_state::IsHttp...
in
> the autofill code directly.

Done.

https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
File components/security_state/core/security_state.h (right):

https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:29: // and credit cards fields
on HTTP pages.
On 2016/12/19 20:11:46, elawrence wrote:
> Is this about "HTTP pages" or "non-secure contexts"? E.g. if I have an
> FTP-served page, what happens? What about a HTTPS page with active mixed
> content?

Changed to "when the top-level page is not HTTPS" which describes pretty much
exactly what we do.

https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:224: // Returns true if an
experimental form warning about HTTP passwords and
On 2016/12/19 20:11:46, elawrence wrote:
> Should we be explicit that this is a "warning UI" as opposed to some other
> method of warning (e.g. dev console)?

Done.

https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
components/security_state/core/security_state.h:226: //
--mark-non-secure-as=show-non-secure-passwords-cc-chip-and-form-warning
On 2016/12/19 20:11:46, elawrence wrote:
> Is the "show-non-secure-passwords-cc-chip-and-form-warning" value still
> supported? The change in the subsequent file
>
(https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...)
> and switches.h makes me think it's not?

No, forgot to update this comment. Fixed.

[commit-bot: I haz the power, 2016-12-19 20:50:19]

Dry run: 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-12-19 20:52:51]

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

[commit-bot: I haz the power, 2016-12-19 20:52:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[elawrence, 2016-12-19 21:06:35]

On 2016/12/19 20:49:46, estark wrote:
>
https://codereview.chromium.org/2588133002/diff/1/chrome/app/generated_resour...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/2588133002/diff/1/chrome/app/generated_resour...
> chrome/app/generated_resources.grd:5365: +        Shows an in-form warning
when
> password or credit card fields are detected on an HTTP page
> On 2016/12/19 20:11:46, elawrence wrote:
> > The wording "in-form warnings" seems somewhat more clear than "form
warnings"
> > but we don't use that wording in the name, only in the description? 
> 
> Done.
> 
> > 
> > "Shows an in-form warning when password or credit card fields are detected
on
> an
> > HTTP page" -- Are the warnings in the |form| even if the field is not, or
are
> > the warnings added to the field itself, such that an invisible field does
not
> > show the warning? If the latter, maybe "Attaches a warning UI to any
password
> or
> > credit card fields detected on a non-secure page"?
> 
> Done.
> 
> > 
> > The phrasing `HTTP Page` feels a bit problematic, insofar as a common
pattern
> is
> > to embed a HTTPS page containing a sensitive field, in a frame on a
non-secure
> > outer page.
> 
> Changed to "when the top-level page is not HTTPS" which is kind of wordy, but
> honestly I've kinda given up hope on using great terminology for these things.
> Everything I can think of is confusing and/or inaccurate in one way or
another.
> 
>
https://codereview.chromium.org/2588133002/diff/1/components/autofill/core/br...
> File components/autofill/core/browser/autofill_experiments.cc (right):
> 
>
https://codereview.chromium.org/2588133002/diff/1/components/autofill/core/br...
> components/autofill/core/browser/autofill_experiments.cc:231: bool
> IsCreditCardAutofillHttpWarningEnabled() {
> On 2016/12/19 20:15:49, Mathieu (OOO Dec 21-Jan 4) wrote:
> > I think it's fine to remove this function and call security_state::IsHttp...
> in
> > the autofill code directly.
> 
> Done.
> 
>
https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
> File components/security_state/core/security_state.h (right):
> 
>
https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
> components/security_state/core/security_state.h:29: // and credit cards fields
> on HTTP pages.
> On 2016/12/19 20:11:46, elawrence wrote:
> > Is this about "HTTP pages" or "non-secure contexts"? E.g. if I have an
> > FTP-served page, what happens? What about a HTTPS page with active mixed
> > content?
> 
> Changed to "when the top-level page is not HTTPS" which describes pretty much
> exactly what we do.
> 
>
https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
> components/security_state/core/security_state.h:224: // Returns true if an
> experimental form warning about HTTP passwords and
> On 2016/12/19 20:11:46, elawrence wrote:
> > Should we be explicit that this is a "warning UI" as opposed to some other
> > method of warning (e.g. dev console)?
> 
> Done.
> 
>
https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...
> components/security_state/core/security_state.h:226: //
> --mark-non-secure-as=show-non-secure-passwords-cc-chip-and-form-warning
> On 2016/12/19 20:11:46, elawrence wrote:
> > Is the "show-non-secure-passwords-cc-chip-and-form-warning" value still
> > supported? The change in the subsequent file
> >
>
(https://codereview.chromium.org/2588133002/diff/1/components/security_state/c...)
> > and switches.h makes me think it's not?
> 
> No, forgot to update this comment. Fixed.

LGTM. I assume the values changed in histograms.xml are automatically being used
by some pre-existing Feature logging code?

[Evan Stade, 2016-12-19 21:11:17]

estade@chromium.org changed reviewers:
+ estade@chromium.org

[Evan Stade, 2016-12-19 21:11:17]

components/autofill lgtm

[Mathieu, 2016-12-19 23:56:00]

lgtm

https://codereview.chromium.org/2588133002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2588133002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5361: +      <message
name="IDS_ENABLE_HTTP_FORM_WARNING_NAME" desc="Name of the 'HTTP form warning'
lab.">
lab is unintuitive to me. How about "feature"?

[estark, 2016-12-20 02:05:31]

Patchset #4 (id:60001) has been deleted

[estark, 2016-12-20 02:07:01]

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

[estark, 2016-12-20 02:07:02]

isherman, can you please review histograms.xml?

https://codereview.chromium.org/2588133002/diff/20001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2588133002/diff/20001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5361: +      <message
name="IDS_ENABLE_HTTP_FORM_WARNING_NAME" desc="Name of the 'HTTP form warning'
lab.">
On 2016/12/19 23:56:00, Mathieu (OOO Dec 21-Jan 4) wrote:
> lab is unintuitive to me. How about "feature"?

Done.

[Ilya Sherman, 2016-12-20 03:27:39]

histograms.xml lgtm

[estark, 2016-12-20 04:09:44]

The CQ bit was checked by estark@chromium.org

[estark, 2016-12-20 04:09:44]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
elawrence@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2588133002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-20 04:10:11]

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-12-20 04:17:50]

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

[commit-bot: I haz the power, 2016-12-20 04:17:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vabr (Chromium), 2016-12-20 07:18:39]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2016-12-20 07:18:40]

components/password_manager LGTM, good to see base::Feature instead of a command
line flag!

Cheers,
Vaclav

[estark, 2016-12-20 07:20:32]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-12-20 07:20:59]

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-12-20 08:47:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482218432662270,
"parent_rev": "980685583eeb5015971053b4b623e2f9e8dac697", "commit_rev":
"96229ab95e858c160c89ceea798bf3491e7ff902"}

[commit-bot: I haz the power, 2016-12-20 08:47:49]

Description was changed from

==========
Make form-not-secure controllable as its own separate Finch feature

Previously, form-not-secure was enabled as its own value of the
--mark-non-secure-as flag. That value also enabled the "Not secure" omnibox
warning. Also, the autofill code that checked for the command-line flag
didn't allow us to enable it via Finch.

This CL moves the form-not-secure warning into its own Finch feature and
corresponding chrome://flags flag. This lets us turn it on and off via Finch,
independently from the "Not secure" omnibox warning that is controlled by
--mark-non-secure-as.

BUG=674320
==========

to

==========
Make form-not-secure controllable as its own separate Finch feature

Previously, form-not-secure was enabled as its own value of the
--mark-non-secure-as flag. That value also enabled the "Not secure" omnibox
warning. Also, the autofill code that checked for the command-line flag
didn't allow us to enable it via Finch.

This CL moves the form-not-secure warning into its own Finch feature and
corresponding chrome://flags flag. This lets us turn it on and off via Finch,
independently from the "Not secure" omnibox warning that is controlled by
--mark-non-secure-as.

BUG=674320

Review-Url: https://codereview.chromium.org/2588133002
==========

[commit-bot: I haz the power, 2016-12-20 08:47:50]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-12-20 08:49:59]

Description was changed from

==========
Make form-not-secure controllable as its own separate Finch feature

Previously, form-not-secure was enabled as its own value of the
--mark-non-secure-as flag. That value also enabled the "Not secure" omnibox
warning. Also, the autofill code that checked for the command-line flag
didn't allow us to enable it via Finch.

This CL moves the form-not-secure warning into its own Finch feature and
corresponding chrome://flags flag. This lets us turn it on and off via Finch,
independently from the "Not secure" omnibox warning that is controlled by
--mark-non-secure-as.

BUG=674320

Review-Url: https://codereview.chromium.org/2588133002
==========

to

==========
Make form-not-secure controllable as its own separate Finch feature

Previously, form-not-secure was enabled as its own value of the
--mark-non-secure-as flag. That value also enabled the "Not secure" omnibox
warning. Also, the autofill code that checked for the command-line flag
didn't allow us to enable it via Finch.

This CL moves the form-not-secure warning into its own Finch feature and
corresponding chrome://flags flag. This lets us turn it on and off via Finch,
independently from the "Not secure" omnibox warning that is controlled by
--mark-non-secure-as.

BUG=674320

Committed: https://crrev.com/04c3307c17d85118389afd8e3811daf4d60f823e
Cr-Commit-Position: refs/heads/master@{#439753}
==========

[commit-bot: I haz the power, 2016-12-20 08:50:00]

Patchset 4 (id:??) landed as
https://crrev.com/04c3307c17d85118389afd8e3811daf4d60f823e
Cr-Commit-Position: refs/heads/master@{#439753}