Ensure GetSecurityStyle sets correct explanations for HTTP_SHOW_WARNING

Ensure GetSecurityStyle sets correct explanations for HTTP_SHOW_WARNING

When the security level is HTTP_SHOW_WARNING, we may add an explanation
to the Developer Tools console. We must ensure that we only add the
Form Not Secure explanation if the markup displayed a sensitive input
field.

Also remove adjacent dead code related to the completed field trial; Form Not Secure is always on now.

BUG=691412
TEST=SecurityStateTabHelperTest.*

TBR=sdefresne@chromium.org

Review-Url: https://codereview.chromium.org/2727353002
Cr-Commit-Position: refs/heads/master@{#454443}
Committed: https://chromium.googlesource.com/chromium/src/+/abcf7eeec655ec47740a7676a4c61e164e69189d

[elawrence, 2017-03-02 21:38:46]

Description was changed from

==========
Ensure GetSecurityStyle sets correct explanations for HTTP_SHOW_WARNING

When the security level is HTTP_SHOW_WARNING, we may add an explanation
to the Developer Tools console. We must ensure that we only add the
Form Not Secure explanation if the markup displayed a sensitive input
field.

BUG=691412
TEST=SecurityStateTabHelperTest.*
==========

to

==========
Ensure GetSecurityStyle sets correct explanations for HTTP_SHOW_WARNING

When the security level is HTTP_SHOW_WARNING, we may add an explanation
to the Developer Tools console. We must ensure that we only add the
Form Not Secure explanation if the markup displayed a sensitive input
field.

Also remove adjacent dead code related to the completed field trial; Form Not
Secure is always on now.

BUG=691412
TEST=SecurityStateTabHelperTest.*
==========

[elawrence, 2017-03-02 21:48:08]

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

[commit-bot: I haz the power, 2017-03-02 21:48:44]

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

[elawrence, 2017-03-02 22:15:28]

Patchset #2 (id:20001) has been deleted

[elawrence, 2017-03-02 22:27:53]

Patchset #2 (id:40001) has been deleted

[elawrence, 2017-03-02 22:32:29]

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

[commit-bot: I haz the power, 2017-03-02 22:32:55]

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

[elawrence, 2017-03-02 22:34:00]

elawrence@chromium.org changed reviewers:
+ estark@chromium.org, sdefresne@chromium.org

[elawrence, 2017-03-02 22:34:01]

sdefresne@chromium.org: Please review the deletion in the .grd files?

estark@chromium.org: Please review changes in the .cc files?

[estark, 2017-03-02 22:53:33]

lgtm with a few nits. Thanks for cleaning this up!

https://codereview.chromium.org/2727353002/diff/60001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right):

https://codereview.chromium.org/2727353002/diff/60001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:810: // Ensure
Developer Tools don't show an incorrect Form Not Secure explanation.
layering-and-comment nit: it's mildly undesirable to talk about devtools here,
since ostensibly you're testing a generic API that happens to be used by
devtools but could also be used by other stuff. Also, I sometimes like to point
out when something's a regression test with a link to the bug. Suggested
rephrasing:

// Ensure that WebContentsObservers don't show an incorrect Form Not Secure
explanation. Regression test for https://crbug.com/XXXXXX.

https://codereview.chromium.org/2727353002/diff/60001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:812:
observer.latest_explanations();
Perhaps also check that latest_security_style() is Unauthenticated, to sanity
check that the observer indeed saw the event.

https://codereview.chromium.org/2727353002/diff/60001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:813: EXPECT_EQ(0u,
explanations.unauthenticated_explanations.size());
optional nit: since you're only using it once, I'd skip the |explanations| var
and just inline it:

EXPECT_EQ(0u,
observer.latest_explanations().unauthenticated_explanations.size());

https://codereview.chromium.org/2727353002/diff/60001/components/security_sta...
File components/security_state/content/content_utils.cc (right):

https://codereview.chromium.org/2727353002/diff/60001/components/security_sta...
components/security_state/content/content_utils.cc:192: // Display an
unauthenticated explanation explaining why the omnibox warning
optional nit: I don't think this comment is helpful, as it pretty much just
restates the code.

https://codereview.chromium.org/2727353002/diff/60001/components/security_sta...
File components/security_state/content/content_utils_unittest.cc (right):

https://codereview.chromium.org/2727353002/diff/60001/components/security_sta...
components/security_state/content/content_utils_unittest.cc:206: //
content::SecurityStyle of UNAUTHENTICATED and an explanation if appropriate.
"content::SecurityStyle" is out of date (sorry) -- can you please update it to
"Tests that a security level of HTTP_SHOW_WARNING produces
blink::WebSecurityStyleUnauthenticated and ..."

[elawrence, 2017-03-02 23:11:43]

Thanks for the fast turnaround, estark!

As the change to the .grd is trivial, I'll TBR sdefresne in the hopes of making
it in before the branch.

https://codereview.chromium.org/2727353002/diff/60001/chrome/browser/ssl/secu...
File chrome/browser/ssl/security_state_tab_helper_browser_tests.cc (right):

https://codereview.chromium.org/2727353002/diff/60001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:810: // Ensure
Developer Tools don't show an incorrect Form Not Secure explanation.
On 2017/03/02 22:53:33, estark wrote:
> layering-and-comment nit: it's mildly undesirable to talk about devtools here,
> since ostensibly you're testing a generic API that happens to be used by
> devtools but could also be used by other stuff. Also, I sometimes like to
point
> out when something's a regression test with a link to the bug. Suggested
> rephrasing:
> 
> // Ensure that WebContentsObservers don't show an incorrect Form Not Secure
> explanation. Regression test for https://crbug.com/XXXXXX.

Done.

https://codereview.chromium.org/2727353002/diff/60001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:812:
observer.latest_explanations();
On 2017/03/02 22:53:33, estark wrote:
> Perhaps also check that latest_security_style() is Unauthenticated, to sanity
> check that the observer indeed saw the event.

Done.

https://codereview.chromium.org/2727353002/diff/60001/chrome/browser/ssl/secu...
chrome/browser/ssl/security_state_tab_helper_browser_tests.cc:813: EXPECT_EQ(0u,
explanations.unauthenticated_explanations.size());
On 2017/03/02 22:53:33, estark wrote:
> optional nit: since you're only using it once, I'd skip the |explanations| var
> and just inline it:
> 
> EXPECT_EQ(0u,
> observer.latest_explanations().unauthenticated_explanations.size());

Done.

https://codereview.chromium.org/2727353002/diff/60001/components/security_sta...
File components/security_state/content/content_utils.cc (right):

https://codereview.chromium.org/2727353002/diff/60001/components/security_sta...
components/security_state/content/content_utils.cc:192: // Display an
unauthenticated explanation explaining why the omnibox warning
On 2017/03/02 22:53:33, estark wrote:
> optional nit: I don't think this comment is helpful, as it pretty much just
> restates the code.

Done.

https://codereview.chromium.org/2727353002/diff/60001/components/security_sta...
File components/security_state/content/content_utils_unittest.cc (right):

https://codereview.chromium.org/2727353002/diff/60001/components/security_sta...
components/security_state/content/content_utils_unittest.cc:206: //
content::SecurityStyle of UNAUTHENTICATED and an explanation if appropriate.
On 2017/03/02 22:53:33, estark wrote:
> "content::SecurityStyle" is out of date (sorry) -- can you please update it to
> "Tests that a security level of HTTP_SHOW_WARNING produces
> blink::WebSecurityStyleUnauthenticated and ..."

Done.

[elawrence, 2017-03-02 23:12:15]

Description was changed from

==========
Ensure GetSecurityStyle sets correct explanations for HTTP_SHOW_WARNING

When the security level is HTTP_SHOW_WARNING, we may add an explanation
to the Developer Tools console. We must ensure that we only add the
Form Not Secure explanation if the markup displayed a sensitive input
field.

Also remove adjacent dead code related to the completed field trial; Form Not
Secure is always on now.

BUG=691412
TEST=SecurityStateTabHelperTest.*
==========

to

==========
Ensure GetSecurityStyle sets correct explanations for HTTP_SHOW_WARNING

When the security level is HTTP_SHOW_WARNING, we may add an explanation
to the Developer Tools console. We must ensure that we only add the
Form Not Secure explanation if the markup displayed a sensitive input
field.

Also remove adjacent dead code related to the completed field trial; Form Not
Secure is always on now.

BUG=691412
TEST=SecurityStateTabHelperTest.*

TBR=sdefresne@chromium.org
==========

[elawrence, 2017-03-02 23:12:20]

The CQ bit was checked by elawrence@chromium.org

[elawrence, 2017-03-02 23:12:20]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2727353002/#ps80001
(title: "Address estark feedback")

[commit-bot: I haz the power, 2017-03-02 23:13:03]

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

[sdefresne, 2017-03-02 23:23:28]

*.grd lgtm

[commit-bot: I haz the power, 2017-03-03 00:01:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488496340057550,
"parent_rev": "55fe0730698fe3a8c88756055c9746a4b6984a7c", "commit_rev":
"abcf7eeec655ec47740a7676a4c61e164e69189d"}

[commit-bot: I haz the power, 2017-03-03 00:02:14]

Description was changed from

==========
Ensure GetSecurityStyle sets correct explanations for HTTP_SHOW_WARNING

When the security level is HTTP_SHOW_WARNING, we may add an explanation
to the Developer Tools console. We must ensure that we only add the
Form Not Secure explanation if the markup displayed a sensitive input
field.

Also remove adjacent dead code related to the completed field trial; Form Not
Secure is always on now.

BUG=691412
TEST=SecurityStateTabHelperTest.*

TBR=sdefresne@chromium.org
==========

to

==========
Ensure GetSecurityStyle sets correct explanations for HTTP_SHOW_WARNING

When the security level is HTTP_SHOW_WARNING, we may add an explanation
to the Developer Tools console. We must ensure that we only add the
Form Not Secure explanation if the markup displayed a sensitive input
field.

Also remove adjacent dead code related to the completed field trial; Form Not
Secure is always on now.

BUG=691412
TEST=SecurityStateTabHelperTest.*

TBR=sdefresne@chromium.org

Review-Url: https://codereview.chromium.org/2727353002
Cr-Commit-Position: refs/heads/master@{#454443}
Committed:
https://chromium.googlesource.com/chromium/src/+/abcf7eeec655ec47740a7676a4c6...
==========

[commit-bot: I haz the power, 2017-03-03 00:02:15]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/abcf7eeec655ec47740a7676a4c6...