[Material] Update Material Security Verbose Decoration Flag

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_LOCATION_BAR_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_LOCATION_BAR_VIEW_H_
 
 #include <stddef.h>
 
 #include <string>
@@ skipping 307 matching lines @@
   bool HasValidSuggestText() const;
 
   // Returns text describing the URL's security level, to be placed in the
   // security chip.
   base::string16 GetSecurityText() const;
 
   bool ShouldShowKeywordBubble() const;
   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
   // current position of the omnibox, calculates what value the desired
   // animation (|hide_url_animation_| if |hide| is true, |show_url_animation_|
   // if it's false) should be set to in order to produce the same omnibox
   // position.  This way we can stop the old animation, set the new animation to
   // this value, and start it running, and the text will appear to reverse
   // directions from its current location.
   double GetValueForAnimation(bool hide) const;
@@ skipping 133 matching lines @@
   // Tracks this preference to determine whether bookmark editing is allowed.
   BooleanPrefMember edit_bookmarks_enabled_;
 
   // This is a debug state variable that stores if the WebContents was null
   // during the last RefreshPageAction.
   bool web_contents_null_at_last_refresh_;
 
   // 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);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_LOCATION_BAR_VIEW_H_