Show HTTP-Bad security chip UI on Mac and Views

Show HTTP-Bad security chip UI on Mac and Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Mac and Views.

(Note that this will only display if the #mark-non-secure-as flag is
set, since the SecurityStateModel only emits HTTP_SHOW_WARNING when the
flag is set appropriately.)

BUG=647562
Committed: https://crrev.com/de79182ad7338200103842786ab2260e3ab74df3
Cr-Commit-Position: refs/heads/master@{#426033}

[felt, 2016-10-17 19:31:31]

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

[commit-bot: I haz the power, 2016-10-17 19:32:33]

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

[felt, 2016-10-17 19:47:04]

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

[commit-bot: I haz the power, 2016-10-17 19:47:39]

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

[felt, 2016-10-17 19:47:48]

Description was changed from

==========
Show HTTP-Bad security chip UI on Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Views.

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

to

==========
Show HTTP-Bad security chip UI on Mac and Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Mac and Views.

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

[felt, 2016-10-17 20:09:20]

Description was changed from

==========
Show HTTP-Bad security chip UI on Mac and Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Mac and Views.

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

to

==========
Show HTTP-Bad security chip UI on Mac and Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Mac and Views. 

(Note that this will only display if the #mark-non-secure-as flag is
set, since the SecurityStateModel only emits HTTP_SHOW_WARNING when the
flag is set appropriately.)

BUG=647562
==========

[felt, 2016-10-17 20:37:23]

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

[felt, 2016-10-17 20:37:34]

spqchan@, could you please review?

[spqchan, 2016-10-17 20:39:45]

lgtm

[spqchan, 2016-10-17 20:39:47]

lgtm

lgtm

[felt, 2016-10-17 20:43:52]

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

[felt, 2016-10-17 20:44:00]

pkasting, PTAL?

[Peter Kasting, 2016-10-17 20:56:39]

LGTM

https://codereview.chromium.org/2422303002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2422303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:829: security_level ==
security_state::SecurityStateModel::HTTP_SHOW_WARNING)
Hmm, should SHOW_WARNING really use the text color, and not a security text
color (which would presumably be red)?

https://codereview.chromium.org/2422303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1023: level ==
SecurityLevel::HTTP_SHOW_WARNING;
Nit: Shorter, and seems more readable:

  using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
  const SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
  if (level == SecurityLevel::SECURE || level == SecurityLevel::EV_SECURE)
    return should_show_secure_state_;
  return level == SecurityLevel::DANGEROUS ||
         level == SecurityLevel::HTTP_SHOW_WARNING;

https://codereview.chromium.org/2422303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1035: (is_secure_level
&& should_animate_secure_state_));
Nit: Feels more readable to me, and is parallel to my suggestion above:

  if (!ShouldShowSecurityChip())
    return false;

  using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
  const SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
  if (level == SecurityLevel::EV_SECURE || level == SecurityLevel::SECURE)
    return should_animate_secure_state_;
  return should_animate_nonsecure_state_ &&
         (level == SecurityLevel::DANGEROUS ||
          level == SecurityLevel::HTTP_SHOW_WARNING);

You might not need the first conditional at all, depending on how callers call
these two functions.  (If "animate when showing" is orthogonal to "showing to
begin with", you can get away with not having it.)

[commit-bot: I haz the power, 2016-10-17 21:33:59]

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

[commit-bot: I haz the power, 2016-10-17 21:34:00]

Dry run: This issue passed the CQ dry run.

[felt, 2016-10-17 22:56:51]

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

[felt, 2016-10-17 22:56:58]

https://codereview.chromium.org/2422303002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2422303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:829: security_level ==
security_state::SecurityStateModel::HTTP_SHOW_WARNING)
On 2016/10/17 20:56:39, Peter Kasting wrote:
> Hmm, should SHOW_WARNING really use the text color, and not a security text
> color (which would presumably be red)?

That's correct. It intentionally is set to show as the text color and not as
red, as per specs. It's meant to be a mild warning.

https://codereview.chromium.org/2422303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1023: level ==
SecurityLevel::HTTP_SHOW_WARNING;
On 2016/10/17 20:56:39, Peter Kasting wrote:
> Nit: Shorter, and seems more readable:
> 
>   using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
>   const SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
>   if (level == SecurityLevel::SECURE || level == SecurityLevel::EV_SECURE)
>     return should_show_secure_state_;
>   return level == SecurityLevel::DANGEROUS ||
>          level == SecurityLevel::HTTP_SHOW_WARNING;

Done.

https://codereview.chromium.org/2422303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1035: (is_secure_level
&& should_animate_secure_state_));
On 2016/10/17 20:56:39, Peter Kasting wrote:
> Nit: Feels more readable to me, and is parallel to my suggestion above:
> 
>   if (!ShouldShowSecurityChip())
>     return false;
> 
>   using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
>   const SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
>   if (level == SecurityLevel::EV_SECURE || level == SecurityLevel::SECURE)
>     return should_animate_secure_state_;
>   return should_animate_nonsecure_state_ &&
>          (level == SecurityLevel::DANGEROUS ||
>           level == SecurityLevel::HTTP_SHOW_WARNING);
> 
> You might not need the first conditional at all, depending on how callers call
> these two functions.  (If "animate when showing" is orthogonal to "showing to
> begin with", you can get away with not having it.)

Done.

[commit-bot: I haz the power, 2016-10-17 22:57:17]

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

[felt, 2016-10-17 22:57:35]

felt@chromium.org changed reviewers:
+ rohitrao@chromium.org

[felt, 2016-10-17 22:57:47]

rohitrao@chromium.org, PTAL as an OWNERS reviewer for Mac?

[commit-bot: I haz the power, 2016-10-17 23:09:12]

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

[commit-bot: I haz the power, 2016-10-17 23:09:13]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[felt, 2016-10-17 23:16:02]

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

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

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-10-18 00:46:30]

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

[commit-bot: I haz the power, 2016-10-18 00:46:31]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2016-10-18 13:05:48]

LGTM but does OmniboxViewMac::GetSecureTextColor() need to be updated to handle
the new enum value (and color it as grey)?

[felt, 2016-10-18 13:16:29]

On 2016/10/18 13:05:48, rohitrao wrote:
> LGTM but does OmniboxViewMac::GetSecureTextColor() need to be updated to
handle
> the new enum value (and color it as grey)?

GetSecureTextColor shouldn't be reachable for either HTTP or HTTP_SHOW_WARNING.
It's only meant for coloring the scheme for secure sites. There's an existing
check in OmniboxViewMac::ApplyTextAttributes that accounts for HTTP_SHOW_WARNING
before GetSecureTextColor is invoked.

[felt, 2016-10-18 20:16:02]

The CQ bit was checked by felt@chromium.org

[felt, 2016-10-18 20:16:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
spqchan@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2422303002/#ps60001
(title: "Fix typo")

[commit-bot: I haz the power, 2016-10-18 20:16:24]

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-18 20:20:54]

Description was changed from

==========
Show HTTP-Bad security chip UI on Mac and Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Mac and Views. 

(Note that this will only display if the #mark-non-secure-as flag is
set, since the SecurityStateModel only emits HTTP_SHOW_WARNING when the
flag is set appropriately.)

BUG=647562
==========

to

==========
Show HTTP-Bad security chip UI on Mac and Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Mac and Views. 

(Note that this will only display if the #mark-non-secure-as flag is
set, since the SecurityStateModel only emits HTTP_SHOW_WARNING when the
flag is set appropriately.)

BUG=647562
==========

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-21 13:02:37]

Description was changed from

==========
Show HTTP-Bad security chip UI on Mac and Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Mac and Views. 

(Note that this will only display if the #mark-non-secure-as flag is
set, since the SecurityStateModel only emits HTTP_SHOW_WARNING when the
flag is set appropriately.)

BUG=647562
==========

to

==========
Show HTTP-Bad security chip UI on Mac and Views

This hooks up the HTTP_SHOW_WARNING state to the security chip in the
omnibox on Mac and Views.

(Note that this will only display if the #mark-non-secure-as flag is
set, since the SecurityStateModel only emits HTTP_SHOW_WARNING when the
flag is set appropriately.)

BUG=647562

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

[commit-bot: I haz the power, 2016-10-21 13:02:38]

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