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..36a5da9b46aff8206e5d36572a382bb8021d11c1 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -326,10 +326,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:
       return ui::MaterialDesignController::IsModeMaterial()
                  ? GetSecureTextColor(
-                       security_state::SecurityStateModel::EV_SECURE)
+                       GetToolbarModel()->GetSecurityLevel(false))
                  : SkColorSetRGB(7, 149, 0);
   }
   NOTREACHED();
@@ -358,7 +358,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);
     }
   }
   return color_utils::GetReadableColor(text_color, GetColor(BACKGROUND));
@@ -532,11 +532,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 +612,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 +790,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 +802,14 @@ void LocationBarView::Update(const WebContents* contents) {
   else
     omnibox_view_->Update();
 
+  if (!ShouldShowSecurityChip()) {
+    location_icon_view_->HideSecurityChip();
+  } else if (contents) {
+    location_icon_view_->ShowSecurityChip();
+  } else {
+    location_icon_view_->AnimateSecurityChip();
+  }

[Peter Kasting, 2016/08/26 20:59:12]

AnimateSecurityChip() is unclear, because it doesn't say whether we're showing
or hiding the chip.

I still think this API is not only more complicated but less clear than
something like:

Here:
  location_icon_view_->SetSecurityState(ShouldShowSecurityChip(), !contents);

In LocationIconView:
  // Triggers a security chip state change.  |showing| describes the desired new
  // state.  |updates_current_tab| is true if this is a change to the current
  // tab's state (as opposed to a switch to a different tab).
  LocationIconView::SetSecurityState(bool showing, bool updates_current_tab);

(You could also convert the second arg to an enum, which might be more clear, or
change the name or reverse the meaning of it.)

It's not just about having a smaller API surface, although that does matter. 
It's also about making the details of when and how the chip animates an
implementation detail inside LocationIconView, instead of having LocationBarView
control them.  LocationBarView is already extremely complicated, and the more
logic it can push into its children, the better.

[Kevin Bailey, 2016/08/29 13:38:10]

That would be trivially fixed by adopting Sarah's name, AnimateIn.
I'm sorry but you won't convince me that a parameter of '!contents' is more
clear than AnimateIn(). That you need enums to make it readable is proof of
this.
You don't move lines from one file to another simply to make them balanced. You
put lines where they belong. If something is too complicated, one can move the
logic to a subroutine, which we also could have done here.

The problem with smearing responsibility across classes is that it requires the
reader to understand the whole collection to understand any of it. It makes it
so that a tiny change here causes a problem over there. It makes the classes
unusable elsewhere. Smearing undermines decoupling.

I hope you like my version. I think it at least draws a line between these 2
classes.

+
   OnChanged();  // NOTE: Calls Layout().
 }
 
@@ -1038,6 +1046,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 +1061,13 @@ bool LocationBarView::ShouldShowEVBubble() const {
           security_state::SecurityStateModel::EV_SECURE);
 }
 
+bool LocationBarView::ShouldShowSecurityChip() const {
+  using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;
+  SecurityLevel level = GetToolbarModel()->GetSecurityLevel(false);
+  return level == SecurityLevel::SECURE || level == SecurityLevel::EV_SECURE ||
+         level == SecurityLevel::SECURITY_ERROR;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private LocationBar implementation: