[Material][Mac] Implement Omnibox Verbose State Chips

chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm

Index: chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
diff --git a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
index 81d7668b84f7c13a8fa018b7d7b75a3d88c1e196..ebd9899db89c991c85f6d25835eb43af3cc6de8e 100644
--- a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
+++ b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
@@ -34,12 +34,12 @@
 #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.h"
 #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h"
 #import "chrome/browser/ui/cocoa/location_bar/content_setting_decoration.h"
-#import "chrome/browser/ui/cocoa/location_bar/ev_bubble_decoration.h"
 #import "chrome/browser/ui/cocoa/location_bar/keyword_hint_decoration.h"
 #import "chrome/browser/ui/cocoa/location_bar/location_icon_decoration.h"
 #import "chrome/browser/ui/cocoa/location_bar/manage_passwords_decoration.h"
 #import "chrome/browser/ui/cocoa/location_bar/page_action_decoration.h"
 #import "chrome/browser/ui/cocoa/location_bar/save_credit_card_decoration.h"
+#import "chrome/browser/ui/cocoa/location_bar/security_state_bubble_decoration.h"
 #import "chrome/browser/ui/cocoa/location_bar/selected_keyword_decoration.h"
 #import "chrome/browser/ui/cocoa/location_bar/star_decoration.h"
 #import "chrome/browser/ui/cocoa/location_bar/translate_decoration.h"
@@ -57,7 +57,6 @@
 #include "components/prefs/pref_service.h"
 #include "components/search_engines/template_url.h"
 #include "components/search_engines/template_url_service.h"
-#include "components/security_state/security_state_model.h"
 #include "components/translate/core/browser/language_state.h"
 #include "components/zoom/zoom_controller.h"
 #include "components/zoom/zoom_event_manager.h"
@@ -87,6 +86,9 @@ const static int kFirstRunBubbleYOffset = 1;
 
 const int kDefaultIconSize = 16;
 
+// The minimum width the URL should have for the verbose state to be shown.
+const int kMinURLWidth = 120;
+
 // Color of the vector graphic icons when the location bar is dark.
 // SkColorSetARGB(0xCC, 0xFF, 0xFF 0xFF);
 const SkColor kMaterialDarkVectorIconColor = SK_ColorWHITE;
@@ -106,8 +108,9 @@ LocationBarViewMac::LocationBarViewMac(AutocompleteTextField* field,
       field_(field),
       location_icon_decoration_(new LocationIconDecoration(this)),
       selected_keyword_decoration_(new SelectedKeywordDecoration()),
-      ev_bubble_decoration_(
-          new EVBubbleDecoration(location_icon_decoration_.get())),
+      security_state_bubble_decoration_(
+          new SecurityStateBubbleDecoration(location_icon_decoration_.get(),
+                                            this)),
       save_credit_card_decoration_(
           new SaveCreditCardDecoration(command_updater)),
       star_decoration_(new StarDecoration(command_updater)),
@@ -392,8 +395,9 @@ NSPoint LocationBarViewMac::GetManagePasswordsBubblePoint() const {
 }
 
 NSPoint LocationBarViewMac::GetPageInfoBubblePoint() const {
-  if (ev_bubble_decoration_->IsVisible()) {
-    return [field_ bubblePointForDecoration:ev_bubble_decoration_.get()];
+  if (security_state_bubble_decoration_->IsVisible()) {
+    return [field_
+        bubblePointForDecoration:security_state_bubble_decoration_.get()];
   } else {
     return [field_ bubblePointForDecoration:location_icon_decoration_.get()];
   }
@@ -422,7 +426,7 @@ void LocationBarViewMac::Layout() {
   [cell clearDecorations];
   [cell addLeftDecoration:location_icon_decoration_.get()];
   [cell addLeftDecoration:selected_keyword_decoration_.get()];
-  [cell addLeftDecoration:ev_bubble_decoration_.get()];
+  [cell addLeftDecoration:security_state_bubble_decoration_.get()];
   [cell addRightDecoration:star_decoration_.get()];
   [cell addRightDecoration:translate_decoration_.get()];
   [cell addRightDecoration:zoom_decoration_.get()];
@@ -445,9 +449,13 @@ void LocationBarViewMac::Layout() {
   // By default only the location icon is visible.
   location_icon_decoration_->SetVisible(true);
   selected_keyword_decoration_->SetVisible(false);
-  ev_bubble_decoration_->SetVisible(false);
   keyword_hint_decoration_->SetVisible(false);
 
+  if (omnibox_view_->IsEditingOrEmpty())
+    security_state_bubble_decoration_->ResetAndHide();
+  else
+    security_state_bubble_decoration_->SetVisible(false);
+
   // Get the keyword to use for keyword-search and hinting.
   const base::string16 keyword = omnibox_view_->model()->keyword();
   base::string16 short_name;
@@ -468,17 +476,41 @@ void LocationBarViewMac::Layout() {
     // Design we need to set its color, which we cannot do until we know the
     // theme (by being installed in a browser window).
     selected_keyword_decoration_->SetImage(GetKeywordImage(keyword));
+  } else if (!keyword.empty() && is_keyword_hint) {
+    keyword_hint_decoration_->SetKeyword(short_name, is_extension_keyword);
+    keyword_hint_decoration_->SetVisible(true);
   } else if (ShouldShowEVBubble()) {
     // Switch from location icon to show the EV bubble instead.
     location_icon_decoration_->SetVisible(false);
-    ev_bubble_decoration_->SetVisible(true);
+    security_state_bubble_decoration_->SetVisible(true);
 
     base::string16 label(GetToolbarModel()->GetEVCertName());
-    ev_bubble_decoration_->SetFullLabel(base::SysUTF16ToNSString(label));
-  } else if (!keyword.empty() && is_keyword_hint) {
-    keyword_hint_decoration_->SetKeyword(short_name,
-                                         is_extension_keyword);
-    keyword_hint_decoration_->SetVisible(true);
+    security_state_bubble_decoration_->SetFullLabel(
+        base::SysUTF16ToNSString(label));
+  } else if (ShouldShowSecurityState()) {
+    bool is_security_state_visible = true;
+    CGFloat available_width =
+        [cell availableWidthInFrame:[[cell controlView] frame]];
+
+    // If there's not enough space, animate out the security state bubble.
+    // Show it until it has finished animating out.
+    if (available_width < kMinURLWidth) {
+      is_security_state_visible =
+          security_state_bubble_decoration_->AnimatingOut();
+      if (security_state_bubble_decoration_->HasAnimatedIn())
+        security_state_bubble_decoration_->AnimateOut();
+    }
+
+    // Don't change the label if the bubble is in the process of animating
+    // out the old one.
+    base::string16 label(GetToolbarModel()->GetSecurityVerboseText());
+    if (!security_state_bubble_decoration_->AnimatingOut()) {
+      security_state_bubble_decoration_->SetFullLabel(
+          base::SysUTF16ToNSString(label));
+    }
+
+    location_icon_decoration_->SetVisible(!is_security_state_visible);
+    security_state_bubble_decoration_->SetVisible(is_security_state_visible);
   }
 
   // These need to change anytime the layout changes.
@@ -567,32 +599,10 @@ void LocationBarViewMac::UpdateWithoutTabRestore() {
 }
 
 void LocationBarViewMac::UpdateLocationIcon() {
-  bool in_dark_mode = IsLocationBarDark();
-
-  SkColor vector_icon_color = gfx::kPlaceholderColor;
-  gfx::VectorIconId vector_icon_id = gfx::VectorIconId::VECTOR_ICON_NONE;
-  if (ShouldShowEVBubble()) {
-    vector_icon_id = gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID;
-    vector_icon_color = gfx::kGoogleGreen700;
-  } else {
-    vector_icon_id = omnibox_view_->GetVectorIcon();
-    security_state::SecurityStateModel::SecurityLevel security_level =
-        GetToolbarModel()->GetSecurityLevel(false);
-    if (security_level == security_state::SecurityStateModel::NONE) {
-      vector_icon_color = gfx::kChromeIconGrey;
-    } else {
-      NSColor* sRGBColor =
-          OmniboxViewMac::GetSecureTextColor(security_level, in_dark_mode);
-      NSColor* deviceColor =
-          [sRGBColor colorUsingColorSpace:[NSColorSpace deviceRGBColorSpace]];
-      vector_icon_color = skia::NSDeviceColorToSkColor(deviceColor);
-    }
-  }
-
-  // If the theme is dark, then the color should always be
-  // kMaterialDarkVectorIconColor.
-  if (in_dark_mode)
-    vector_icon_color = kMaterialDarkVectorIconColor;
+  SkColor vector_icon_color = GetLocationBarIconColor();
+  gfx::VectorIconId vector_icon_id =
+      ShouldShowEVBubble() ? gfx::VectorIconId::LOCATION_BAR_HTTPS_VALID
+                           : omnibox_view_->GetVectorIcon();
 
   DCHECK(vector_icon_id != gfx::VectorIconId::VECTOR_ICON_NONE);
   NSImage* image = NSImageFromImageSkiaWithColorSpace(
@@ -600,8 +610,29 @@ void LocationBarViewMac::UpdateLocationIcon() {
                             vector_icon_color),
       base::mac::GetSRGBColorSpace());
   location_icon_decoration_->SetImage(image);
-  ev_bubble_decoration_->SetImage(image);
+  security_state_bubble_decoration_->SetImage(image);
+  security_state_bubble_decoration_->SetLabelColor(vector_icon_color);
+
   Layout();
+
+  // If the security level has changed, check if the verbose state decoration
+  // needs to be animated. If we need to show it, animate it in. Otherwise,
+  // animate it out if it's already fully displayed.

[Peter Kasting, 2016/08/22 22:39:42]

This isn't the right way to do things (sorry, krb just pointed me at this, I
didn't look before).

I believe you're trying to handle toggling between tabs that have different
security states showing.  If so, then instead of comparing new security level to
current (which is not only complicated, but incorrect if we switch from an
animating tab to a different tab that has the same level -- the animation should
snap open, but it will continue running instead), you should check whether this
update is the result of a tab switch or an up[date within the current tab.

At least on views, this distinction is obvious:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/location_bar/loc...
Here, if Update() is called with a null pointer, this is an update to the
current tab.  If not, it is a tab change.

Fixing this should allow you to greatly simplify the APIs involved here, because
instead of animatein/out and hasanimatedin/out, you can just do a single "set
state to(state, bool animate)" method.

[Kevin Bailey, 2016/08/23 14:10:41]

I'm not sure it simplifies things. First, when the user switches tabs, what it
really should do is restore the state of that other tab. If that tab was
animating, it should pick up where it left off. Since the odds of a user
catching a previous animation are nil, we can just instantly show or hide but
it's technically cheating.

Second, 'has_tab_changed' is just another condition added to the mix:

if (has_tab_changed) {
  if (ShouldShowSecurityState())
    Show();
  else
    Hide();
} else {
  // level change detection logic
}

Sure, you can push it all to a subroutine but I wouldn't call that
simplification.

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

It's not cheating, for two reasons.

First, the animation is not really an inherent property of the tab, it's just a
UI affordance to help indicate that something has changed about the state within
this tab.  When the user manually switches tabs, there's no reason to show that
affordance.

Second, if we ignore that and treat the animation as a property of the tab, the
total animation time is 150 ms.  If we were already in the middle of the
animation when the user changed tabs away, they would have to complete the tab
switch away and then back in less than whatever is left of the 150 ms, and we'd
need to pick up where that left us.  That's extremely unlikely -- it probably
takes a good 100 ms just to punch the next tab + previous tab shortcuts -- and
the remaining animation time would be so short that it would be a few pixels of
movement.  That's more likely to look glitchy than intentional.

So I think we get a better result, overall, from not trying to resume the
animation in this case.
It reduces the class' API surface by collapsing four methods to one.  Even if we
only hide the complexity in the implementation instead of exposing it in the
interface, that's still a win.  We isolate the logic that animates from the
logic that decides what state we should be in, effectively separating semantics
from presentation, and the API surface is simpler.

Also, we entirely omit the idea of looking at the previous state and deciding if
we've changed.  The implementation, on the callee side, basically becomes:

  if (!animate)
    Reset(show ? 1 : 0);
  else if (show)
    Show();
  else
    Hide();

+  security_state::SecurityStateModel::SecurityLevel new_security_level =
+      GetToolbarModel()->GetSecurityLevel(false);
+  bool is_secure_to_secure = IsSecureConnection(new_security_level) &&
+                             IsSecureConnection(security_level_);
+  if (security_level_ != new_security_level && !is_secure_to_secure) {
+    if (ShouldShowSecurityState())
+      security_state_bubble_decoration_->AnimateIn();
+    else if (security_state_bubble_decoration_->HasAnimatedIn())
+      security_state_bubble_decoration_->AnimateOut();
+  } else if (security_state_bubble_decoration_->HasAnimatedOut()) {
+    // Animate the verbose back in if it was animated out.
+    if (ShouldShowSecurityState())
+      security_state_bubble_decoration_->AnimateIn(false);
+  }
+  security_level_ = new_security_level;
 }
 
 void LocationBarViewMac::UpdateColorsToMatchTheme() {
@@ -636,7 +667,7 @@ void LocationBarViewMac::OnChanged() {
     const int resource_id = omnibox_view_->GetIcon();
     NSImage* image = OmniboxViewMac::ImageForResource(resource_id);
     location_icon_decoration_->SetImage(image);
-    ev_bubble_decoration_->SetImage(image);
+    security_state_bubble_decoration_->SetImage(image);
     Layout();
     return;
   }
@@ -664,6 +695,20 @@ bool LocationBarViewMac::ShouldShowEVBubble() const {
           security_state::SecurityStateModel::EV_SECURE);
 }
 
+bool LocationBarViewMac::ShouldShowSecurityState() const {
+  security_state::SecurityStateModel::SecurityLevel security =
+      GetToolbarModel()->GetSecurityLevel(false);
+  bool has_verbose_for_security =
+      IsSecureConnection(security) ||
+      security == security_state::SecurityStateModel::SECURITY_ERROR;
+
+  return ui::MaterialDesignController::IsModeMaterial() &&
+         (has_verbose_for_security ||
+          security_state_bubble_decoration_->AnimatingOut()) &&
+         !omnibox_view_->IsEditingOrEmpty() &&
+         !omnibox_view_->model()->is_keyword_hint();
+}
+
 bool LocationBarViewMac::IsLocationBarDark() const {
   return [[field_ window] inIncognitoModeWithSystemTheme];
 }
@@ -689,6 +734,27 @@ NSImage* LocationBarViewMac::GetKeywordImage(const base::string16& keyword) {
   return OmniboxViewMac::ImageForResource(IDR_OMNIBOX_SEARCH);
 }
 
+SkColor LocationBarViewMac::GetLocationBarIconColor() const {
+  bool in_dark_mode = IsLocationBarDark();
+  if (in_dark_mode)
+    return kMaterialDarkVectorIconColor;
+
+  if (ShouldShowEVBubble())
+    return gfx::kGoogleGreen700;
+
+  security_state::SecurityStateModel::SecurityLevel security_level =
+      GetToolbarModel()->GetSecurityLevel(false);
+
+  if (security_level == security_state::SecurityStateModel::NONE)
+    return gfx::kChromeIconGrey;
+
+  NSColor* sRGB_color =

[Robert Sesek, 2016/08/18 14:43:23]

naming: all lowercase

[spqchan, 2016/08/18 18:37:18]

Done.

+      OmniboxViewMac::GetSecureTextColor(security_level, in_dark_mode);
+  NSColor* device_color =
+      [sRGB_color colorUsingColorSpace:[NSColorSpace deviceRGBColorSpace]];
+  return skia::NSDeviceColorToSkColor(device_color);
+}
+
 void LocationBarViewMac::PostNotification(NSString* notification) {
   [[NSNotificationCenter defaultCenter] postNotificationName:notification
                                         object:[NSValue valueWithPointer:this]];
@@ -823,6 +889,12 @@ bool LocationBarViewMac::UpdateZoomDecoration(bool default_zoom_changed) {
       IsLocationBarDark());
 }
 
+bool LocationBarViewMac::IsSecureConnection(
+    security_state::SecurityStateModel::SecurityLevel level) const {
+  return level == security_state::SecurityStateModel::SECURE ||
+         level == security_state::SecurityStateModel::EV_SECURE;
+}
+
 void LocationBarViewMac::UpdateAccessibilityViewPosition(
     LocationBarDecoration* decoration) {
   if (!decoration->IsVisible())
@@ -839,7 +911,7 @@ std::vector<LocationBarDecoration*> LocationBarViewMac::GetDecorations() {
   // are page actions and the keyword hint.
   decorations.push_back(location_icon_decoration_.get());
   decorations.push_back(selected_keyword_decoration_.get());
-  decorations.push_back(ev_bubble_decoration_.get());
+  decorations.push_back(security_state_bubble_decoration_.get());
   decorations.push_back(save_credit_card_decoration_.get());
   decorations.push_back(star_decoration_.get());
   decorations.push_back(translate_decoration_.get());