Implemented drop hander for the Home toolbar button that accepts (only) links

chrome/browser/ui/views/toolbar_view.cc

Index: chrome/browser/ui/views/toolbar_view.cc
===================================================================
--- chrome/browser/ui/views/toolbar_view.cc	(revision 175785)
+++ chrome/browser/ui/views/toolbar_view.cc	(working copy)
@@ -51,8 +51,12 @@
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/canvas_image_source.h"
 #include "ui/views/controls/button/button_dropdown.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/controls/link.h"
 #include "ui/views/controls/menu/menu_listener.h"
 #include "ui/views/focus/view_storage.h"
+#include "ui/views/layout/grid_layout.h"
+#include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/tooltip_manager.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/non_client_view.h"
@@ -147,8 +151,197 @@
   DISALLOW_COPY_AND_ASSIGN(BadgeImageSource);
 };
 
+class HomePageUndoBubble : public views::BubbleDelegateView,
+                       public views::ButtonListener,

[Peter Kasting, 2013/01/17 23:48:42]

Nit: All part declarations should be aligned

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+                       public views::LinkListener {
+ public:
+  // |browser| is the opening browser and is NULL in unittests.
+  static void ShowBubble(Browser* browser,
+      bool undo_value_is_ntp,

[Peter Kasting, 2013/01/17 23:48:42]

Nit: All args should be aligned

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+      const GURL &undo_url,

[Peter Kasting, 2013/01/17 23:48:42]

Nit: & binds to type, not name

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+      const GURL &new_home_page,
+      views::View* anchor_view);
+  static void HideBubble();
+
+ protected:

[Peter Kasting, 2013/01/17 23:48:42]

Nit: Why protected?  No one subclasses this class... this should probably be
private.

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+  // views::BubbleDelegateView overrides:
+  virtual void Init() OVERRIDE;
+
+ private:
+  HomePageUndoBubble(Browser* browser,
+      bool undo_value_is_ntp,
+      const GURL &undo_url,
+      const GURL &new_home_page,
+      views::View* anchor_view);
+  virtual ~HomePageUndoBubble();
+
+  // views::ButtonListener overrides:

[Peter Kasting, 2013/01/17 23:48:42]

Nit: I've been trying to standardize on just "// views::ButtonListener:"

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+  virtual void ButtonPressed(views::Button* sender,
+                             const ui::Event& event) OVERRIDE;
+
+  // views::LinkListener overrides:
+  virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
+
+  // views::WidgetDelegate method.

[Peter Kasting, 2013/01/17 23:48:42]

Nit: You don't directly subclass WidgetDelegate.  Name this based on your direct
parent, and declare in the same order you declared the parent classes above.

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+  virtual void WindowClosing() OVERRIDE;
+
+  static HomePageUndoBubble* sethomeundo_bubble_;

[Peter Kasting, 2013/01/17 23:48:42]

Nit: No run-together words in variable names.

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+
+  enum HomePageUndoBubbleLinkIds {

[Peter Kasting, 2013/01/17 23:48:42]

Nit: enums get declared atop the section... though in this case I'd omit the
enum entirely, not tag the link, and not check the ID in LinkClicked().  Future
modifiers can do that if we ever add another link.

[Tom Cassiotis, 2013/01/18 15:18:49]

Removed enum.  Done.

+    ID_LINK_UNDO  = 1,
+  };
+
+  Browser* browser_;
+  bool undo_value_is_ntp_;
+  GURL undo_url_;
+  GURL new_home_page_;

[Tom Cassiotis, 2013/01/18 15:18:49]

new_home_page_ was not used as we removed the hyperlink from the bubble.  I
removed the variable and the unnecessary parameters to the constructor.

+
+  DISALLOW_COPY_AND_ASSIGN(HomePageUndoBubble);
+};
+
+// static
+HomePageUndoBubble* HomePageUndoBubble::sethomeundo_bubble_ = NULL;
+
+void HomePageUndoBubble::ShowBubble(Browser* browser,
+                                    bool undo_value_is_ntp,
+                                    const GURL &undo_url,
+                                    const GURL &new_home_page,
+                                    views::View* anchor_view) {
+  HideBubble();
+  sethomeundo_bubble_ = new HomePageUndoBubble(browser, undo_value_is_ntp,
+                                               undo_url, new_home_page,
+                                               anchor_view);
+  views::BubbleDelegateView::CreateBubble(sethomeundo_bubble_);
+  sethomeundo_bubble_->StartFade(true);
+}
+
+void HomePageUndoBubble::HideBubble() {
+  if (sethomeundo_bubble_ != NULL)
+     sethomeundo_bubble_->GetWidget()->Close();

[Peter Kasting, 2013/01/17 23:48:42]

Nit; Indent 2, not 3

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+}
+
+HomePageUndoBubble::HomePageUndoBubble(
+    Browser* browser,
+    bool undo_value_is_ntp,
+    const GURL &undo_url,
+    const GURL &new_home_page,
+    views::View* anchor_view)
+        : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT),

[Peter Kasting, 2013/01/17 23:48:42]

Nit: Indent 4, not 8

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+          browser_(browser),
+          undo_value_is_ntp_(undo_value_is_ntp),
+          undo_url_(undo_url),
+          new_home_page_(new_home_page) {
+}
+
+HomePageUndoBubble::~HomePageUndoBubble() {
+}
+
+void HomePageUndoBubble::Init() {
+  ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
+  views::GridLayout* layout = new views::GridLayout(this);
+  SetLayoutManager(layout);
+
+  layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+
+  // Create two columns for the message and the undo link

[Peter Kasting, 2013/01/17 23:48:42]

Nit: Trailing period

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+  views::ColumnSet* cs = layout->AddColumnSet(0);
+  cs = layout->AddColumnSet(1);

[Peter Kasting, 2013/01/17 23:48:42]

Make sure you test all this in RTL (try starting with --lang=he) and verify that
the undo link is to the left of the label.

[Tom Cassiotis, 2013/01/18 15:18:49]

Verified that is works.

+  cs->AddColumn(views::GridLayout::LEADING, views::GridLayout::CENTER, 0,

[Peter Kasting, 2013/01/17 23:48:42]

I don't think you want CENTER vertical alignment for these.  In that case, if
the label and the link don't have the same height, they'll be offset in weird
ways.  I think you want BASELINE.

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+                views::GridLayout::USE_PREF, 0, 0);
+  cs->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
+  cs->AddColumn(views::GridLayout::CENTER, views::GridLayout::CENTER, 0,
+                views::GridLayout::USE_PREF, 0, 0);
+
+  views::Label* message_label = new views::Label(
+      l10n_util::GetStringUTF16(IDS_TOOLBAR_INFORM_SET_HOME_PAGE));
+  message_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  message_label->SetMultiLine(true);

[Peter Kasting, 2013/01/17 23:48:42]

If you're using a multiline label, you need to give the label a particular max
width.  I think maybe you don't want a multiline label here anyway, since the
undo link will look weird in that case?

[Tom Cassiotis, 2013/01/18 15:18:49]

This was remnants of an alternative approach. Fixed/Done.

+  layout->StartRow(0, 1);
+  layout->AddView(message_label);
+
+  views::Link* undo_link = new views::Link(
+      l10n_util::GetStringUTF16(IDS_ONE_CLICK_BUBBLE_UNDO));
+  undo_link->set_id(ID_LINK_UNDO);
+  undo_link->set_listener(this);
+  layout->AddView(undo_link);
+
+  layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);

[Peter Kasting, 2013/01/17 23:48:42]

Why this?

[Peter Kasting, 2013/01/17 23:49:46]

Oh, I see you have one at top too.

I'd remove both of these.  They make the bubble look weirdly tall and padded in
your screenshot.

[Tom Cassiotis, 2013/01/18 15:18:49]

I thought it needed some padding. Without the padding it looked 'anemic'.  I
have removed the padding. 

Done.

+}
+
+void HomePageUndoBubble::ButtonPressed(views::Button* sender,

[Peter Kasting, 2013/01/17 23:48:42]

Why do we need to override this function at all?

[Tom Cassiotis, 2013/01/18 15:18:49]

We do not, removed.

Done.

+                                      const ui::Event& event) {
+  GetWidget()->Close();
+}
+
+void HomePageUndoBubble::LinkClicked(views::Link* source, int event_flags) {
+  if (source->id() == ID_LINK_UNDO) {
+    browser_->profile()->GetPrefs()->SetBoolean(prefs::kHomePageIsNewTabPage,
+                                                undo_value_is_ntp_);
+    browser_->profile()->GetPrefs()->SetString(prefs::kHomePage,
+                                               undo_url_.spec());
+    GetWidget()->Close();

[Peter Kasting, 2013/01/17 23:48:42]

Nit: Perhaps this should call HideBubble() instead so all closes go through the
same codepath.

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+  }
+}
+
+void HomePageUndoBubble::WindowClosing() {
+  // We have to reset |sethomeundo_bubble_| here, not in our destructor,
+  // because  we'll be destroyed asynchronously and the shown state will

[Peter Kasting, 2013/01/17 23:48:42]

Nit: Extra space.  Also "will" -> "could", I believe -- you might want to fill
out the scenario where this gets checked.

[Tom Cassiotis, 2013/01/18 15:18:49]

This is a comment I copied from another singleton bubble.  The "and the shown
state will be checked before then." is not actually applicable in this class.

But I have left the first part of the comment as it wouldn't be obvious to a
'reader'.

+  // be checked before then.
+  DCHECK_EQ(sethomeundo_bubble_, this);

[Peter Kasting, 2013/01/17 23:48:42]

Nit: (expected, actual)

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+  sethomeundo_bubble_ = NULL;
+}
+
 }  // namespace
 
+HomeImageButton::HomeImageButton(
+    views::ButtonListener* listener,
+    Browser* browser)
+    : views::ImageButton(listener),
+      browser_(browser) {
+}
+
+HomeImageButton::~HomeImageButton() {
+}
+
+bool HomeImageButton::GetDropFormats(
+      int* formats,
+      std::set<OSExchangeData::CustomFormat>* custom_formats) {
+  *formats = ui::OSExchangeData::URL;
+  return true;
+}
+
+bool HomeImageButton::CanDrop(const OSExchangeData& data) {
+  return data.HasURL();
+}
+
+int HomeImageButton::OnDragUpdated(const ui::DropTargetEvent& event) {
+  if (event.source_operations() & ui::DragDropTypes::DRAG_LINK) {

[Peter Kasting, 2013/01/17 23:48:42]

Nit: {} not needed.  You could also use ?: to make things even shorter.

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+    return ui::DragDropTypes::DRAG_LINK;
+  }
+  return ui::DragDropTypes::DRAG_NONE;
+}
+
+int HomeImageButton::OnPerformDrop(const ui::DropTargetEvent& event) {
+  GURL new_homepage_url;
+  string16 title;
+  if (event.data().GetURLAndTitle(&new_homepage_url, &title) &&
+      new_homepage_url.is_valid()) {
+    PrefService* preferences = browser_->profile()->GetPrefs();

[Peter Kasting, 2013/01/17 23:48:42]

Nit: |prefs| is more common a name (and might avoid some line wrapping)

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+

[Peter Kasting, 2013/01/17 23:48:42]

Nit: Unnecessary newline

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+    const bool old_is_ntp = preferences->GetBoolean(

[Peter Kasting, 2013/01/17 23:48:42]

Nit: While I personally like them, Chrome code generally does not use const on
locals.

[Tom Cassiotis, 2013/01/18 15:18:49]

Done.

+        prefs::kHomePageIsNewTabPage);
+    const GURL old_homepage(preferences->GetString(
+        prefs::kHomePage));
+
+    preferences->SetBoolean(prefs::kHomePageIsNewTabPage, false);
+    preferences->SetString(prefs::kHomePage, new_homepage_url.spec());
+
+    HomePageUndoBubble::ShowBubble(browser_, old_is_ntp, old_homepage,
+        new_homepage_url, this);
+  }
+  return ui::DragDropTypes::DRAG_NONE;
+}
+
 // static
 const char ToolbarView::kViewClassName[] = "browser/ui/views/ToolbarView";
 // The space between items is 3 px in general.
@@ -246,7 +439,7 @@
   reload_->SetAccessibleName(l10n_util::GetStringUTF16(IDS_ACCNAME_RELOAD));
   reload_->set_id(VIEW_ID_RELOAD_BUTTON);
 
-  home_ = new views::ImageButton(this);
+  home_ = new HomeImageButton(this, browser_);
   home_->set_triggerable_event_flags(ui::EF_LEFT_MOUSE_BUTTON |
                                      ui::EF_MIDDLE_MOUSE_BUTTON);
   home_->set_tag(IDC_HOME);