Render extension URLs with chips

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_LOCATION_BAR_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_LOCATION_BAR_VIEW_H_
 
 #include <stddef.h>
 
 #include <string>
@@ skipping 59 matching lines @@
                         public gfx::AnimationDelegate,
                         public ChromeOmniboxEditController,
                         public DropdownBarHostDelegate,
                         public TemplateURLServiceObserver,
                         public zoom::ZoomEventManagerObserver {
  public:
   class Delegate {
    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;
 
     // Creates PageActionImageView. Caller gets an ownership.
     virtual PageActionImageView* CreatePageActionImageView(
         LocationBarView* owner,
         ExtensionAction* action) = 0;
 
     // Returns ContentSettingBubbleModelDelegate.
@@ skipping 227 matching lines @@
 
   bool ShouldShowKeywordBubble() const;
 
   // Returns true when the current page is explicitly secure or insecure.
   // In these cases, we should show the state of the security chip.
   bool ShouldShowSecurityChip() const;
 
   // 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
   // animation (|hide_url_animation_| if |hide| is true, |show_url_animation_|
   // if it's false) should be set to in order to produce the same omnibox
   // position.  This way we can stop the old animation, set the new animation to
   // this value, and start it running, and the text will appear to reverse
   // directions from its current location.
   double GetValueForAnimation(bool hide) const;
 
@@ skipping 135 matching lines @@
   // These allow toggling the verbose security state behavior via flags.
   bool should_show_secure_state_;
   bool should_show_nonsecure_state_;
   bool should_animate_secure_state_;
   bool should_animate_nonsecure_state_;
 
   DISALLOW_COPY_AND_ASSIGN(LocationBarView);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_LOCATION_BAR_LOCATION_BAR_VIEW_H_