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..cac429c34f81038a8dcd58289fa68ec35f3c8f9a 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -22,6 +22,7 @@
 #include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
+#include "chrome/browser/ssl/chrome_security_state_model_client.h"

[Peter Kasting, 2016/08/18 06:24:37]

Nit: I think none of these new #includes are necessary anymore.

[Kevin Bailey, 2016/08/18 16:09:36]

Done.

 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/translate/chrome_translate_client.h"
 #include "chrome/browser/translate/translate_service.h"
@@ -63,8 +64,10 @@
 #include "components/translate/core/browser/language_state.h"
 #include "components/zoom/zoom_controller.h"
 #include "components/zoom/zoom_event_manager.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/ssl_status.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/feature_switch.h"
 #include "grit/components_scaled_resources.h"
@@ -137,6 +140,7 @@ LocationBarView::LocationBarView(Browser* browser,
       size_animation_(this),
       is_popup_mode_(is_popup_mode),
       show_focus_rect_(false),
+      showing_security_chip_(false),
       template_url_service_(NULL),
       web_contents_null_at_last_refresh_(true) {
   edit_bookmarks_enabled_.Init(
@@ -326,10 +330,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 +362,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 +536,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 label = GetSecurityText();

[Peter Kasting, 2016/08/18 06:24:37]

Nit: |security_text| might be a better name.

[Kevin Bailey, 2016/08/18 16:09:36]

If you prefer. I think once we cross line 540, it's more interesting to the
reader that it will be a label.

+    leading_width +=
+        GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
+        location_icon_view_->GetMinimumSizeForLabelText(label).width();
   } else {
     leading_width += padding + location_icon_view_->GetMinimumSize().width();
   }
@@ -612,8 +616,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 +794,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 +806,16 @@ void LocationBarView::Update(const WebContents* contents) {
   else
     omnibox_view_->Update();
 
+  if (ShouldShowSecurityChip() != showing_security_chip_) {

[Peter Kasting, 2016/08/18 06:24:37]

If it is safe to rely on the animation calls being no-ops when the state isn't
changing (which is I think currently the case), we can simplify this. 
|showing_security_chip_| can be entirely removed.  We can rename
StartAnimation() and StopAnimation() to make it clearer they aren't always
called when the animation is stopped and started, respectively.  The result is
that the code here just becomes:

  if (ShouldShowSecurityChip())
    location_icon_view_->ShowSecurityChip();
  else
    location_icon_view_->HideSecurityChip();

[Kevin Bailey, 2016/08/18 16:09:36]

Done, but it looks like this whole thing needs to change again so, done for now.

+    if (ShouldShowSecurityChip()) {
+      showing_security_chip_ = true;
+      location_icon_view_->StartAnimation();
+    } else {
+      showing_security_chip_ = false;
+      location_icon_view_->StopAnimation();
+    }
+  }
+
   OnChanged();  // NOTE: Calls Layout().
 }
 
@@ -1038,6 +1052,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 +1067,20 @@ bool LocationBarView::ShouldShowEVBubble() const {
           security_state::SecurityStateModel::EV_SECURE);
 }
 
+bool LocationBarView::ShouldShowSecurityChip() const {
+  if (ShouldShowKeywordBubble())
+    return false;
+  security_state::SecurityStateModel::SecurityLevel level =
+      GetToolbarModel()->GetSecurityLevel(false);
+  if (level == security_state::SecurityStateModel::SecurityLevel::EV_SECURE ||

[Peter Kasting, 2016/08/18 06:24:37]

Nit: Could just

  return level == ... ||
         level == ... ||
         level == ...;

I suggest a "using security_state::SecurityStateModel::SecurityLevel;" inside
this function to reduce the verbosity of this statement.

[Kevin Bailey, 2016/08/18 16:09:36]

I can go as far as SecurityStateModel, so I don't know that it helps much.

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

Helps enough, probably.  A type alias might get you the rest of the way:

  using SecurityLevel = security_state::SecurityStateModel::SecurityLevel;

But as long as the conditional terms each fit on one line, you're probably good.

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

Done.

+      level == security_state::SecurityStateModel::SecurityLevel::SECURE ||
+      level ==
+          security_state::SecurityStateModel::SecurityLevel::SECURITY_ERROR) {

[Peter Kasting, 2016/08/18 06:24:37]

What about SECURITY_WARNING and SECURITY_POLICY_WARNING?

Maybe this should return true iff ToolbarModelImpl::GetSecureVerboseText()
returns a nonempty string (see also comments in .grd file).

[Kevin Bailey, 2016/08/18 16:09:36]

I asked spqchan why it wasn't handled in her CL and she said that, according to
Emily, we're not doing it yet. If the model simply isn't providing those enums
yet, then we could reduce this to 'level != NONE'.

+    return true;
+  }
+  return false;

[Peter Kasting, 2016/08/18 06:24:37]

Checking: Does the toolbar model return NONE any time we're editing, and thus
automatically hide this in that case?

If so, I don't think we need the ShouldShowKeywordBubble() check at the top
since that can only show when editing.

If not, then I think the chip can be showing while editing, which isn't right.

[Kevin Bailey, 2016/08/18 16:09:36]

If the user starts editing after things have settled, the level instantly drops
(according to my logs.) If I changed the above to:

if (ShouldShowKeywordBubble() || omnibox_view_->IsEditingOrEmpty()) return
false;

then, prior to it settling, it would be bouncing all over the place, which is
one reason I added the earlier state machine. So I think for this particular
case, we should leave it as is.

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

I don't understand this comment.

What does "prior to it settling, it would be bouncing around all over the place"
mean?  Can you describe the precise series of events you're thinking of?

I still don't understand how the ShouldShowKeywordBubble() check is relevant. 
The only way that something would "bounce around" without that conditional but
not with it is if we would otherwise be showing both the security chip and the
keyword bubble at the same time.  But if that's the case it seems like we
already have problems (as I was trying to say).

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

'omnibox_view_->IsEditingOrEmpty()' almost appears as an RNG in my log.
Therefore, to your original question, "Does the toolbar return NONE ...", I
don't believe that there's any correlation. It is neither, "If so..." nor "If
not...".

I am (now) not seeing 'ShouldShowKeywordBubble()' ever set, perhaps because
we're calling it in fewer places, so I'm taking it out.

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

ToolbarModel::GetSecurityLevel()'s argument is called |ignore_editing|.  If that
is false -- as it is here -- you can see from
https://cs.chromium.org/chromium/src/components/toolbar/toolbar_model_impl.cc...
that, when the omnibox is editing, we return NONE.

The reason you're getting confused is because you're looking at
IsEditingOrEmpty(), which also returns true when the omnibox is
empty-but-not-editing -- that is, on the New Tab Page.  The result of this is
not RNG; "editing" or "not editing and on NTP" are the only cases where it
should be true.

I think just removing the ShouldShowKeywordBubble() call was the right change
here.

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

You should see my log. The omnibox_view_ goes from empty to editing(partial
string) to editing(full string) back to empty and finally to not-editing(full
string). So, for example, if says that it's empty, it does not mean that we're
on a new tab.

Saying "RNG" is hyperbole but it's still unpredictable.

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

Since I don't know what you're looking at to determine empty versus editing, how
you're getting these strings, or what states you're monitoring, I can't tell you
what you should expect to see.  But I can tell you that the code is not
unpredictable, because the behavior visible to the user is not unpredictable.  I
would have to walk through things with you step by step to be sure we were on
the same page.

+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private LocationBar implementation: