HTTP Bad: Add a switch for showing both verbose state and form warning

HTTP Bad: Add a switch for showing both verbose state and form warning

This CL introduces a new switch kMarkHttpWithPasswordsOrCcWithChipAndFormWarning
that enables both the omnibox warning and the autofill dropdown warning in HTTP
Bad. That way, in case there are problems, we can launch the omnibox warning
without the autofill dropdown warning via Finch.

BUG=662298, 662297
Committed: https://crrev.com/b928f12db89f12ed5c1911859080318ca21172eb
Cr-Commit-Position: refs/heads/master@{#430151}

[lshang, 2016-11-04 08:08:03]

Description was changed from

==========
HTTP Bad: Add a switch for showing both verbose state and form warning


add a switch flag

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

to

==========
HTTP Bad: Add a switch for showing both verbose state and form warning

This CL introduces a new switch kMarkHttpWithPasswordsOrCcWithChipAndFormWarning

that enables both the omnibox warning and the autofill dropdown warning in HTTP 
Bad. That way, in case there are problems, we can launch the omnibox warning 
without the autofill dropdown warning via Finch.

BUG=662298, 662297
==========

[lshang, 2016-11-04 08:11:51]

lshang@chromium.org changed reviewers:
+ estark@chromium.org, sky@chromium.org

[lshang, 2016-11-04 08:11:51]

sky@chromium.org: Please review changes in
  chrome/browser/about_flags.cc
  chrome/app/generated_resources.grd

estark@chromium.org: Please review changes in 
  components/security_state/

PTAL thanks!

[estark, 2016-11-04 15:18:37]

Thanks! Looks good, just one request: could instantiate the tests in
chrome_security_state_model_client_browser_tests.cc to test the new switch (i.e.
make sure that the HTTP_SHOW_WARNING state gets set when the new switch is in
use)? I'm thinking that you can have a boolean parameter and, in
https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state...,
use kMarkHttpWithPasswordsOrCcWithChip if GetParam() is false and
kMarkHttpWithPasswordsOrCcWithChipAndFormWarning if it's true.

So you'd just have to use GetParam() to set the switch there, and then
INSTANTIATE_TEST_CASE_P(ChromeSecurityStateModelClientTestWithPasswordCcSwitch,
ChromeSecurityStateModelClientTestWithPasswordCcSwitch, ...), and change
IN_PROC_BROWSER_TEST_F to IN_PROC_BROWSER_TEST_F.

(Here's an example of using INSTANTIATE_TEST_CASE in case you haven't used that
before:
https://cs.chromium.org/chromium/src/chrome/browser/policy/policy_browsertest...)

[sky, 2016-11-04 15:53:26]

LGTM

[lshang, 2016-11-05 03:34:01]

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

[commit-bot: I haz the power, 2016-11-05 03:34:15]

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

[lshang, 2016-11-05 03:35:07]

On 2016/11/04 15:18:37, estark wrote:
> Thanks! Looks good, just one request: could instantiate the tests in
> chrome_security_state_model_client_browser_tests.cc to test the new switch
(i.e.
> make sure that the HTTP_SHOW_WARNING state gets set when the new switch is in
> use)? I'm thinking that you can have a boolean parameter and, in
>
https://cs.chromium.org/chromium/src/chrome/browser/ssl/chrome_security_state...,
> use kMarkHttpWithPasswordsOrCcWithChip if GetParam() is false and
> kMarkHttpWithPasswordsOrCcWithChipAndFormWarning if it's true.
> 
> So you'd just have to use GetParam() to set the switch there, and then
>
INSTANTIATE_TEST_CASE_P(ChromeSecurityStateModelClientTestWithPasswordCcSwitch,
> ChromeSecurityStateModelClientTestWithPasswordCcSwitch, ...), and change
> IN_PROC_BROWSER_TEST_F to IN_PROC_BROWSER_TEST_F.
> 
> (Here's an example of using INSTANTIATE_TEST_CASE in case you haven't used
that
> before:
>
https://cs.chromium.org/chromium/src/chrome/browser/policy/policy_browsertest...)

Done!

Thanks for the detailed explanation. I found that INSTANTIATE_TEST_CASE is
pretty good and useful! XD

[commit-bot: I haz the power, 2016-11-05 03:36:27]

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

[commit-bot: I haz the power, 2016-11-05 03:36:28]

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-11-05 03:44:22]

lgtm, thanks Liu!

https://codereview.chromium.org/2472943004/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2472943004/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1504:
testing::Bool());
nit: maybe you could add a comment here that 'true' means that the omnibox
warning + form warning is enabled and false means just the omnibox warning is
enabled

[lshang, 2016-11-05 03:44:50]

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

[commit-bot: I haz the power, 2016-11-05 03:44:58]

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

[lshang, 2016-11-05 03:56:53]

Thanks all!

https://codereview.chromium.org/2472943004/diff/20001/chrome/browser/ssl/chro...
File chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc
(right):

https://codereview.chromium.org/2472943004/diff/20001/chrome/browser/ssl/chro...
chrome/browser/ssl/chrome_security_state_model_client_browser_tests.cc:1504:
testing::Bool());
On 2016/11/05 03:44:22, estark wrote:
> nit: maybe you could add a comment here that 'true' means that the omnibox
> warning + form warning is enabled and false means just the omnibox warning is
> enabled

Done.

[lshang, 2016-11-05 03:57:02]

The CQ bit was checked by lshang@chromium.org

[lshang, 2016-11-05 03:57:02]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2472943004/#ps60001
(title: "add a comment")

[commit-bot: I haz the power, 2016-11-05 03:57: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-11-05 04:35:31]

Description was changed from

==========
HTTP Bad: Add a switch for showing both verbose state and form warning

This CL introduces a new switch kMarkHttpWithPasswordsOrCcWithChipAndFormWarning

that enables both the omnibox warning and the autofill dropdown warning in HTTP 
Bad. That way, in case there are problems, we can launch the omnibox warning 
without the autofill dropdown warning via Finch.

BUG=662298, 662297
==========

to

==========
HTTP Bad: Add a switch for showing both verbose state and form warning

This CL introduces a new switch kMarkHttpWithPasswordsOrCcWithChipAndFormWarning

that enables both the omnibox warning and the autofill dropdown warning in HTTP 
Bad. That way, in case there are problems, we can launch the omnibox warning 
without the autofill dropdown warning via Finch.

BUG=662298, 662297
==========

[commit-bot: I haz the power, 2016-11-05 04:35:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-05 04:37:22]

Description was changed from

==========
HTTP Bad: Add a switch for showing both verbose state and form warning

This CL introduces a new switch kMarkHttpWithPasswordsOrCcWithChipAndFormWarning

that enables both the omnibox warning and the autofill dropdown warning in HTTP 
Bad. That way, in case there are problems, we can launch the omnibox warning 
without the autofill dropdown warning via Finch.

BUG=662298, 662297
==========

to

==========
HTTP Bad: Add a switch for showing both verbose state and form warning

This CL introduces a new switch kMarkHttpWithPasswordsOrCcWithChipAndFormWarning
that enables both the omnibox warning and the autofill dropdown warning in HTTP
Bad. That way, in case there are problems, we can launch the omnibox warning
without the autofill dropdown warning via Finch.

BUG=662298, 662297

Committed: https://crrev.com/b928f12db89f12ed5c1911859080318ca21172eb
Cr-Commit-Position: refs/heads/master@{#430151}
==========

[commit-bot: I haz the power, 2016-11-05 04:37:23]

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