[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..8b6418a8007cc90f8f3dced96ea7eaeed3122159 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,28 +21,144 @@
 #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;

[Peter Kasting, 2013/12/06 02:16:52]

Nit: Anonymous-namespace these and move them to the top of the SiteChipView
section (see below)

[Greg Billock, 2013/12/06 19:54:26]

Done.

+const int k16x16IconExtraSpacing = 3;
 const int kIconTextSpacing = 4;
 const int kTrailingLabelMargin = 2;
 
+class SiteChipExtensionIcon : public extensions::IconImage::Observer {

[Peter Kasting, 2013/12/06 02:16:52]

Nit: Add a // SiteChipExtensionIcon ----... divider above this class as well as
above the SiteChipView section below.  -s go out to column 79, two blank lines
above a divider and one below.

[Greg Billock, 2013/12/06 19:54:26]

Done.

+ public:
+  SiteChipExtensionIcon(LocationIconView* icon_view,
+                        Profile* profile,
+                        const extensions::Extension* extension)
+      : icon_view_(icon_view) {

[Peter Kasting, 2013/12/06 02:16:52]

Don't inline definitions into the class declaration except in test files.

[Greg Billock, 2013/12/06 19:54:26]

Done.

+    icon_image_.reset(new extensions::IconImage(

[Peter Kasting, 2013/12/06 02:16:52]

Nit: Avoid reset() call by initing this in the initializer list above.

[Greg Billock, 2013/12/06 19:54:26]

Done.

+        profile,
+        extension,
+        extensions::IconsInfo::GetIcons(extension),
+        extension_misc::EXTENSION_ICON_BITTY,
+        extensions::IconsInfo::GetDefaultAppIcon(),
+        this));
+    icon_image_->image_skia().GetRepresentation(1.0f);
+
+    if (icon_image_->image_skia().image_reps().size() > 0)

[Peter Kasting, 2013/12/06 02:16:52]

Nit: Use !empty()

[Greg Billock, 2013/12/06 19:54:26]

Done.

+      icon_view_->SetImage(&(icon_image_->image_skia()));

[Peter Kasting, 2013/12/06 02:16:52]

Nit: Call OnExtensionIconImageChanged() instead.

Is the !empty() check necessary there too?  It seems strange that we would need
it in one place but not the other.

[Greg Billock, 2013/12/06 19:54:26]

Done.

+  }
+
+  ~SiteChipExtensionIcon() {
+    icon_view_ = NULL;
+    icon_image_.reset();

[Peter Kasting, 2013/12/06 02:16:52]

Why are we doing either of these?

[Greg Billock, 2013/12/06 19:54:26]

I hunted through the IconImage code for a while trying to figure out how it
cancelled requests to make sure it doesn't use after free. I think I ended up
with this as I figured out it was in the destructor, but I don't think we need
them as it's all on the UI thread. A bit uncanny for my taste...

+  }
+
+  // IconImage::Observer:
+  virtual void OnExtensionIconImageChanged(
+      extensions::IconImage* image) OVERRIDE {
+    if (icon_view_)
+      icon_view_->SetImage(&(icon_image_->image_skia()));

[Peter Kasting, 2013/12/06 02:16:52]

Nit: Are the innermost parens necessary?  Normally I'd expect to not see them
here.

[Greg Billock, 2013/12/06 19:54:26]

Done.

+  }
+
+ private:
+  scoped_ptr<extensions::IconImage> icon_image_;
+  LocationIconView* icon_view_;
+};

[Peter Kasting, 2013/12/06 02:16:52]

Nit: DISALLOW_COPY_AND_ASSIGN

[Greg Billock, 2013/12/06 19:54:26]

Done.

+
+string16 HostnameFromURL(const GURL& url, Profile* profile) {

[Peter Kasting, 2013/12/06 02:16:52]

This function name is misleading.  We're not necessarily getting a hostname, so
neither this function nor our caller should call it that.  We should just call
this SiteChipContents or SiteChipText or something.

Also, I'd make this a private member so you can drop the second arg, since this
is only called from inside SiteChipView anyway.

[Greg Billock, 2013/12/06 19:54:26]

Renamed.

On 2013/12/06 02:16:52, Peter Kasting wrote:

+  // The NTP.
+  if (!url.is_valid()) {

[Peter Kasting, 2013/12/06 02:16:52]

Nit: No {}

[Greg Billock, 2013/12/06 19:54:26]

Done.

+    return string16(UTF8ToUTF16("Chrome"));

[Peter Kasting, 2013/12/06 02:16:52]

Thought we were doing "New Tab" here.

Also, the explicit string16() should not be necessary here or on any of the
similar calls below.

[Greg Billock, 2013/12/06 19:54:26]

I have a TODO here below -- If we use the page name, I'll make a little
translation table. I'm not sure whether we need translated strings or what,
though. Let's talk about it.

+  }
+
+  // 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"));
+  }
+
+

[Peter Kasting, 2013/12/06 02:16:52]

Nit: Extra blank line

[Greg Billock, 2013/12/06 19:54:26]

Done.

+  // For file: urls, return the BaseName of the file       .

[Peter Kasting, 2013/12/06 02:16:52]

That seems wrong.  I'd expect to see the whole file path.

[Greg Billock, 2013/12/06 19:54:26]

I'm not sure what the use case is. If its basically developers, then perhaps we
want to show the path in the omnibox in that case. (similarly for view-source)

+  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?

[Peter Kasting, 2013/12/06 02:16:52]

Normally we'd strip the view-source: part and then process the remainder
normally.

[Greg Billock, 2013/12/06 19:54:26]

ack. Leaving as TODO for now.

+
+  // For chrome-extension urls, return the extension name.
+  if (url.SchemeIs(extensions::kExtensionScheme)) {
+    ExtensionService* service =
+        extensions::ExtensionSystem::Get(profile)->extension_service();
+    if (!service)

[Peter Kasting, 2013/12/06 02:16:52]

Can this actually fail?  Maybe in tests?

[Greg Billock, 2013/12/06 19:54:26]

Yeah, this should never fail in production. I don't think tests should touch
this; I'll check.

+      return string16(base::UTF8ToUTF16(url.host()));
+
+    const extensions::Extension* extension =
+        service->extensions()->GetExtensionOrAppByURL(url);
+    if (!extension)
+      return string16(UTF8ToUTF16(url.host()));
+    return string16(base::UTF8ToUTF16(extension->name()));

[Peter Kasting, 2013/12/06 02:16:52]

Nit: Shorter:

  return base::UTF8ToUTF16(extension ? extension->name() : url.host());

[Greg Billock, 2013/12/06 19:54:26]

Done.

+  }
+
+  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.

[Peter Kasting, 2013/12/06 02:16:52]

This whole section is way too complicated for what you want.

Get url.host(), pass the result through net::StripWWW(), and you're done.

[Greg Billock, 2013/12/06 19:54:26]

I'm worried about i18n domain names and other unicode gotchas that FormatUrl is
accounting for inside. This produces identical output to what we draw in the
omnibox, which is reassuring. I'm all for simplifying, but there's a couple
hundred lines of code, at least, doing all kinds of magic in there that look
like they may have started with StripWWW(host()) and grew hair.

[Peter Kasting, 2013/12/06 21:26:11]

That's fair.  It seems like in that case though we just need to run the hostname
through net_util::IDNToUnicode().  That's still simpler than the sort of
conditional stripping you're doing.
I don't know precisely where you're looking so I can't comment.

[Greg Billock, 2013/12/06 22:11:50]

FormatUrl calls through a couple layers internally, but eventually you get to a
big pile of code that does most of the work. I'm sure at least some of it is
unnecessary -- code specific to paths and so on. But URLs are weird enough that
I'm reluctant to improvise too much.

+    std::string languages;
+    if (profile)
+      languages = profile->GetPrefs()->GetString(prefs::kAcceptLanguages);
+
+    base::string16 formatted = net::FormatUrl(url.GetOrigin(), languages,
+        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.

[Peter Kasting, 2013/12/06 02:16:52]

We should treat this like HTTP/HTTPS.  Prepending "ftp:" not only results in
some bizarre string that looks kind of like a URL but not really, it isn't
necessary information in this chip.

[Greg Billock, 2013/12/06 19:54:26]

I'm not sure where this originated -- it's in the experiment doc, so I put it
here, but let's figure out if that came from a security concern or what.

+  if (url.SchemeIs(content::kFtpScheme)) {
+    return string16(UTF8ToUTF16(std::string("ftp:") + url.host()));
+  }
+
+  // If all else fails, return hostname.
+  return string16(UTF8ToUTF16(url.host()));

[Peter Kasting, 2013/12/06 02:16:52]

Just remove the scheme check above and let that code handle this case too.

[Greg Billock, 2013/12/06 19:54:26]

Let's figure out whether we need net::FormatUrl first.

+}
+
 SiteChipView::SiteChipView(ToolbarView* toolbar_view)
     : ToolbarButton(this, NULL),
-      toolbar_view_(toolbar_view) {
+      toolbar_view_(toolbar_view),
+      showing_16x16_icon_(false) {
+  set_drag_controller(this);
 }
 
 SiteChipView::~SiteChipView() {
+  extension_icon_loader_.reset();

[Peter Kasting, 2013/12/06 02:16:52]

Why does this need to be done explicitly?

[Greg Billock, 2013/12/06 19:54:26]

I don't like leaving the icon fetch to clean itself up implicitly, but if that's
how the API is supposed to work, I'll just use it. If it was called
ScopedIconImageFetcher I think I'd be happier. :-)

[Peter Kasting, 2013/12/06 21:26:11]

I don't understand your comment here or in ~SiteChipExtensionIcon().  The reason
you don't need these reset()s is because these are scoped_ptrs, and thus on
class destruction they delete themselves.  Having an explicit reset() in both
these functions is functionally identical to having no reset(), regardless of
what class is contained in the scoped_ptr.  The only difference is if you need
to control the order in which the member variables are destroyed.  If you're
just trying to make sure destruction happens at all, explicit reset()s aren't
needed.

[Greg Billock, 2013/12/06 22:11:50]

Agreed. I got gunshy after getting unexpected crashes in the IconImage class. I
don't like its API, and put in defenses around it, but I agree I went overboard.
:-) I took them all out, I believe.

 }
 
 void SiteChipView::Init() {
@@ -51,18 +173,9 @@ 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"));
 }
 
 bool SiteChipView::ShouldShow() {
@@ -70,8 +183,101 @@ bool SiteChipView::ShouldShow() {
 }
 
 void SiteChipView::Update(content::WebContents* tab) {

[Peter Kasting, 2013/12/06 02:16:52]

Nit: While here: This should be named |web_contents| instead of |tab|.

[Greg Billock, 2013/12/06 19:54:26]

Done.

+  if (!tab)
+    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/06 02:16:52]

Nit: Parens around binary subexprs (several places)

[Greg Billock, 2013/12/06 19:54:26]

Done.

+    return;
+
+  url_displayed_ = url;
+
+  string16 host = HostnameFromURL(url, toolbar_view_->browser()->profile());
+
+  if (security_level != security_level_ &&
+      security_level == ToolbarModel::EV_SECURE) {

[Peter Kasting, 2013/12/06 02:16:52]

Nit: No {}

[Greg Billock, 2013/12/06 19:54:26]

Done.

+    host = toolbar_view_->GetToolbarModel()->GetEVCertName();
+  }
+
+  host_ = host;
+  host_label_->SetText(host_);
+  host_label_->SetTooltipText(host_);
+  // TODO(gbillock): do better than this: goal is to use black unless
+  // it'd be too low contrast, then switch to ?? inverse?

[Peter Kasting, 2013/12/06 02:16:52]

Yeah, so what's the problem?  That seems to be what you're doing.

If you're worried about the background painter making the toolbar color too
different, look at how the ev bubble and keyword search bubble code calculate
the appropriate background color to compare against.

[Greg Billock, 2013/12/06 19:54:26]

stale comment, I ended up just doing this but didn't erase the comment.

+  SkColor toolbar_background =
+      GetThemeProvider()->GetColor(ThemeProperties::COLOR_TOOLBAR);
+  SkColor host_color =
+      color_utils::GetReadableColor(SK_ColorBLACK, toolbar_background);
+  host_label_->SetEnabledColor(host_color);
+
+  security_level_ = security_level;
+  // See ToolbarModelImpl::GetIcon()

[Peter Kasting, 2013/12/06 02:16:52]

Instead of copying parts of that code here can we just share it?

[Greg Billock, 2013/12/06 19:54:26]

I'm not sure why this icon is calculated in so disjointed a way. I don't
understand why there's a whole header for types, except if there's some
invisible test or something that's consuming it. It looked like a yak shave to
touch, but perhaps that's all just stale.

[Peter Kasting, 2013/12/06 21:26:11]

It doesn't look that bad -- it looks like GetIcon() does what you want except
that it accounts for search term replacement, which is probably not a step you
want here.  That could be done conditionally or split into a separate function.

[Greg Billock, 2013/12/06 22:11:50]

I'll give it a shot. I'm trying to prioritize keeping experimental changes from
disrupting the existing code too much, and when it happens, doing generally
useful things. This seems on the edge of useful to me.

[Greg Billock, 2013/12/06 22:42:41]

Sent you a CL under separate cover to create this API. It looks about like you'd
expect. It's a bit sad that it requires a new method, but I think I prefer that
here to a mystery-meat boolean argument.

+  static int icon_ids[ToolbarModel::NUM_SECURITY_LEVELS] = {
+    IDR_LOCATION_BAR_HTTP,
+    IDR_OMNIBOX_HTTPS_VALID,
+    IDR_OMNIBOX_HTTPS_VALID,
+    IDR_OMNIBOX_HTTPS_WARNING,
+    IDR_OMNIBOX_HTTPS_POLICY_WARNING,
+    IDR_OMNIBOX_HTTPS_INVALID,
+  };
+  DCHECK(arraysize(icon_ids) == ToolbarModel::NUM_SECURITY_LEVELS);
+  int icon = icon_ids[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();
+    if (service) {

[Peter Kasting, 2013/12/06 02:16:52]

Again, when can this fail?

[Greg Billock, 2013/12/06 19:54:26]

Done.

+      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);
+
+  if (security_level_ == ToolbarModel::EV_SECURE) {
+    const int kBackgroundImages[] = IMAGE_GRID(IDR_SITE_CHIP_EV);
+    background_painter_.reset(
+        views::Painter::CreateImageGridPainter(kBackgroundImages));

[Peter Kasting, 2013/12/06 02:16:52]

Recreating these painters from scratch on every change sucks.  Let's keep
background painters for both states that we set up in the contstructor, and then
simply set a Painter* member to point at the right one or NULL.

[Greg Billock, 2013/12/06 19:54:26]

Yeah, it's kind of painful. I'll make special-purpose ones.

+  } else if (security_level_ == ToolbarModel::SECURITY_ERROR) {
+    const int kBackgroundImages[] = IMAGE_GRID(IDR_SITE_CHIP_BROKENSSL);
+    background_painter_.reset(
+        views::Painter::CreateImageGridPainter(kBackgroundImages));
+  } else {
+    // TODO(gbillock): Add malware accounting.
+    background_painter_.reset();
+  }
+
   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/06 02:16:52]

Why?

[Greg Billock, 2013/12/06 19:54:26]

The layout of the site chip can change depending on the site. In a couple cases,
there are infobars that follow the toolbar around, and those need redrawn or
they anchor into the wrong place. It's kind of an unusual thing, but awkward. We
need to account for it somehow -- perhaps by anchoring those infobars elsewhere.

[Peter Kasting, 2013/12/06 21:26:11]

I would ask the UI leads about this and consider proposing that we anchor
infobars to the Chrome menu ("wrench") button.

[Greg Billock, 2013/12/06 22:11:50]

will do.

 }
 
 gfx::Size SiteChipView::GetPreferredSize() {
@@ -79,19 +285,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),

[Peter Kasting, 2013/12/06 02:16:52]

The whole point of the spacing we set up before was so 16x16 icons would look
good without adding more extra space around them.  We should definitely not be
adding extra space here or anywhere else in this file.

[Greg Billock, 2013/12/06 19:54:26]

Let's talk to Alan about this. The location icons are higher than they are wide
-- they have a visual width quite a bit narrower than the typical 16x16 favicon
type icon, particularly for the lock and the page icon, which are the most
commonly seen. Alan thought extra padding was necessary for the Chrome 16x16,
and I think the visual size of that icon is more typical of what we'll see for
extensions than are the usual page icon sizes.

[Peter Kasting, 2013/12/06 21:26:11]

I haven't seen that comment.

                    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,
@@ -119,3 +329,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();
+  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());

[Peter Kasting, 2013/12/06 02:16:52]

It seems like this ought to be the same data the omnibox would normally write. 
Can we share the code?

[Greg Billock, 2013/12/06 19:54:26]

This is basically copy-pasted from the drag support for the location icon view.
If the experiment succeeds, we'll eliminate that and it'll just live here.

[Peter Kasting, 2013/12/06 21:26:11]

Still seems like until then we could make this a static method that takes a
WebContents* and just call it from both places.  I guess I'm just always leery
of "we can clean this up later once ______ occurs" because I've been burned too
many times by it not getting cleaned up :/

[Greg Billock, 2013/12/06 22:11:50]

:-) I hear you. I guess my intuition is that this experiment has a low
probability of success, and so refactoring if it is successful will need to
happen, whereas re-consolidating a bunch of marginal-utility refactorings after
we delete it is the thing that'll be harder to make sure happens. If your
intuition is opposite, though, that's probably a good thing, and I'd consider it
more reliable. :-)

+}
+
+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;
+}
+