Render extension URLs with chips

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

Index: chrome/browser/ui/views/location_bar/location_bar_view.h
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.h b/chrome/browser/ui/views/location_bar/location_bar_view.h
index 82fb098456682549969271da999e4042c19f8c32..3d6901f029b4e81b35af380e8e267045cab79500 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.h
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.h
@@ -77,6 +77,7 @@ class LocationBarView : public LocationBar,
    public:
     // Should return the current web contents.
     virtual content::WebContents* GetWebContents() = 0;
+    virtual const content::WebContents* GetWebContents() const = 0;

[Peter Kasting, 2016/12/14 22:47:07]

Not clear to me why this is useful, since your GetExtensionName() function takes
a non-const pointer.

[meacer, 2016/12/16 18:46:17]

LocationBarView::GetPreferredSize is a const method and it calls
GetExtensionName, so I can't use the non-const GetWebContents there. So it's to
keep the constness of the caller of GetExtensionName instead.

[Peter Kasting, 2016/12/20 01:59:31]

My point is, the const method you're adding returns a pointer-to-const.  But
GetExtensionName() requires a pointer-to-non-const.  If the const method here
were required, your code wouldn't compile, because the compiler would change
from complaining that GetWebContents() is non-const to complaining that the
argument to GetExtensionName() needs to be non-const.

What's actually happening, I think, is that the compiler is not calling this new
method at all.  It's calling the non-const method, and it can get away with that
because it's a method on |delegate_|, rather than a method on |this|, at the
time of the call in GetPreferredSize().  Because what |delegate_| points to, and
not the pointer itself, is being modified, that's legal.

So I strongly suspect this is unused, and if you remove it, everything will keep
working.

[meacer, 2016/12/21 01:38:56]

Done.

 
     virtual ToolbarModel* GetToolbarModel() = 0;
     virtual const ToolbarModel* GetToolbarModel() const = 0;
@@ -324,6 +325,10 @@ class LocationBarView : public LocationBar,
   // Returns true if the chip should be animated
   bool ShouldAnimateSecurityChip() const;
 
+  // Returns true when the current page is a chrome-extension:// or a chrome://
+  // URL.

[Peter Kasting, 2016/12/14 22:47:07]

This seems inaccurate since we don't return true for chrome:// URLs.

[meacer, 2016/12/16 18:46:18]

Removed and folded into ShouldShowSecurityText.

+  bool ShouldShowExtensionChip() const;

[Peter Kasting, 2016/12/14 22:47:07]

Rather than adding this, and then having to add duplicate code blocks for the
existing security chip stuff to handle the similar "extension chip" stuff, just
change the "security chip" concept to speak broadly about the location icon also
having text.  (This also starts to phase out "chip", which is a meaningless word
in our current presentation.)

GetSecurityText() -> GetLocationIconText()
ShouldShowSecurityChip() -> ShouldShowLocationIconText()
ShouldAnimateSecurityChip() -> ShouldAnimateLocationIconTextVisibilityChange()

etc.

This will force you to think about how we should animate state changes in/out of
showing the extension icon, to make sure ShouldAnimate...() returns the right
thing.  That's probably a good thing.  I haven't thought deeply enough to be
convinced the current patch does the right thing.

[meacer, 2016/12/16 18:46:17]

Sure, done. I checked with the UI folks and they said it's okay not to animate
the extension text (reason being the animation is used to draw attention when
the security state changes). I kept ShouldShowExtensionChip naming in the cocoa
code though, as they already distinguish between security and EV chips.

+
   // Used to "reverse" the URL showing/hiding animations, since we use separate
   // animations whose curves are not true inverses of each other.  Based on the
   // current position of the omnibox, calculates what value the desired