[Material] Show "Dangerous" for Safe Browsing Security State

[Material] Show "Dangerous" for Safe Browsing Security State

BUG=644353
Committed: https://crrev.com/9cb8fb393e8af43e4961d3aaff549138508d0966
Cr-Commit-Position: refs/heads/master@{#418388}

[spqchan, 2016-09-12 18:59:11]

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

[spqchan, 2016-09-12 18:59:11]

PTAL

[spqchan, 2016-09-12 18:59:25]

Description was changed from

==========
[Material] Show "Dangerous" for Safe Browsering Security State

BUG=644353
==========

to

==========
[Material] Show "Dangerous" for Safe Browsing Security State

BUG=644353
==========

[Peter Kasting, 2016-09-13 05:55:59]

LGTM

https://codereview.chromium.org/2331133002/diff/1/chrome/browser/ui/toolbar/c...
File chrome/browser/ui/toolbar/chrome_toolbar_model_delegate.cc (right):

https://codereview.chromium.org/2331133002/diff/1/chrome/browser/ui/toolbar/c...
chrome/browser/ui/toolbar/chrome_toolbar_model_delegate.cc:97: //
initialization), assume it did not failed.
Nit: If there's no WebContents, there's no current page, so nothing _can_ have
failed.  So maybe:

  // During toolbar initialization there may be no active WebContents.
  content::WebContents* web_contents = GetActiveWebContents();
  return web_contents &&
         ChromeSecurityStateModelClient::FromWebContents(web_contents)
             ->GetSecurityInfo().fails_malware_check;

https://codereview.chromium.org/2331133002/diff/1/components/omnibox_strings....
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/2331133002/diff/1/components/omnibox_strings....
components/omnibox_strings.grdp:22: <message name="IDS_DANGEROUS_VERBOSE_STATE"
desc="Text for the Dangerous Omnibox Verbose State.">
Nit: For all these messages, can we add a brief description or example of when
each state is shown and what it's trying to convey?  I'm worried that
translators will not be certain how to shade meaning between e.g. "Not Secure"
versus "Dangerous".

https://codereview.chromium.org/2331133002/diff/1/components/toolbar/toolbar_...
File components/toolbar/toolbar_model_delegate.h (right):

https://codereview.chromium.org/2331133002/diff/1/components/toolbar/toolbar_...
components/toolbar/toolbar_model_delegate.h:47: virtual bool
GetFailsMalwareCheck() const = 0;
Nit: Just call this FailsMalwareCheck().

https://codereview.chromium.org/2331133002/diff/1/components/toolbar/toolbar_...
File components/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/2331133002/diff/1/components/toolbar/toolbar_...
components/toolbar/toolbar_model_impl.cc:139: :
l10n_util::GetStringUTF16(IDS_NOT_SECURE_VERBOSE_STATE);
Nit: Move ?: inside call to GetStringUTF16()

[spqchan, 2016-09-13 21:23:41]

thanks!

https://codereview.chromium.org/2331133002/diff/1/chrome/browser/ui/toolbar/c...
File chrome/browser/ui/toolbar/chrome_toolbar_model_delegate.cc (right):

https://codereview.chromium.org/2331133002/diff/1/chrome/browser/ui/toolbar/c...
chrome/browser/ui/toolbar/chrome_toolbar_model_delegate.cc:97: //
initialization), assume it did not failed.
On 2016/09/13 05:55:59, Peter Kasting wrote:
> Nit: If there's no WebContents, there's no current page, so nothing _can_ have
> failed.  So maybe:
> 
>   // During toolbar initialization there may be no active WebContents.
>   content::WebContents* web_contents = GetActiveWebContents();
>   return web_contents &&
>          ChromeSecurityStateModelClient::FromWebContents(web_contents)
>              ->GetSecurityInfo().fails_malware_check;

Done.

https://codereview.chromium.org/2331133002/diff/1/components/omnibox_strings....
File components/omnibox_strings.grdp (right):

https://codereview.chromium.org/2331133002/diff/1/components/omnibox_strings....
components/omnibox_strings.grdp:22: <message name="IDS_DANGEROUS_VERBOSE_STATE"
desc="Text for the Dangerous Omnibox Verbose State.">
On 2016/09/13 05:55:59, Peter Kasting wrote:
> Nit: For all these messages, can we add a brief description or example of when
> each state is shown and what it's trying to convey?  I'm worried that
> translators will not be certain how to shade meaning between e.g. "Not Secure"
> versus "Dangerous".

Done.

https://codereview.chromium.org/2331133002/diff/1/components/toolbar/toolbar_...
File components/toolbar/toolbar_model_delegate.h (right):

https://codereview.chromium.org/2331133002/diff/1/components/toolbar/toolbar_...
components/toolbar/toolbar_model_delegate.h:47: virtual bool
GetFailsMalwareCheck() const = 0;
On 2016/09/13 05:55:59, Peter Kasting wrote:
> Nit: Just call this FailsMalwareCheck().

Done.

https://codereview.chromium.org/2331133002/diff/1/components/toolbar/toolbar_...
File components/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/2331133002/diff/1/components/toolbar/toolbar_...
components/toolbar/toolbar_model_impl.cc:139: :
l10n_util::GetStringUTF16(IDS_NOT_SECURE_VERBOSE_STATE);
On 2016/09/13 05:55:59, Peter Kasting wrote:
> Nit: Move ?: inside call to GetStringUTF16()

Done.

[spqchan, 2016-09-13 21:23:45]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-09-13 21:23:45]

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

[commit-bot: I haz the power, 2016-09-13 21:24:10]

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-09-13 22:14:25]

Description was changed from

==========
[Material] Show "Dangerous" for Safe Browsing Security State

BUG=644353
==========

to

==========
[Material] Show "Dangerous" for Safe Browsing Security State

BUG=644353
==========

[commit-bot: I haz the power, 2016-09-13 22:14:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-13 22:17:05]

Description was changed from

==========
[Material] Show "Dangerous" for Safe Browsing Security State

BUG=644353
==========

to

==========
[Material] Show "Dangerous" for Safe Browsing Security State

BUG=644353

Committed: https://crrev.com/9cb8fb393e8af43e4961d3aaff549138508d0966
Cr-Commit-Position: refs/heads/master@{#418388}
==========

[commit-bot: I haz the power, 2016-09-13 22:17:06]

Patchset 2 (id:??) landed as
https://crrev.com/9cb8fb393e8af43e4961d3aaff549138508d0966
Cr-Commit-Position: refs/heads/master@{#418388}