New location security strings and animation.

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

Index: chrome/browser/ui/views/location_bar/location_bar_view.cc
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc
index 7ffa3e9e7d73a16df4e74ffa17c4b9c1c0b02f7e..99876e4df91ac45c6b698f0aff3b46e0e222b8a8 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -138,7 +138,9 @@ LocationBarView::LocationBarView(Browser* browser,
       is_popup_mode_(is_popup_mode),
       show_focus_rect_(false),
       template_url_service_(NULL),
-      web_contents_null_at_last_refresh_(true) {
+      web_contents_null_at_last_refresh_(true),
+      previous_security_level_(
+          security_state::SecurityStateModel::SecurityLevel::NONE) {
   edit_bookmarks_enabled_.Init(
       bookmarks::prefs::kEditBookmarksEnabled, profile->GetPrefs(),
       base::Bind(&LocationBarView::UpdateWithoutTabRestore,
@@ -326,10 +328,10 @@ SkColor LocationBarView::GetColor(
     case DEEMPHASIZED_TEXT:
       return color_utils::AlphaBlend(GetColor(TEXT), GetColor(BACKGROUND), 128);
 
-    case EV_BUBBLE_TEXT_AND_BORDER:
+    case SECURITY_CHIP_TEXT_AND_BORDER:
       return ui::MaterialDesignController::IsModeMaterial()
                  ? GetSecureTextColor(
-                       security_state::SecurityStateModel::EV_SECURE)
+                       GetToolbarModel()->GetSecurityLevel(false))
                  : SkColorSetRGB(7, 149, 0);
   }
   NOTREACHED();
@@ -358,7 +360,7 @@ SkColor LocationBarView::GetSecureTextColor(
                security_state::SecurityStateModel::SECURITY_ERROR) {
       text_color = SkColorSetRGB(162, 0, 0);
     } else {
-      text_color = GetColor(EV_BUBBLE_TEXT_AND_BORDER);
+      text_color = GetColor(SECURITY_CHIP_TEXT_AND_BORDER);
     }
   }
   return color_utils::GetReadableColor(text_color, GetColor(BACKGROUND));
@@ -532,11 +534,11 @@ gfx::Size LocationBarView::GetPreferredSize() const {
   int leading_width = edge_thickness;
   if (ShouldShowKeywordBubble()) {
     // The selected keyword view can collapse completely.
-  } else if (ShouldShowEVBubble()) {
-    leading_width += GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
-                     location_icon_view_->GetMinimumSizeForLabelText(
-                                            GetToolbarModel()->GetEVCertName())
-                         .width();
+  } else if (ShouldShowSecurityChip()) {
+    base::string16 security_text = GetSecurityText();
+    leading_width +=
+        GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
+        location_icon_view_->GetMinimumSizeForLabelText(security_text).width();
   } else {
     leading_width += padding + location_icon_view_->GetMinimumSize().width();
   }
@@ -612,8 +614,8 @@ void LocationBarView::Layout() {
         selected_keyword_view_->set_is_extension_icon(false);
       }
     }
-  } else if (ShouldShowEVBubble()) {
-    location_icon_view_->SetLabel(GetToolbarModel()->GetEVCertName());
+  } else if (ShouldShowSecurityChip()) {
+    location_icon_view_->SetLabel(GetSecurityText());
     location_icon_view_->SetBackground(true);
     // The largest fraction of the omnibox that can be taken by the EV bubble.
     const double kMaxBubbleFraction = 0.5;
@@ -790,7 +792,7 @@ void LocationBarView::Update(const WebContents* contents) {
   RefreshTranslateIcon();
   RefreshSaveCreditCardIconView();
   RefreshManagePasswordsIconView();
-  content::WebContents* web_contents_for_sub_views =
+  WebContents* web_contents_for_sub_views =
       GetToolbarModel()->input_in_progress() ? nullptr : GetWebContents();
   open_pdf_in_reader_view_->Update(web_contents_for_sub_views);
 
@@ -802,6 +804,22 @@ void LocationBarView::Update(const WebContents* contents) {
   else
     omnibox_view_->Update();
 
+  if (ShouldShowSecurityChip()) {
+    security_state::SecurityStateModel::SecurityLevel level =
+        GetToolbarModel()->GetSecurityLevel(false);
+    if (level == security_state::SecurityStateModel::SecurityLevel::EV_SECURE)
+      level = security_state::SecurityStateModel::SecurityLevel::SECURE;
+    if (previous_security_level_ != level) {
+      previous_security_level_ = level;
+      location_icon_view_->AnimateSecurityChip();
+    } else if (!location_icon_view_->IsAnimatingSecurityChip()) {
+      location_icon_view_->ShowSecurityChip();
+    }

[Peter Kasting, 2016/08/20 01:16:43]

I don't understand why this whole complicated block is needed.  What are you
trying to solve?  Does this have something to do with handling toggling between
tabs that are supposed to have different security states showing?  If so that
shouldn't be based on looking at the previous security state, it should be based
on whether this Update() is begin called for a tab switch or an update to the
current tab's state, which you can tell by whether |contents| is non-null.

Something like:

// Here, replacing this whole added chunk
  location_icon_view_->SetSecurityChipState(ShouldShowSecurityChip(),
!contents);

// In LocationIconView, replacing all the new methods there
  void SetSecurityChipState(bool showing, bool animate);

[Kevin Bailey, 2016/08/22 15:58:56]

Correct. I assumed that you saw this in:

codereview.chromium.org/2119033002/diff/600001/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm

lines 621 on.
As I mentioned in a previous comment, I've never seen 'web_contents' not be
null, except at start-up.

[Peter Kasting, 2016/08/22 22:58:09]

I left a similar comment there.  THanks for pointing me at it.
If |contents| was always non-null here, we'd always call
omnibox_view_->OnTabChanged(contents); above, and that in turn would cause
noticeable glitches in the omnibox, e.g. reverting your edits when the
underlying page updated.

If you look at
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?rcl=1471879...
, which is the thing that will indirectly call this, we only set the argument to
non-null when |should_restore_state| is true.  Checking the callers of that
function, they all pass false except ActiveTabChanged().

So I think you must have been looking at the wrong thing, or the wrong time, or
something.  Relying on whether this argument is null or not should be reliable.

[Kevin Bailey, 2016/08/23 15:17:13]

When the user presses back arrow or forwards, contents is null, and we have to
compare security levels to know what to do. The security model can transition
straight from secure to secure in this case. (I suspect we're on the same page;
just pointing out all these cases we need to handle.)

[Peter Kasting, 2016/08/23 18:01:57]

Yes, because that's an update to the state of the existing tab.
We don't need to look at the old level, and neither does the location icon view.
 All we need do is decide what the new state should be.  The animation handles
the fact that, if we're already in that state, we don't need to animate.

[Kevin Bailey, 2016/08/23 20:01:39]

I'm not sure who "we" is, but LocationBarView::Update() definitely needs to look
at level. For example, your previous level is SECURITY_ERROR, and animation is
done. Your new level is SECURE. How do you decide to animate without knowing the
previous level?

If all you're proposing is, "Let's reduce ::Animate ::Show ::Hide etc. to a
single function with 2 parameters." that's fine, but personally I detest candy
machine interfaces.

Please advise.

[Peter Kasting, 2016/08/23 20:11:00]

You always tell the icon to animate (or snap, if this is a tab switch) to the
state you want.  The animation object itself will convert this to a no-op if you
e.g. call Show() when we're already animating open.

[Kevin Bailey, 2016/08/23 20:39:25]

The UI designers want it such that, if you go from one level to the same level,
do not animate. I believe it is to avoid UI churn. This is why I believe we need
to look at the previous level.

[Peter Kasting, 2016/08/23 20:42:20]

I'm confused.  That's what you should get with my proposal, no?  That's why I
mentioned the no-oping in the animation layer.

[groby-ooo-7-16, 2016/08/23 22:11:15]

I think the confusion here stems from a previous comment: "We don't need to look
at the old level, and neither does the location icon view."

But as I understand it, we do, to avoid unnecessary animation. E.g. if you
navigate back/forth in the same tab, between https sites, we do not want to
animate the icon. So somewhere we need to keep track of the current state of the
icon. I.e.:

LocationIconView::SetState(State security_state, bool animate) {
 if (security_state == current_state_)
    return;
  
  if (!animate)
    SetState(state);
  else
    AnimateTo(state);
  
  ...
}

I think that's what everybody in this debate is actually saying, and we're
trying to read between each others lines. Peter: Is that what you had in mind?

[Peter Kasting, 2016/08/23 22:17:18]

No.  We don't need to track the previous level.  If you try to animate to a
state you're already at, the animation will automatically do nothing.  In other
words, if you took the sample code block you have above, and remove the initial
conditional check that early returns, the behavior will be the exact same as
with the check.

I think people are assuming that if you call SlideAnimation::Show(), it causes
the animation to run from 0 to 1.  But it doesn't.  It runs from whatever the
current value is to 1.  When the current value is 1, that means nothing happens.
 When the animation is already in the process of showing, it doesn't change
anything about that.  Only if the animation is stopped at a value other than the
target, or in the midst of animating the wrong direction, will the call have any
effect.

So all these comments have been me trying to say "rely on this".

[groby-ooo-7-16, 2016/08/23 23:12:40]

Thank you for clarifying that! That does cover the current behavior, I think.

Alas, UX currently calls for re-animating when states change.  From the spec:
"Navigating from one verbose state connection to another (e.g. secure to
malware) there would be an animation to transition between icons and text."

I'd prefer we figure that one out in a follow-up CL, though.

[Peter Kasting, 2016/08/23 23:19:15]

OK, that contradicts Kevin's earlier comment.

I would say let's get the CL in without that and we can decide whether to do it,
what the animation should look like, and how to implement that all later.

+  } else {
+    previous_security_level_ = GetToolbarModel()->GetSecurityLevel(false);
+    location_icon_view_->HideSecurityChip();
+  }
+
   OnChanged();  // NOTE: Calls Layout().
 }
 
@@ -1038,6 +1056,11 @@ bool LocationBarView::HasValidSuggestText() const {
       !suggested_text_view_->size().IsEmpty();
 }
 
+base::string16 LocationBarView::GetSecurityText() const {
+  return ShouldShowEVBubble() ? GetToolbarModel()->GetEVCertName()
+                              : GetToolbarModel()->GetSecureVerboseText();
+}
+
 bool LocationBarView::ShouldShowKeywordBubble() const {
   return !omnibox_view_->model()->keyword().empty() &&
       !omnibox_view_->model()->is_keyword_hint();
@@ -1048,6 +1071,17 @@ bool LocationBarView::ShouldShowEVBubble() const {
           security_state::SecurityStateModel::EV_SECURE);
 }
 
+bool LocationBarView::ShouldShowSecurityChip() const {
+  if (ShouldShowKeywordBubble())
+    return false;
+  using security_state::SecurityStateModel;
+  SecurityStateModel::SecurityLevel level =
+      GetToolbarModel()->GetSecurityLevel(false);
+  return level == SecurityStateModel::SecurityLevel::SECURE ||
+         level == SecurityStateModel::SecurityLevel::EV_SECURE ||
+         level == SecurityStateModel::SecurityLevel::SECURITY_ERROR;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private LocationBar implementation: