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..c98104187cd70d2e59d319e000e6c3f1ec9fd3eb 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"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/translate/chrome_translate_client.h"
 #include "chrome/browser/translate/translate_service.h"
@@ -63,12 +64,15 @@
 #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"
 #include "grit/theme_resources.h"
+#include "third_party/re2/src/re2/re2.h"
 #include "ui/accessibility/ax_view_state.h"
 #include "ui/base/dragdrop/drag_drop_types.h"
 #include "ui/base/material_design/material_design_controller.h"
@@ -137,6 +141,7 @@ LocationBarView::LocationBarView(Browser* browser,
       size_animation_(this),
       is_popup_mode_(is_popup_mode),
       show_focus_rect_(false),
+      load_state_(new LoadState()),

[Peter Kasting, 2016/08/15 23:29:49]

If we always have a non-null |load_state_|, why use a scoped_ptr to begin with? 
Just use a member-by-value.

[Kevin Bailey, 2016/08/16 16:39:36]

The scoped_ptr is to free it, and having it a new'd structure at all is to get
around the const methods. If our policy is to use mutable here, I'm happy to
change it.

[Peter Kasting, 2016/08/16 18:15:05]

If an object represents an internal cache rather than state that is directly or
indirectly visible to external observers, use mutable.  If the object doesn't
fit those constraints, the method should not be const.

       template_url_service_(NULL),
       web_contents_null_at_last_refresh_(true) {
   edit_bookmarks_enabled_.Init(
@@ -329,7 +334,7 @@ SkColor LocationBarView::GetColor(
     case EV_BUBBLE_TEXT_AND_BORDER:
       return ui::MaterialDesignController::IsModeMaterial()
                  ? GetSecureTextColor(
-                       security_state::SecurityStateModel::EV_SECURE)
+                       GetToolbarModel()->GetSecurityLevel(false))

[Peter Kasting, 2016/08/15 23:29:49]

This is necessary only if we use the "EV bubble" for more cases than just EV. 
In that case, it (and all related constants/enums/classes) should be renamed.

[Kevin Bailey, 2016/08/16 16:39:36]

Sorry, I'm not understanding the comment. The caller is simply asking for what
color the EV bubble should be. Now we must return red in some cases.

[Peter Kasting, 2016/08/16 18:15:05]

You're changing the meaning of "EV bubble" to "verbose security bubble".  It's
not for "display EV cert name", it's for "display some verbose security
information".

[Kevin Bailey, 2016/08/17 16:33:30]

Ok, all I noticed was this enum.

                  : SkColorSetRGB(7, 149, 0);
   }
   NOTREACHED();
@@ -344,18 +349,22 @@ SkColor LocationBarView::GetSecureTextColor(
   }
 
   SkColor text_color = GetColor(TEXT);
-  if ((security_level == security_state::SecurityStateModel::EV_SECURE) ||
+  if ((security_level == security_state::SecurityStateModel::NONE) ||

[Peter Kasting, 2016/08/15 23:29:49]

Simpler than all the changes you've made here is to just add to the top of the
function:

  if (security_level == security_state::SecurityStateModel::NONE)
    security_level = security_state::SecurityStateModel::SECURITY_ERROR;

(Though this implies that maybe the toolbar model itself should change instead.)

But I didn't think we were yet showing warnings about HTTP pages, so I'm
confused as to why we want this.

[Kevin Bailey, 2016/08/16 16:39:36]

Done.
I'm not entirely sure what you're implying but I chatted with groby@ about
something like this and we agreed that this week's UI decisions shouldn't affect
the security model, which should instead be more accurate than it is now.
In a previous comment, you wrote, "I thought we wanted to be verbose about HTTP
too?  Or is that a later stage?" and I think jdonnely mentioned it too. The doc
says, "Eventually 'Insecure'." I'll double-check.

+      (security_level == security_state::SecurityStateModel::EV_SECURE) ||
       (security_level == security_state::SecurityStateModel::SECURE) ||
       (security_level == security_state::SecurityStateModel::SECURITY_ERROR)) {
     if (ui::MaterialDesignController::IsModeMaterial()) {
       if (color_utils::IsDark(GetColor(BACKGROUND)))
         return text_color;
-      if (security_level == security_state::SecurityStateModel::SECURITY_ERROR)
+      if ((security_level == security_state::SecurityStateModel::NONE) ||
+          (security_level ==
+           security_state::SecurityStateModel::SECURITY_ERROR))
         text_color = gfx::kGoogleRed700;
       else
         text_color = gfx::kGoogleGreen700;
-    } else if (security_level ==
-               security_state::SecurityStateModel::SECURITY_ERROR) {
+    } else if ((security_level == security_state::SecurityStateModel::NONE) ||
+               (security_level ==
+                security_state::SecurityStateModel::SECURITY_ERROR)) {
       text_color = SkColorSetRGB(162, 0, 0);
     } else {
       text_color = GetColor(EV_BUBBLE_TEXT_AND_BORDER);
@@ -532,12 +541,13 @@ 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();
+    leading_width +=
+        GetLayoutConstant(LOCATION_BAR_BUBBLE_HORIZONTAL_PADDING) +
+        location_icon_view_->GetMinimumSizeForLabelText(label).width();
   } else {
+    location_icon_view_->StopAnimation();
     leading_width += padding + location_icon_view_->GetMinimumSize().width();
   }
 
@@ -612,15 +622,17 @@ 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);
+    location_icon_view_->StartAnimation();

[Peter Kasting, 2016/08/15 23:29:49]

Don't start and stop animations from Layout().  Layout() can be called any
number of times at any point in time (whether or not it is today).  Trigger
animation changes directly from some sort of listener or handler that is called
when the corresponding state changes.

[Kevin Bailey, 2016/08/16 16:39:36]

I had hoped to use this strategy, but, for example, the NavigationEntry
notification comes way too early, and requires us to monitor the WebContents
anyways. I think this will require some of the other pieces to get re-plumbed.

     // The largest fraction of the omnibox that can be taken by the EV bubble.
     const double kMaxBubbleFraction = 0.5;
     leading_decorations.AddDecoration(
         vertical_padding, location_height, false, kMaxBubbleFraction,
         bubble_horizontal_padding, item_padding, location_icon_view_);
   } else {
+    location_icon_view_->StopAnimation();
     leading_decorations.AddDecoration(vertical_padding, location_height,
                                       location_icon_view_);
   }
@@ -790,7 +802,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);
 
@@ -1038,6 +1050,11 @@ bool LocationBarView::HasValidSuggestText() const {
       !suggested_text_view_->size().IsEmpty();
 }
 
+base::string16 LocationBarView::GetSecurityText() const {
+  return ShouldShowEVBubble() ? GetToolbarModel()->GetEVCertName()

[Peter Kasting, 2016/08/15 23:29:49]

Nit: This is now the only caller of this function, and the function is simple;
I'd inline it here.

[Kevin Bailey, 2016/08/16 16:39:36]

The Mac code should use it. That's where I borrowed it from.

+                              : GetToolbarModel()->GetSecureVerboseText();
+}
+
 bool LocationBarView::ShouldShowKeywordBubble() const {
   return !omnibox_view_->model()->keyword().empty() &&
       !omnibox_view_->model()->is_keyword_hint();
@@ -1048,6 +1065,60 @@ bool LocationBarView::ShouldShowEVBubble() const {
           security_state::SecurityStateModel::EV_SECURE);
 }
 
+std::string LocationBarView::TrimSite(const std::string& url) const {
+  std::string prefix, site;
+  if (RE2::FullMatch(url, "http(s?)://([^/]*)((/?).*)", &prefix, &site))

[Peter Kasting, 2016/08/15 23:29:49]

Please don't process things with a regex.

What are you trying to do here?  It looks like you want to return the hostname
of the site.  If so, just use the GURL::host() for the actual site we're
currently on.  This should, I think, be omnibox_view_->model()->PermanentURL().

[Kevin Bailey, 2016/08/16 16:39:36]

Done.

+    return site;
+  else
+    return url;
+}
+
+bool LocationBarView::ShouldShowSecurityChip() const {
+  WebContents* web_contents = delegate_->GetWebContents();
+  if (!web_contents)
+    return false;
+  content::NavigationEntry* entry =
+      web_contents->GetController().GetVisibleEntry();
+  if (omnibox_view_->IsEditingOrEmpty()) {
+    load_state_->last_returned_ = false;
+    return false;
+  }
+  std::string site(UTF16ToUTF8(omnibox_view_->GetText()));
+  site = TrimSite(site);
+  // If URL does not have same site.
+  if (TrimSite(entry->GetURL().spec()) != site) {
+    load_state_->last_returned_ = false;
+    return false;
+  }
+  if (site == load_state_->last_site_) {
+    if (load_state_->last_index_ !=
+        web_contents->GetController().GetLastCommittedEntryIndex()) {
+      // New page, same site.
+      load_state_->last_index_ =
+          web_contents->GetController().GetLastCommittedEntryIndex();
+      // Return false if last was true, so it animates.
+      load_state_->last_returned_ = !load_state_->last_returned_;

[Peter Kasting, 2016/08/15 23:29:49]

I don't understand this logic.  It means each time we do a navigation we'll
change whether we want to show or hide the animation.

In general, the core problem here is that this function doesn't just calculate
if the UI should be showing, it actually toggles things.  This means calling the
function too many or too few times changes how the UI appears.  This isn't the
right way to handle state changes.  Instead a ShouldShowXXX() function should
return the same thing if you call it repeatedly.

This is tied to my comments about not kicking off animations from Layout().  You
want to isolate the event that should trigger an animation show or hide, then
add listener code that kicks off the animation from that point.  ShouldShowXXX()
should simply return whether we're before or after that point.

It seems like the core problem here is that you can't figure out how to tell
when the right navigation state changes happen and you're hacking around it. 
Don't do that.  Get creis or other nav controller people to handhold you through
doing the right thing.

You may wish to look back in version history for how we implemented the Origin
Chip and refer to some of that code.

[Kevin Bailey, 2016/08/16 16:39:36]

btw, it does return the same thing (if other state doesn't change.) This
particular path will update 'last_index_' and not fall through here next time.
Will do.

+      return load_state_->last_returned_;
+    } else {

[Peter Kasting, 2016/08/15 23:29:49]

Nit: No else after return.

Anyway here you could have just done:

      ...
      load_state_->last_returned_ = !load_state_->last_returned_;
    } else {
      load_state_->last_returned_ = true;
    }
    return load_state_->last_returned_;

[Kevin Bailey, 2016/08/16 16:39:36]

Done, and more.

+      load_state_->last_returned_ = true;
+      return true;
+    }
+  }
+  if (load_state_->last_site_ != site &&
+      load_state_->last_index_ !=
+          web_contents->GetController().GetLastCommittedEntryIndex()) {

[Peter Kasting, 2016/08/15 23:29:49]

Nit:

  } else if (load_state_->last_index_ !=
                 web_contents->GetController().GetLastCommittedEntryIndex()) {

[Kevin Bailey, 2016/08/16 16:39:36]

Done.

+    // New page, new site.
+    load_state_->last_site_ = site;
+    load_state_->last_index_ =
+        web_contents->GetController().GetLastCommittedEntryIndex();
+    // Return false if last was true, so it animates.
+    load_state_->last_returned_ = !load_state_->last_returned_;
+    return load_state_->last_returned_;
+  }
+  load_state_->last_returned_ = false;
+  return false;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, private LocationBar implementation: