[SiteChip] Draw site chip icon and site title. Drag support.

chrome/browser/ui/views/toolbar/site_chip_view.cc

Index: chrome/browser/ui/views/toolbar/site_chip_view.cc
diff --git a/chrome/browser/ui/views/toolbar/site_chip_view.cc b/chrome/browser/ui/views/toolbar/site_chip_view.cc
index 0e1f862548a0aa21e1e1f01c8aaf583a03656dc6..a20c560747591e9a36bf7605b74d7dc8b132641d 100644
--- a/chrome/browser/ui/views/toolbar/site_chip_view.cc
+++ b/chrome/browser/ui/views/toolbar/site_chip_view.cc
@@ -4,9 +4,15 @@
 
 #include "chrome/browser/ui/views/toolbar/site_chip_view.h"
 
+#include "base/files/file_path.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/extensions/extension_icon_image.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "chrome/browser/favicon/favicon_tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
 #include "chrome/browser/themes/theme_properties.h"
@@ -15,25 +21,157 @@
 #include "chrome/browser/ui/toolbar/toolbar_model.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/toolbar/toolbar_view.h"
+#include "chrome/common/extensions/extension_constants.h"
+#include "chrome/common/extensions/manifest_handlers/icons_handler.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/common/constants.h"
 #include "grit/theme_resources.h"
 #include "net/base/net_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
 #include "ui/views/background.h"
+#include "ui/views/button_drag_utils.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/label_button_border.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/painter.h"
 
-const int kEdgeThickness = 4;
-const int kIconTextSpacing = 4;
-const int kTrailingLabelMargin = 2;
+
+// SiteChipExtensionIcon ------------------------------------------------------
+
+class SiteChipExtensionIcon : public extensions::IconImage::Observer {
+ public:
+  SiteChipExtensionIcon(LocationIconView* icon_view,
+                        Profile* profile,
+                        const extensions::Extension* extension);
+  ~SiteChipExtensionIcon();
+
+  // IconImage::Observer:
+  virtual void OnExtensionIconImageChanged(
+      extensions::IconImage* image) OVERRIDE;
+
+ private:
+  LocationIconView* icon_view_;
+  scoped_ptr<extensions::IconImage> icon_image_;
+
+  DISALLOW_COPY_AND_ASSIGN(SiteChipExtensionIcon);
+};
+
+SiteChipExtensionIcon::SiteChipExtensionIcon(
+    LocationIconView* icon_view,
+    Profile* profile,
+    const extensions::Extension* extension)
+    : icon_view_(icon_view),
+      icon_image_(new extensions::IconImage(
+          profile,
+          extension,
+          extensions::IconsInfo::GetIcons(extension),
+          extension_misc::EXTENSION_ICON_BITTY,
+          extensions::IconsInfo::GetDefaultAppIcon(),
+          this)) {
+  // Forces load of the image.
+  icon_image_->image_skia().GetRepresentation(1.0f);
+
+  if (!icon_image_->image_skia().image_reps().empty())
+    OnExtensionIconImageChanged(icon_image_.get());
+}
+
+SiteChipExtensionIcon::~SiteChipExtensionIcon() {
+}
+
+void SiteChipExtensionIcon::OnExtensionIconImageChanged(
+    extensions::IconImage* image) {
+  if (icon_view_)
+    icon_view_->SetImage(&icon_image_->image_skia());
+}
+
+
+// SiteChipView ---------------------------------------------------------------
+
+namespace {
+
+const int kEdgeThickness = 5;
+const int k16x16IconExtraSpacing = 3;

[Peter Kasting, 2013/12/10 03:28:39]

Nit: Change this to two constants for now (a before and after extension icon);
that way you can easily mock up my proposal (1 px before and 2 after), even if
you don't check it in as part of this patch.

[Greg Billock, 2013/12/10 17:37:59]

Done.

+const int kIconTextSpacing = 3;
+const int kTrailingLabelMargin = 0;
+
+}  // namespace
+
+string16 SiteChipView::SiteLabelFromURL(const GURL& url) {
+  // The NTP.
+  if (!url.is_valid())
+    return string16(UTF8ToUTF16("Chrome"));
+
+  // TODO(gbillock): for kChromeUIScheme and kAboutScheme, return the title of
+  // the page.
+  // See url_constants.cc for hosts. ?? Or just show "Chrome"?
+  if (url.SchemeIs(chrome::kChromeUIScheme) ||
+      url.SchemeIs(chrome::kAboutScheme)) {
+    return string16(UTF8ToUTF16("Chrome"));
+  }
+
+  // For file: urls, return the BaseName of the file       .

[Peter Kasting, 2013/12/10 03:28:39]

Nit: Lots of spaces before period?

[Greg Billock, 2013/12/10 17:37:59]

Done.

+  if (url.SchemeIsFile()) {
+    base::FilePath path = base::FilePath::FromUTF8Unsafe(url.path());
+    // TODO(gbillock): Need nuance here.
+    return string16(base::UTF8ToUTF16(path.BaseName().value()));
+  }
+
+  // TODO(gbillock): Handle filesystem urls the same way?
+  // Also: should handle interstitials differently?
+
+  // TODO(gbillock): think about view-source?
+
+  Profile* profile = toolbar_view_->browser()->profile();
+
+  // For chrome-extension urls, return the extension name.
+  if (url.SchemeIs(extensions::kExtensionScheme)) {
+    ExtensionService* service =
+        extensions::ExtensionSystem::Get(profile)->extension_service();
+    const extensions::Extension* extension =
+        service->extensions()->GetExtensionOrAppByURL(url);
+    return extension ? base::UTF8ToUTF16(extension->name()) :

[Peter Kasting, 2013/12/10 03:28:39]

Nit: break after '?' instead of ':'

[Greg Billock, 2013/12/10 17:37:59]

Done.

+        UTF8ToUTF16(url.host());
+  }
+
+  if (url.SchemeIsHTTPOrHTTPS()) {
+    // See ToolbarModelImpl::GetText(). Does not pay attention to any user
+    // edits, and uses GetURL/net::FormatUrl -- We don't really care about
+    // length or the autocomplete parser.
+    std::string languages;
+    if (profile)
+      languages = profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
+
+    base::string16 formatted = net::FormatUrl(url.GetOrigin(), languages,

[Peter Kasting, 2013/12/10 03:28:39]

I still think this should just be the host, run through StripWWW() and
IDNToUnicode().  Not only is that shorter, it's actually more accurate to the
intent of the code.  We don't want the site chip to "match how the omnibox
formats URLs" so much as we explicitly want "the hostname (in Unicode), sans
'www.'".  So calling FormatURL() is a very roundabout way of doing what we want,
and in theory could break someday (though I can't imagine why it would).

[Greg Billock, 2013/12/10 17:37:59]

I agree this is conservative. Let's keep it for this change and fix it up later.
That gives us a baseline we know is identical to the display logic we have now.

[Peter Kasting, 2013/12/10 19:44:31]

OK I guess, but consider at least adding a TODO about simplifying this.

+        net::kFormatUrlOmitAll, net::UnescapeRule::NORMAL, NULL, NULL, NULL);
+    // Remove scheme, "www.", and trailing "/".
+    if (StartsWith(formatted, ASCIIToUTF16("http://"), false))
+      formatted = formatted.substr(7);
+    if (StartsWith(formatted, ASCIIToUTF16("https://"), false))
+      formatted = formatted.substr(8);
+    if (StartsWith(formatted, ASCIIToUTF16("www."), false))
+      formatted = formatted.substr(4);
+    if (EndsWith(formatted, ASCIIToUTF16("/"), false))
+      formatted = formatted.substr(0, formatted.size()-1);
+    return formatted;
+  }
+
+  // For FTP, prepend "ftp:" to hostname.
+  if (url.SchemeIs(content::kFtpScheme)) {
+    return string16(UTF8ToUTF16(std::string("ftp:") + url.host()));
+  }
+
+  // If all else fails, return hostname.
+  return string16(UTF8ToUTF16(url.host()));
+}
 
 SiteChipView::SiteChipView(ToolbarView* toolbar_view)
     : ToolbarButton(this, NULL),
-      toolbar_view_(toolbar_view) {
+      toolbar_view_(toolbar_view),
+      painter_(NULL),
+      showing_16x16_icon_(false) {
+  set_drag_controller(this);
 }
 
 SiteChipView::~SiteChipView() {
@@ -51,27 +189,103 @@ void SiteChipView::Init() {
   AddChildView(location_icon_view_);
   AddChildView(host_label_);
 
-  // temporary background painter
-  const int kBackgroundImages[] = IMAGE_GRID(IDR_SITE_CHIP_EV);
-  background_painter_.reset(
-      views::Painter::CreateImageGridPainter(kBackgroundImages));
-
-  // temporary icon filler
   location_icon_view_->SetImage(GetThemeProvider()->GetImageSkiaNamed(
-      IDR_OMNIBOX_HTTPS_VALID));
+      IDR_LOCATION_BAR_HTTP));
   location_icon_view_->ShowTooltip(true);
 
-  // temporary filler text.
-  host_label_->SetText(ASCIIToUTF16("site.chip"));
+  const int kEVBackgroundImages[] = IMAGE_GRID(IDR_SITE_CHIP_EV);
+  ev_background_painter_.reset(
+      views::Painter::CreateImageGridPainter(kEVBackgroundImages));
+  const int kBadSSLBackgroundImages[] = IMAGE_GRID(IDR_SITE_CHIP_BROKENSSL);
+  badssl_background_painter_.reset(
+      views::Painter::CreateImageGridPainter(kBadSSLBackgroundImages));
 }
 
 bool SiteChipView::ShouldShow() {
   return chrome::ShouldDisplayOriginChip();
 }
 
-void SiteChipView::Update(content::WebContents* tab) {
+void SiteChipView::Update(content::WebContents* web_contents) {
+  if (!web_contents)
+    return;
+
+  // Note: security level can change async as the connection is made.
+  GURL url = toolbar_view_->GetToolbarModel()->GetURL();
+  const ToolbarModel::SecurityLevel security_level =
+      toolbar_view_->GetToolbarModel()->GetSecurityLevel(true);
+  if (url == url_displayed_ && (security_level == security_level_))

[Peter Kasting, 2013/12/10 03:28:39]

Nit: Parens on both subexprs

[Greg Billock, 2013/12/10 17:37:59]

Done.

+    return;
+
+  url_displayed_ = url;
+
+  string16 host = SiteLabelFromURL(url);
+
+  // TODO(gbillock): Deal with RTL here better? Use a separate
+  // label for cert name?
+  if ((security_level != security_level_) &&
+      (security_level == ToolbarModel::EV_SECURE)) {
+    host = toolbar_view_->GetToolbarModel()->GetEVCertName() +
+           ASCIIToUTF16(" ") + host;

[Peter Kasting, 2013/12/10 03:28:39]

This isn't good l10n, even ignoring RTL.

Use some kind of a substitutable string defined in the GRD file to be "$1 $2" or
"$1 - $2" or something, then substitute the strings in.  That lets localizers
reorder and change delimiters as necessary.

[Greg Billock, 2013/12/10 17:37:59]

I hadn't considered using L10N to deal with this -- it seems more like a display
than formatting issue, but I think that's a good plan. I'll add it in.

+  }
+
+  host_ = host;
+  host_label_->SetText(host_);
+  host_label_->SetTooltipText(host_);
+  SkColor toolbar_background =
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_TOOLBAR);
+  SkColor host_color =

[Peter Kasting, 2013/12/10 03:28:39]

Nit: I would at least inline this into the next statement.

[Greg Billock, 2013/12/10 17:37:59]

Done.

+      color_utils::GetReadableColor(SK_ColorBLACK, toolbar_background);
+  host_label_->SetEnabledColor(host_color);
+
+  security_level_ = security_level;
+  int icon = toolbar_view_->GetToolbarModel()->GetIconForSecurityLevel(
+      security_level_);
+  showing_16x16_icon_ = false;
+
+  if (!url.is_valid() ||
+      url.SchemeIs(chrome::kChromeUIScheme)) {
+    icon = IDR_PRODUCT_LOGO_16;
+    showing_16x16_icon_ = true;
+  }
+
+  if (url.SchemeIs(extensions::kExtensionScheme)) {
+    icon = IDR_EXTENSIONS_FAVICON;
+    showing_16x16_icon_ = true;
+
+    ExtensionService* service =
+        extensions::ExtensionSystem::Get(
+            toolbar_view_->browser()->profile())->extension_service();
+    const extensions::Extension* extension =
+        service->extensions()->GetExtensionOrAppByURL(url);
+    extension_icon_loader_.reset(
+        new SiteChipExtensionIcon(location_icon_view_,
+                                  toolbar_view_->browser()->profile(),
+                                  extension));
+  } else {
+    extension_icon_loader_.reset();
+  }
+
+  location_icon_view_->SetImage(GetThemeProvider()->GetImageSkiaNamed(icon));
+  location_icon_view_->ShowTooltip(true);
+
+  // TODO(gbillock): Add malware accounting.
+  if (security_level_ == ToolbarModel::SECURITY_ERROR)
+    painter_ = badssl_background_painter_.get();
+  else if (security_level_ == ToolbarModel::EV_SECURE)
+    painter_ = ev_background_painter_.get();
+  else
+    painter_ = NULL;
+
   Layout();
   SchedulePaint();
+  // TODO(gbillock): Need to schedule paint on parent to erase any background?
+}
+
+void SiteChipView::OnChanged() {
+  Update(toolbar_view_->GetWebContents());
+  toolbar_view_->Layout();
+  toolbar_view_->SchedulePaint();
+  // TODO(gbillock): Also need to potentially redo infobars.

[Peter Kasting, 2013/12/10 03:28:39]

Nit: Might be nice to clarify that this is about where the infobar arrows point.

Also seems like the toolbar, rather than the site chip, should be responsible
for doing that update.

[Greg Billock, 2013/12/10 17:37:59]

Done. The update signals come in lower, though, so I think we'll have to trigger
it from here.

 }
 
 gfx::Size SiteChipView::GetPreferredSize() {
@@ -79,19 +293,23 @@ gfx::Size SiteChipView::GetPreferredSize() {
   gfx::Size icon_size = location_icon_view_->GetPreferredSize();
   return gfx::Size(icon_size.width() + label_size.width() +
                    kIconTextSpacing + kTrailingLabelMargin +
-                   2 * kEdgeThickness,
+                   2 * kEdgeThickness +
+                   (showing_16x16_icon_ ? 2 * k16x16IconExtraSpacing : 0),
                    icon_size.height());
 }
 
 void SiteChipView::Layout() {
+  // TODO(gbillock): Eventually we almost certainly want to use
+  // LocationBarLayout for leading and trailing decorations.
+
   location_icon_view_->SetBounds(
-      kEdgeThickness,
+      kEdgeThickness + (showing_16x16_icon_ ? k16x16IconExtraSpacing : 0),
       LocationBarView::kNormalEdgeThickness,
       location_icon_view_->GetPreferredSize().width(),
       height() - 2 * LocationBarView::kNormalEdgeThickness);
 
   int host_label_x = location_icon_view_->x() + location_icon_view_->width() +
-      kIconTextSpacing;
+      (showing_16x16_icon_ ? k16x16IconExtraSpacing : 0) + kIconTextSpacing;
   int host_label_width =
       width() - host_label_x - kEdgeThickness - kTrailingLabelMargin;
   host_label_->SetBounds(host_label_x,
@@ -102,10 +320,8 @@ void SiteChipView::Layout() {
 
 void SiteChipView::OnPaint(gfx::Canvas* canvas) {
   gfx::Rect rect(GetLocalBounds());
-  rect.Inset(LocationBarView::kNormalEdgeThickness,
-             LocationBarView::kNormalEdgeThickness);
-  if (background_painter_.get())
-    views::Painter::PaintPainterAt(canvas, background_painter_.get(), rect);
+  if (painter_)
+    views::Painter::PaintPainterAt(canvas, painter_, rect);
 
   ToolbarButton::OnPaint(canvas);
 }
@@ -119,3 +335,29 @@ void SiteChipView::ButtonPressed(views::Button* sender,
   toolbar_view_->location_bar()->GetOmniboxView()->model()->
       SetCaretVisibility(true);
 }
+
+void SiteChipView::WriteDragDataForView(View* sender,
+                                        const gfx::Point& press_pt,
+                                        OSExchangeData* data) {
+  content::WebContents* web_contents = toolbar_view_->GetWebContents();

[Peter Kasting, 2013/12/10 03:28:39]

Nit: At least add a TODO here about consolidating this, even if this patch
doesn't do it.

[Greg Billock, 2013/12/10 17:37:59]

Done.

+  FaviconTabHelper* favicon_tab_helper =
+      FaviconTabHelper::FromWebContents(web_contents);
+  gfx::ImageSkia favicon = favicon_tab_helper->GetFavicon().AsImageSkia();
+  button_drag_utils::SetURLAndDragImage(web_contents->GetURL(),
+                                        web_contents->GetTitle(),
+                                        favicon,
+                                        data,
+                                        sender->GetWidget());
+}
+
+int SiteChipView::GetDragOperationsForView(View* sender,
+                                       const gfx::Point& p) {
+  return ui::DragDropTypes::DRAG_COPY | ui::DragDropTypes::DRAG_LINK;
+}
+
+bool SiteChipView::CanStartDragForView(View* sender,
+                                   const gfx::Point& press_pt,
+                                   const gfx::Point& p) {
+  return true;
+}
+