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

chrome/browser/ui/views/home_button.cc

Index: chrome/browser/ui/views/home_button.cc
===================================================================
--- chrome/browser/ui/views/home_button.cc	(revision 0)
+++ chrome/browser/ui/views/home_button.cc	(revision 0)
@@ -0,0 +1,174 @@
+// 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.
+
+#include "chrome/browser/ui/views/home_button.h"
+
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/pref_names.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/views/bubble/bubble_delegate.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/controls/link.h"
+#include "ui/views/controls/link_listener.h"
+#include "ui/views/layout/grid_layout.h"
+#include "ui/views/layout/layout_constants.h"
+#include "ui/views/widget/widget.h"
+
+namespace {

[Peter Kasting, 2013/01/18 18:19:27]

Nit: When there are multiple classes in a file, I tend to separate them with
comment lines as follows:

...<previous file contents above here>...
<blank line>
<blank line>
// Classname --------------- <out to 79 cols>
<blank line>
...<class contents>...

Repeat this for every distinct class.

When one class is in an anonymous namespace I usually go ahead and treat the
namespace opener and closer as "part of the class" too for this purpose.

None of this is required, I just find it helps make the different classes stand
out.

[Tom Cassiotis, 2013/01/18 21:24:19]

Done.

+class HomePageUndoBubble : public views::BubbleDelegateView,

[Peter Kasting, 2013/01/18 18:19:27]

Nit: Go ahead and add a blank line here.

[Tom Cassiotis, 2013/01/18 21:24:19]

Done.

+                           public views::LinkListener {
+ public:
+  static void ShowBubble(Browser* browser,
+                         bool undo_value_is_ntp,
+                         const GURL& undo_url,
+                         views::View* anchor_view);
+  static void HideBubble();
+
+ private:
+  HomePageUndoBubble(Browser* browser,
+      bool undo_value_is_ntp,

[Peter Kasting, 2013/01/18 18:19:27]

Nit: Args should be aligned

[Tom Cassiotis, 2013/01/18 21:24:19]

I reread the style guides and have a better understanding.

Question: The examples imply this but it isn't explicit. Is this a guideline:
If you have to split parameters on multiple lines dedicate a line per parameter.
 

Avoid:
home_page_undo_bubble_ = new HomePageUndoBubble(browser, undo_value_is_ntp,
                                                undo_url, anchor_view);
Prefer:
home_page_undo_bubble_ = new HomePageUndoBubble(browser, 
                                                undo_value_is_ntp,
                                                undo_url, 
                                                anchor_view);
(hopefully the spaces are respected in comments)

Is there any difference between function calls and definitions?

My guess is that this is not enforced because I have seen a both styles in the
code.

Thank you for the detailed review. My next CL to have *significantly* less style
issues (hopefully none :) ). 

Done.

[Peter Kasting, 2013/01/18 21:45:34]

The only rule is, if you use multiple lines, the beginnings of all the lines
should all be aligned.

There is no rule about one-vs.-many args per line, nor about definitions being
distinct from calls.

We used to have a team style rule about not using multiple args per line at
declarations/definitions (but not calls), but this was removed.  I still tend to
encourage people to use one arg per line at declarations/definitions for
clarity.  At callsites I don't really care.

So my personal preference would be one arg per line here, but it's not
mandatory.

+      const GURL &undo_url,

[Peter Kasting, 2013/01/18 18:19:27]

Nit: '&' and '*' go on type name, not variable name

[Tom Cassiotis, 2013/01/18 21:24:19]

Done.

+      views::View* anchor_view);
+  virtual ~HomePageUndoBubble();
+
+  // views::BubbleDelegateView methods.

[Peter Kasting, 2013/01/18 18:19:27]

Nit: " methods." -> ":" for consistency with below

[Tom Cassiotis, 2013/01/18 21:24:19]

Done.

+  virtual void Init() OVERRIDE;
+  virtual void WindowClosing() OVERRIDE;
+
+  // views::LinkListener:
+  virtual void LinkClicked(views::Link* source, int event_flags) OVERRIDE;
+
+  static HomePageUndoBubble* home_page_undo_bubble_;
+
+  Browser* browser_;
+  bool undo_value_is_ntp_;
+  GURL undo_url_;
+
+  DISALLOW_COPY_AND_ASSIGN(HomePageUndoBubble);
+};
+
+// static
+HomePageUndoBubble* HomePageUndoBubble::home_page_undo_bubble_ = NULL;
+
+void HomePageUndoBubble::ShowBubble(Browser* browser,
+                                    bool undo_value_is_ntp,
+                                    const GURL &undo_url,
+                                    views::View* anchor_view) {
+  HideBubble();
+  home_page_undo_bubble_ = new HomePageUndoBubble(browser, undo_value_is_ntp,
+                                                  undo_url, anchor_view);
+  views::BubbleDelegateView::CreateBubble(home_page_undo_bubble_);
+  home_page_undo_bubble_->StartFade(true);
+}
+
+void HomePageUndoBubble::HideBubble() {
+  if (home_page_undo_bubble_ != NULL)
+    home_page_undo_bubble_->GetWidget()->Close();
+}
+
+HomePageUndoBubble::HomePageUndoBubble(
+    Browser* browser,
+    bool undo_value_is_ntp,
+    const GURL &undo_url,
+    views::View* anchor_view)
+    : BubbleDelegateView(anchor_view, views::BubbleBorder::TOP_LEFT),
+      browser_(browser),
+      undo_value_is_ntp_(undo_value_is_ntp),
+      undo_url_(undo_url) {
+}
+
+HomePageUndoBubble::~HomePageUndoBubble() {
+}
+
+void HomePageUndoBubble::Init() {
+  ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
+  views::GridLayout* layout = new views::GridLayout(this);
+  SetLayoutManager(layout);
+
+  // Create two columns for the message and the undo link.
+  views::ColumnSet* cs = layout->AddColumnSet(0);
+  cs = layout->AddColumnSet(1);
+  cs->AddColumn(views::GridLayout::LEADING, views::GridLayout::BASELINE, 0,
+                views::GridLayout::USE_PREF, 0, 0);
+  cs->AddPaddingColumn(0, views::kRelatedControlHorizontalSpacing);
+  cs->AddColumn(views::GridLayout::CENTER, views::GridLayout::BASELINE, 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);
+  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_listener(this);
+  layout->AddView(undo_link);
+}
+
+void HomePageUndoBubble::LinkClicked(views::Link* source, int event_flags) {
+  browser_->profile()->GetPrefs()->SetBoolean(prefs::kHomePageIsNewTabPage,

[Peter Kasting, 2013/01/18 18:19:27]

Nit: I'd probably pull this out into a temp as you were doing before, since
GetPrefs() is camel-case and thus we assume not cheap

[Tom Cassiotis, 2013/01/18 21:24:19]

Done.

+                                              undo_value_is_ntp_);
+  browser_->profile()->GetPrefs()->SetString(prefs::kHomePage,
+                                             undo_url_.spec());
+  HideBubble();
+}
+
+void HomePageUndoBubble::WindowClosing() {
+  // We have to reset |home_page_undo_bubble_| here, not in our destructor,
+  // because we'll be destroyed asynchronously.

[Peter Kasting, 2013/01/18 18:19:27]

Nit: OK, but why?  (I think I know why -- so that triggering the bubble show
while the bubble is in the midst of closing doesn't break things -- but you
should be explicit.  Oh, and don't just copy that and say "break things" either,
flesh it out :) )

[Tom Cassiotis, 2013/01/18 21:24:19]

After closer inspection the only thing that would happen is that the widget's
Close() would be called twice.  The widget itself guards against this situation
so in actuality nothing "bad" would happen.  However, I presume that multiple
calls to the widget's Close() is breaking a contract with that class.

I hope I have a good understanding of the situation and I have been able to
communicate this adequately with the change to the comment.

Done.

+  DCHECK_EQ(this, home_page_undo_bubble_);
+  home_page_undo_bubble_ = NULL;
+}
+
+}  // namespace
+
+HomeImageButton::HomeImageButton(
+    views::ButtonListener* listener,
+    Browser* browser)
+    : views::ImageButton(listener),
+      browser_(browser) {
+}
+
+HomeImageButton::~HomeImageButton() {
+}
+
+bool HomeImageButton::GetDropFormats(
+      int* formats,

[Peter Kasting, 2013/01/18 18:19:27]

Nit: Indent 4, not 8

[Tom Cassiotis, 2013/01/18 21:24:19]

Done.

+      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) {
+  return (event.source_operations() & ui::DragDropTypes::DRAG_LINK) ?
+      ui::DragDropTypes::DRAG_LINK :
+      ui::DragDropTypes::DRAG_NONE;

[Peter Kasting, 2013/01/18 18:19:27]

Nit: Welcome to put this on previous line if it fits.

[Tom Cassiotis, 2013/01/18 21:24:19]

Done.

+}
+
+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* prefs = browser_->profile()->GetPrefs();
+    bool old_is_ntp = prefs->GetBoolean(prefs::kHomePageIsNewTabPage);
+    GURL old_homepage(prefs->GetString(prefs::kHomePage));
+
+    prefs->SetBoolean(prefs::kHomePageIsNewTabPage, false);
+    prefs->SetString(prefs::kHomePage, new_homepage_url.spec());
+
+    HomePageUndoBubble::ShowBubble(browser_, old_is_ntp, old_homepage, this);
+  }
+  return ui::DragDropTypes::DRAG_NONE;
+}
+