[Material] Update Material Security Verbose Decoration Flag

chrome/browser/ui/views/location_bar/location_bar_view.h

Index: chrome/browser/ui/views/location_bar/location_bar_view.h
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.h b/chrome/browser/ui/views/location_bar/location_bar_view.h
index b20c54f74a5ad531ffffc264dcd3d527e6b9d284..612a6fd3959884ad7f58d09e019ecea4e8cd92f8 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.h
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.h
@@ -325,8 +325,10 @@ class LocationBarView : public LocationBar,
   bool ShouldShowEVBubble() const;
 
   // Returns true when the current page is explicitly secure or insecure.
-  // In these cases, we should show the state of the security chip.
-  bool ShouldShowSecurityChip() const;
+  // In these cases, we should show the state of the security chip. If
+  // should_animate is not null, then set the value to true if the chip should

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

Nit: "...|should_animate| is non-null, sets it to whether the chip should be
animated."

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

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

Updated the comment

+  // be animated.
+  bool ShouldShowSecurityChip(bool* should_animate) const;

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

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.

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

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]

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.
That would indeed help with verbosity at the other callsites, but not with the
munging of separate (dependent) concepts into one function.

 
   // Used to "reverse" the URL showing/hiding animations, since we use separate
   // animations whose curves are not true inverses of each other.  Based on the
@@ -480,6 +482,7 @@ class LocationBarView : public LocationBar,
   // These allow toggling the verbose security state behavior via flags.
   bool should_show_secure_state_;
   bool should_animate_secure_state_;
+  bool should_animate_nonsecure_state_;
 
   DISALLOW_COPY_AND_ASSIGN(LocationBarView);
 };