[Material] Update Material Security Verbose Decoration Flag

[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
Committed: https://crrev.com/d52dccd7192814d740a8bae50fcd32eb00816ce4
Cr-Commit-Position: refs/heads/master@{#424917}

[spqchan, 2016-09-30 23:10:50]

Description was changed from

==========
[Material][Mac] Update Material Security Verbose Decoration Flag

- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

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

to

==========
[Material][Mac] Update Material Security Verbose Decoration Flag

- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
==========

[spqchan, 2016-09-30 23:16:52]

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

[spqchan, 2016-09-30 23:16:52]

PTAL

[spqchan, 2016-09-30 23:32:24]

On 2016/09/30 23:16:52, spqchan wrote:
> PTAL

Sorry, please hold the CL. Need to add a few more things

[spqchan, 2016-10-03 18:36:26]

Description was changed from

==========
[Material][Mac] Update Material Security Verbose Decoration Flag

- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
==========

to

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
==========

[spqchan, 2016-10-03 18:37:48]

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

[spqchan, 2016-10-03 18:37:48]

PTAL

rsesek - for the cocoa code
krb - for the changes in Views and the flag name

Thanks!

[Kevin Bailey, 2016-10-04 15:10:17]

Not sure if you were ready, but I saw the enum get fixed.

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

https://codereview.chromium.org/2378623007/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:784: bool should_show
= ShouldShowSecurityChip();
You'll need Peter's approval for this file, but speaking for myself, I would add
a 'bool* should_animate' parameter to ShouldShowSecurityChip() and push this
logic down there. '!contents' can remain here.

[Robert Sesek, 2016-10-04 17:16:52]

cocoa/ LGTM

[spqchan, 2016-10-05 01:39:49]

PTAL

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

https://codereview.chromium.org/2378623007/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/location_bar_view.cc:784: bool should_show
= ShouldShowSecurityChip();
On 2016/10/04 15:10:17, Kevin Bailey wrote:
> You'll need Peter's approval for this file, but speaking for myself, I would
add
> a 'bool* should_animate' parameter to ShouldShowSecurityChip() and push this
> logic down there. '!contents' can remain here.

Sounds good, I'll add pkasting. I wanted to get a pass from you first. I added
the param. I'm not too sure about about it since ShouldShowSecurityChip also
gets called with GetPreferredSize(), in which is doesn't need to know if it's
animating or not, but I don't have strong feelings about it.

[spqchan, 2016-10-06 16:59:13]

On 2016/10/05 01:39:49, spqchan wrote:
> PTAL
> 
>
https://codereview.chromium.org/2378623007/diff/60001/chrome/browser/ui/views...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
https://codereview.chromium.org/2378623007/diff/60001/chrome/browser/ui/views...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:784: bool
should_show
> = ShouldShowSecurityChip();
> On 2016/10/04 15:10:17, Kevin Bailey wrote:
> > You'll need Peter's approval for this file, but speaking for myself, I would
> add
> > a 'bool* should_animate' parameter to ShouldShowSecurityChip() and push this
> > logic down there. '!contents' can remain here.
> 
> Sounds good, I'll add pkasting. I wanted to get a pass from you first. I added
> the param. I'm not too sure about about it since ShouldShowSecurityChip also
> gets called with GetPreferredSize(), in which is doesn't need to know if it's
> animating or not, but I don't have strong feelings about it.

ping

[Kevin Bailey, 2016-10-06 17:23:23]

Sorry, didn't see the change go by. Looks good to me. I agree the 'nullptr' is a
bit odd, but I like the way that 'ShouldShowSecurityChip()' bundles things up.

[spqchan, 2016-10-06 23:12:33]

On 2016/10/06 17:23:23, Kevin Bailey wrote:
> Sorry, didn't see the change go by. Looks good to me. I agree the 'nullptr' is
a
> bit odd, but I like the way that 'ShouldShowSecurityChip()' bundles things up.

Thanks! It looks like pkasting is OOO until Oct 9. Do you have an alternate goto
reviewer? Otherwise, I can go through owners list

[spqchan, 2016-10-07 00:31:05]

Description was changed from

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
==========

to

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682, 653602
==========

[Kevin Bailey, 2016-10-07 01:34:42]

On 2016/10/06 23:12:33, spqchan wrote:
> On 2016/10/06 17:23:23, Kevin Bailey wrote:
> > Sorry, didn't see the change go by. Looks good to me. I agree the 'nullptr'
is
> a
> > bit odd, but I like the way that 'ShouldShowSecurityChip()' bundles things
up.
> 
> Thanks! It looks like pkasting is OOO until Oct 9. Do you have an alternate
goto
> reviewer? Otherwise, I can go through owners list

Sorry, no, Peter owns views/location_bar. msw and sky own views/, or just wait
until Mon.

[spqchan, 2016-10-07 18:10:11]

On 2016/10/07 01:34:42, Kevin Bailey wrote:
> On 2016/10/06 23:12:33, spqchan wrote:
> > On 2016/10/06 17:23:23, Kevin Bailey wrote:
> > > Sorry, didn't see the change go by. Looks good to me. I agree the
'nullptr'
> is
> > a
> > > bit odd, but I like the way that 'ShouldShowSecurityChip()' bundles things
> up.
> > 
> > Thanks! It looks like pkasting is OOO until Oct 9. Do you have an alternate
> goto
> > reviewer? Otherwise, I can go through owners list
> 
> Sorry, no, Peter owns views/location_bar. msw and sky own views/, or just wait
> until Mon.

I see, thanks for letting me know! I'll wait until Monday then

[spqchan, 2016-10-07 18:10:27]

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

[spqchan, 2016-10-07 18:10:28]

PTAL

[spqchan, 2016-10-07 18:11:08]

+pkasting for Views

[Peter Kasting, 2016-10-10 21:39:21]

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:329: // should_animate
is not null, then set the value to true if the chip should
Nit: "...|should_animate| is non-null, sets it to whether the chip should be
animated."

(Adds ||, changes imperative to declarative verb, shortens)

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:331: bool
ShouldShowSecurityChip(bool* should_animate) const;
I think it might make more sense to have separate ShouldShowSecurityChip() and
ShouldAnimateSecurityChip() functions.

Semantically, I'm not really a big fan of functions that are named ComputeXXX()
and then take an outparam that is set to the result of some computation YYY that
is different from XXX.  I realize that it doesn't make sense to animate
something that isn't showing, but that just means the second function can be
implemented in terms of the first.  (We need not be more clever about reducing
duplicated work, since ShouldShow is simple to compute and won't be a perf
bottleneck.)

This would allow callers to avoid passing nullptr to signal that they don't
care, and let the caller that does care just do:

  location_icon_view_->SetSecurityState(
      ShouldShowSecurityChip(), !contents && ShouldAnimateSecurityChip());

...no temps needed.

https://codereview.chromium.org/2378623007/diff/120001/chrome/common/chrome_s...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/2378623007/diff/120001/chrome/common/chrome_s...
chrome/common/chrome_switches.h:191: extern const char
kSecurityVerboseDecorationShowAllWithOnlyNonSecureAnimated[];
Nit: This name is kinda reaching ridiculous length.  It's resulting in crazy
wrapping at usage sites.  Let's look for shorter names.

One improvement would be to say SecurityChip in place of
SecurityVerboseDecoration.  Neither is terribly meaningful on its own, but the
former is the language we use in various comments and email threads.

Another might be to split this into two flags.  What are we showing, and what
are we animating?  Showing looks to be "all" or "non-secure".  Animating looks
to be "all", "none", or "only non-secure", with the last meaning the same as
"all" if "showing" is set to "non-secure".

This second suggestion also dovetails nicely with my API proposal elsewhere.

[Kevin Bailey, 2016-10-11 00:44:05]

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:331: bool
ShouldShowSecurityChip(bool* should_animate) const;
On 2016/10/10 21:39:21, Peter Kasting (very slow) wrote:
> I think it might make more sense to have separate ShouldShowSecurityChip() and
> ShouldAnimateSecurityChip() functions.
> 
> Semantically, I'm not really a big fan of functions that are named
ComputeXXX()
> and then take an outparam that is set to the result of some computation YYY
that
> is different from XXX.  I realize that it doesn't make sense to animate
> something that isn't showing, but that just means the second function can be
> implemented in terms of the first.  (We need not be more clever about reducing
> duplicated work, since ShouldShow is simple to compute and won't be a perf
> bottleneck.)
> 
> This would allow callers to avoid passing nullptr to signal that they don't
> care, and let the caller that does care just do:
> 
>   location_icon_view_->SetSecurityState(
>       ShouldShowSecurityChip(), !contents && ShouldAnimateSecurityChip());
> 
> ...no temps needed.

If GetSecurityLevel() were more stable, I would completely agree. Unfortunately,
it seems to change at the drop of a hat. Aren't you concerned with it returning
different values to each caller? Granted, it's only displaying a chip, but it
seems a little harsh to suffer incorrect behavior for a style preference.

A simpler way to eliminate the 'nullptr' is with a default argument.

[Peter Kasting, 2016-10-11 00:54:18]

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:331: bool
ShouldShowSecurityChip(bool* should_animate) const;
On 2016/10/11 00:44:05, Kevin Bailey wrote:
> On 2016/10/10 21:39:21, Peter Kasting (very slow) wrote:
> > I think it might make more sense to have separate ShouldShowSecurityChip()
and
> > ShouldAnimateSecurityChip() functions.
> > 
> > Semantically, I'm not really a big fan of functions that are named
> ComputeXXX()
> > and then take an outparam that is set to the result of some computation YYY
> that
> > is different from XXX.  I realize that it doesn't make sense to animate
> > something that isn't showing, but that just means the second function can be
> > implemented in terms of the first.  (We need not be more clever about
reducing
> > duplicated work, since ShouldShow is simple to compute and won't be a perf
> > bottleneck.)
> > 
> > This would allow callers to avoid passing nullptr to signal that they don't
> > care, and let the caller that does care just do:
> > 
> >   location_icon_view_->SetSecurityState(
> >       ShouldShowSecurityChip(), !contents && ShouldAnimateSecurityChip());
> > 
> > ...no temps needed.
> 
> If GetSecurityLevel() were more stable, I would completely agree.
Unfortunately,
> it seems to change at the drop of a hat. Aren't you concerned with it
returning
> different values to each caller?

No.

If we thought this was problematic, such that we wanted to bake it into the API,
I would suggest changing the name to something that makes it clear we're
fetching both pieces of state (and maybe return a tuple).  I don't think it's
problematic, though.  This is a private method being used in few places, and all
this code, being UI code, is run on the same thread.  The answer to whether the
security state should be shown will change at definable points, i.e. not
suddenly between two adjacent lines in the caller.

> Granted, it's only displaying a chip, but it
> seems a little harsh to suffer incorrect behavior for a style preference.
> 
> A simpler way to eliminate the 'nullptr' is with a default argument.

That would indeed help with verbosity at the other callsites, but not with the
munging of separate (dependent) concepts into one function.

[spqchan, 2016-10-11 17:26:02]

Description was changed from

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682, 653602
==========

to

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
==========

[spqchan, 2016-10-11 17:32:58]

PTAL

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/2378623007/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.h:329: // should_animate
is not null, then set the value to true if the chip should
On 2016/10/10 21:39:21, Peter Kasting (very slow) wrote:
> Nit: "...|should_animate| is non-null, sets it to whether the chip should be
> animated."
> 
> (Adds ||, changes imperative to declarative verb, shortens)

Updated the comment

https://codereview.chromium.org/2378623007/diff/120001/chrome/common/chrome_s...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/2378623007/diff/120001/chrome/common/chrome_s...
chrome/common/chrome_switches.h:191: extern const char
kSecurityVerboseDecorationShowAllWithOnlyNonSecureAnimated[];
On 2016/10/10 21:39:21, Peter Kasting (very slow) wrote:
> Nit: This name is kinda reaching ridiculous length.  It's resulting in crazy
> wrapping at usage sites.  Let's look for shorter names.
> 
> One improvement would be to say SecurityChip in place of
> SecurityVerboseDecoration.  Neither is terribly meaningful on its own, but the
> former is the language we use in various comments and email threads.
> 
> Another might be to split this into two flags.  What are we showing, and what
> are we animating?  Showing looks to be "all" or "non-secure".  Animating looks
> to be "all", "none", or "only non-secure", with the last meaning the same as
> "all" if "showing" is set to "non-secure".
> 
> This second suggestion also dovetails nicely with my API proposal elsewhere.

Agreed, it's getting ridiculous. I split the flag and renamed it.

[Peter Kasting, 2016-10-11 22:14:50]

LGTM

https://codereview.chromium.org/2378623007/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2378623007/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1018: 
Nit: Any particular reason you added this blank line?

[spqchan, 2016-10-12 20:25:47]

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

[spqchan, 2016-10-12 20:25:48]

thanks!

+ isherman for histogram.xml

https://codereview.chromium.org/2378623007/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/2378623007/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1018: 
On 2016/10/11 22:14:50, Peter Kasting (very slow) wrote:
> Nit: Any particular reason you added this blank line?

Whoops, removed

[Ilya Sherman, 2016-10-12 20:45:34]

histograms.xml lgtm % a comment:

https://codereview.chromium.org/2378623007/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2378623007/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:88655: -  <int value="-361948582"
label="material-security-verbose"/>
Please keep this value in place, i.e. please revert this diff.

[spqchan, 2016-10-12 20:54:58]

thanks!

https://codereview.chromium.org/2378623007/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/2378623007/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:88655: -  <int value="-361948582"
label="material-security-verbose"/>
On 2016/10/12 20:45:34, Ilya Sherman wrote:
> Please keep this value in place, i.e. please revert this diff.

Done.

[spqchan, 2016-10-12 20:55:03]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-10-12 20:55:03]

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

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

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-12 20:57:57]

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

[commit-bot: I haz the power, 2016-10-12 20:57:58]

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-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2016-10-12 23:14:20]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-10-12 23:14:21]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
isherman@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2378623007/#ps220001
(title: "rebased")

[commit-bot: I haz the power, 2016-10-12 23:14:55]

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-12 23:18:17]

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

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

Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[spqchan, 2016-10-12 23:26:34]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-10-12 23:26:34]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
isherman@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2378623007/#ps240001
(title: "Rebased again")

[commit-bot: I haz the power, 2016-10-12 23:27:06]

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-13 00:16:45]

Description was changed from

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
==========

to

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
==========

[commit-bot: I haz the power, 2016-10-13 00:16:47]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-10-13 00:21:16]

Description was changed from

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682
==========

to

==========
[Material] Update Material Security Verbose Decoration Flag

- Removed "Material" from the flag's name
- Added the option "Show all, only animate nonsecure"
- Set the default flag value to "Show all, non-animated"

BUG=651991, 649682

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

[commit-bot: I haz the power, 2016-10-13 00:21:18]

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