Changes the bookmark bubble to have the following logic:

. We only apply edits when the bubble is closed (previously selecting
  a different folder resulted in an immediate action).
. Hitting escape cancels any edits.
. When the bubble is shown for newly bookmarked items hitting escape
  removes the bookmark.
. If you click 'Edit...' or select 'Choose another folder...' any
  edits are applied immediately before the editor is shown.

BUG=5015
TEST=thorougly test the bookmark bubble to make sure I haven't broken
  anything.

Review URL: http://codereview.chromium.org/14074

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@6991 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 96 insertions, 60 deletions

diff --git a/chrome/browser/views/bookmark_bubble_view.cc b/chrome/browser/views/bookmark_bubble_view.cc
index 626fd80..09ec122 100644
--- a/chrome/browser/views/bookmark_bubble_view.cc
+++ b/chrome/browser/views/bookmark_bubble_view.cc
@@ -129,7 +129,14 @@ void BookmarkBubbleView::Show(HWND parent,
 }
 
 BookmarkBubbleView::~BookmarkBubbleView() {
-  SetNodeTitleFromTextField();
+  if (apply_edits_) {
+    ApplyEdits();
+  } else if (remove_bookmark_) {
+    BookmarkModel* model = profile_->GetBookmarkModel();
+    BookmarkNode* node = model->GetMostRecentlyAddedNodeForURL(url_);
+    if (node)
+      model->Remove(node->GetParent(), node->GetParent()->IndexOfChild(node));
+  }
 }
 
 void BookmarkBubbleView::DidChangeBounds(const gfx::Rect& previous,
@@ -169,7 +176,9 @@ BookmarkBubbleView::BookmarkBubbleView(InfoBubbleDelegate* delegate,
       newly_bookmarked_(newly_bookmarked),
       parent_model_(
           profile_->GetBookmarkModel(),
-          profile_->GetBookmarkModel()->GetMostRecentlyAddedNodeForURL(url)) {
+          profile_->GetBookmarkModel()->GetMostRecentlyAddedNodeForURL(url)),
+      remove_bookmark_(false),
+      apply_edits_(true) {
   Init();
 }
 
@@ -281,7 +290,13 @@ void BookmarkBubbleView::ButtonPressed(views::NativeButton* sender) {
 
 void BookmarkBubbleView::LinkActivated(Link* source, int event_flags) {
   DCHECK(source == remove_link_);
-  RemoveBookmark();
+  UserMetrics::RecordAction(L"BookmarkBubble_Unstar", profile_);
+
+  // Set this so we remove the bookmark after the window closes.
+  remove_bookmark_ = true;
+  apply_edits_ = false;
+
+  Close();
 }
 
 void BookmarkBubbleView::ItemChanged(ComboBox* combo_box,
@@ -293,20 +308,17 @@ void BookmarkBubbleView::ItemChanged(ComboBox* combo_box,
     ShowEditor();
     return;
   }
-  BookmarkModel* model = profile_->GetBookmarkModel();
-  BookmarkNode* node = model->GetMostRecentlyAddedNodeForURL(url_);
-  if (node) {
-    BookmarkNode* new_parent = parent_model_.GetNodeAt(new_index);
-    if (new_parent != node->GetParent()) {
-      UserMetrics::RecordAction(L"BookmarkBubble_ChangeParent", profile_);
-      model->Move(node, new_parent, new_parent->GetChildCount());
-    }
-  }
 }
 
-void BookmarkBubbleView::InfoBubbleClosing(InfoBubble* info_bubble) {
+void BookmarkBubbleView::InfoBubbleClosing(InfoBubble* info_bubble,
+                                           bool closed_by_escape) {
+  if (closed_by_escape) {
+    remove_bookmark_ = newly_bookmarked_;
+    apply_edits_ = false;
+  }
+
   if (delegate_)
-    delegate_->InfoBubbleClosing(info_bubble);
+    delegate_->InfoBubbleClosing(info_bubble, closed_by_escape);
   NotificationService::current()->Notify(
       NOTIFY_BOOKMARK_BUBBLE_HIDDEN,
       Source<Profile>(profile_->GetOriginalProfile()),
@@ -321,26 +333,12 @@ void BookmarkBubbleView::Close() {
   static_cast<InfoBubble*>(GetWidget())->Close();
 }
 
-void BookmarkBubbleView::RemoveBookmark() {
-  UserMetrics::RecordAction(L"BookmarkBubble_Unstar", profile_);
-
-  GURL url = url_;
-  BookmarkModel* model = profile_->GetBookmarkModel();
-  // Close first, then notify the service. That way we know we won't be
-  // visible and don't have to worry about some other window becoming
-  // activated and deleting us before we invoke Close.
-  Close();
-  // WARNING: we've likely been deleted.
-  if (model)
-    model->SetURLStarred(url, std::wstring(), false);
-}
-
 void BookmarkBubbleView::ShowEditor() {
   BookmarkNode* node =
       profile_->GetBookmarkModel()->GetMostRecentlyAddedNodeForURL(url_);
 
-  // The user may have edited the title, commit it now.
-  SetNodeTitleFromTextField();
+  // Commit any edits now.
+  ApplyEdits();
 
   // Parent the editor to our root ancestor (not the root we're in, as that
   // is the info bubble and will close shortly).
@@ -364,7 +362,10 @@ void BookmarkBubbleView::ShowEditor() {
                              BookmarkEditorView::SHOW_TREE, NULL);
 }
 
-void BookmarkBubbleView::SetNodeTitleFromTextField() {
+void BookmarkBubbleView::ApplyEdits() {
+  // Set this to make sure we don't attempt to apply edits again.
+  apply_edits_ = false;
+
   BookmarkModel* model = profile_->GetBookmarkModel();
   BookmarkNode* node = model->GetMostRecentlyAddedNodeForURL(url_);
   if (node) {
@@ -374,5 +375,15 @@ void BookmarkBubbleView::SetNodeTitleFromTextField() {
       UserMetrics::RecordAction(L"BookmarkBubble_ChangeTitleInBubble",
                                 profile_);
     }
+    // Last index means 'Choose another folder...'
+    if (parent_combobox_->GetSelectedItem() <
+        parent_model_.GetItemCount(parent_combobox_) - 1) {
+      BookmarkNode* new_parent =
+          parent_model_.GetNodeAt(parent_combobox_->GetSelectedItem());
+      if (new_parent != node->GetParent()) {
+        UserMetrics::RecordAction(L"BookmarkBubble_ChangeParent", profile_);
+        model->Move(node, new_parent, new_parent->GetChildCount());
+      }
+    }
   }
 }
diff --git a/chrome/browser/views/bookmark_bubble_view.h b/chrome/browser/views/bookmark_bubble_view.h
index 0e5cefe..c067f4a 100644
--- a/chrome/browser/views/bookmark_bubble_view.h
+++ b/chrome/browser/views/bookmark_bubble_view.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_VIEWS_BOOKMARK_BUBBLE_VIEW_H__
-#define CHROME_BROWSER_VIEWS_BOOKMARK_BUBBLE_VIEW_H__
+#ifndef CHROME_BROWSER_VIEWS_BOOKMARK_BUBBLE_VIEW_H_
+#define CHROME_BROWSER_VIEWS_BOOKMARK_BUBBLE_VIEW_H_
 
 #include "base/gfx/rect.h"
 #include "chrome/browser/views/info_bubble.h"
@@ -77,7 +77,7 @@ class BookmarkBubbleView : public views::View,
     std::vector<BookmarkNode*> nodes_;
     int node_parent_index_;
 
-    DISALLOW_EVIL_CONSTRUCTORS(RecentlyUsedFoldersModel);
+    DISALLOW_COPY_AND_ASSIGN(RecentlyUsedFoldersModel);
   };
 
   // Creates a BookmarkBubbleView.
@@ -108,20 +108,18 @@ class BookmarkBubbleView : public views::View,
   // InfoBubbleDelegate methods. These forward to the InfoBubbleDelegate
   // supplied in the constructor as well as sending out the necessary
   // notification.
-  virtual void InfoBubbleClosing(InfoBubble* info_bubble);
+  virtual void InfoBubbleClosing(InfoBubble* info_bubble,
+                                 bool closed_by_escape);
   virtual bool CloseOnEscape();
 
   // Closes the bubble.
   void Close();
 
-  // Removes the bookmark and closes the view.
-  void RemoveBookmark();
-
   // Shows the BookmarkEditor.
   void ShowEditor();
 
-  // Sets the title of the bookmark from the editor
-  void SetNodeTitleFromTextField();
+  // Sets the title and parent of the node.
+  void ApplyEdits();
 
   // Delegate for the bubble, may be null.
   InfoBubbleDelegate* delegate_;
@@ -157,8 +155,13 @@ class BookmarkBubbleView : public views::View,
   // the current parent.
   views::ComboBox* parent_combobox_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(BookmarkBubbleView);
-};
+  // When the destructor is invoked should the bookmark be removed?
+  bool remove_bookmark_;
 
-#endif  // CHROME_BROWSER_VIEWS_BOOKMARK_BUBBLE_VIEW_H__
+  // When the destructor is invoked should edits be applied?
+  bool apply_edits_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkBubbleView);
+};
 
+#endif  // CHROME_BROWSER_VIEWS_BOOKMARK_BUBBLE_VIEW_H_
diff --git a/chrome/browser/views/first_run_bubble.cc b/chrome/browser/views/first_run_bubble.cc
index 511d445..ac6678a 100644
--- a/chrome/browser/views/first_run_bubble.cc
+++ b/chrome/browser/views/first_run_bubble.cc
@@ -194,7 +194,8 @@ void FirstRunBubble::OnActivate(UINT action, BOOL minimized, HWND window) {
   InfoBubble::OnActivate(action, minimized, window);
 }
 
-void FirstRunBubble::InfoBubbleClosing(InfoBubble* info_bubble) {
+void FirstRunBubble::InfoBubbleClosing(InfoBubble* info_bubble,
+                                       bool closed_by_escape) {
   // Make sure our parent window is re-enabled.
   if (!IsWindowEnabled(GetParent()))
     ::EnableWindow(GetParent(), true);
diff --git a/chrome/browser/views/first_run_bubble.h b/chrome/browser/views/first_run_bubble.h
index 0f06c42..90cbf51 100644
--- a/chrome/browser/views/first_run_bubble.h
+++ b/chrome/browser/views/first_run_bubble.h
@@ -29,7 +29,8 @@ class FirstRunBubble : public InfoBubble,
   virtual void OnActivate(UINT action, BOOL minimized, HWND window);
 
   // InfoBubbleDelegate.
-  virtual void InfoBubbleClosing(InfoBubble* info_bubble);
+  virtual void InfoBubbleClosing(InfoBubble* info_bubble,
+                                 bool closed_by_escape);
   virtual bool CloseOnEscape() { return true; }
 
  private:
diff --git a/chrome/browser/views/info_bubble.cc b/chrome/browser/views/info_bubble.cc
index c896f33..d034a1d 100644
--- a/chrome/browser/views/info_bubble.cc
+++ b/chrome/browser/views/info_bubble.cc
@@ -71,13 +71,14 @@ InfoBubble* InfoBubble::Show(HWND parent_hwnd,
                              views::View* content,
                              InfoBubbleDelegate* delegate) {
   InfoBubble* window = new InfoBubble();
+  DLOG(WARNING) << "new bubble=" << window;
   window->Init(parent_hwnd, position_relative_to, content);
   window->ShowWindow(SW_SHOW);
   window->delegate_ = delegate;
   return window;
 }
 
-InfoBubble::InfoBubble() : content_view_(NULL) {
+InfoBubble::InfoBubble() : content_view_(NULL), closed_(false) {
 }
 
 InfoBubble::~InfoBubble() {
@@ -146,11 +147,7 @@ void InfoBubble::Init(HWND parent_hwnd,
 }
 
 void InfoBubble::Close() {
-  // We don't fade out because it looks terrible.
-  if (delegate_)
-    delegate_->InfoBubbleClosing(this);
-  parent_->DisableInactiveRendering(false);
-  WidgetWin::Close();
+  Close(false);
 }
 
 void InfoBubble::AnimationProgressed(const Animation* animation) {
@@ -168,7 +165,7 @@ void InfoBubble::AnimationProgressed(const Animation* animation) {
 bool InfoBubble::AcceleratorPressed(const views::Accelerator& accelerator) {
   DCHECK(accelerator.GetKeyCode() == VK_ESCAPE);
   if (!delegate_ || delegate_->CloseOnEscape()) {
-    Close();
+    Close(true);
     return true;
   }
   return false;
@@ -180,7 +177,7 @@ void InfoBubble::OnSize(UINT param, const CSize& size) {
 
 void InfoBubble::OnActivate(UINT action, BOOL minimized, HWND window) {
   // The popup should close when it is deactivated.
-  if (action == WA_INACTIVE) {
+  if (action == WA_INACTIVE && !closed_) {
     Close();
   } else if (action == WA_ACTIVE) {
     DCHECK(GetRootView()->GetChildViewCount() > 0);
@@ -192,6 +189,18 @@ InfoBubble::ContentView* InfoBubble::CreateContentView(View* content) {
   return new ContentView(content, this);
 }
 
+void InfoBubble::Close(bool closed_by_escape) {
+  if (closed_)
+    return;
+
+  // We don't fade out because it looks terrible.
+  if (delegate_)
+    delegate_->InfoBubbleClosing(this, closed_by_escape);
+  parent_->DisableInactiveRendering(false);
+  closed_ = true;
+  WidgetWin::Close();
+}
+
 // ContentView ----------------------------------------------------------------
 
 InfoBubble::ContentView::ContentView(views::View* content, InfoBubble* host)
@@ -410,4 +419,3 @@ gfx::Rect InfoBubble::ContentView::CalculateWindowBounds(
   }
   return gfx::Rect(x, y, pref.width(), pref.height());
 }
-
diff --git a/chrome/browser/views/info_bubble.h b/chrome/browser/views/info_bubble.h
index 21a4e2d..4e918425 100644
--- a/chrome/browser/views/info_bubble.h
+++ b/chrome/browser/views/info_bubble.h
@@ -26,7 +26,10 @@ class Window;
 class InfoBubbleDelegate {
  public:
   // Called when the InfoBubble is closing and is about to be deleted.
-  virtual void InfoBubbleClosing(InfoBubble* info_bubble) = 0;
+  // |closed_by_escape| is true if the close is the result of the user pressing
+  // escape.
+  virtual void InfoBubbleClosing(InfoBubble* info_bubble,
+                                 bool closed_by_escape) = 0;
 
   // Whether the InfoBubble should be closed when the Esc key is pressed.
   virtual bool CloseOnEscape() = 0;
@@ -148,6 +151,10 @@ class InfoBubble : public views::WidgetWin,
   virtual ContentView* CreateContentView(views::View* content);
 
  private:
+  // Closes the window notifying the delegate. |closed_by_escape| is true if
+  // the close is the result of pressing escape.
+  void Close(bool closed_by_escape);
+
   // The delegate notified when the InfoBubble is closed.
   InfoBubbleDelegate* delegate_;
 
@@ -160,8 +167,10 @@ class InfoBubble : public views::WidgetWin,
   // The fade-in animation.
   scoped_ptr<SlideAnimation> fade_animation_;
 
+  // Have we been closed?
+  bool closed_;
+
   DISALLOW_COPY_AND_ASSIGN(InfoBubble);
 };
 
 #endif  // CHROME_BROWSER_VIEWS_INFO_BUBBLE_H_
-
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index eb0a7cc..9bc23db 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -994,7 +994,8 @@ bool LocationBarView::SecurityImageView::OnMousePressed(
 }
 
 void LocationBarView::SecurityImageView::InfoBubbleClosing(
-    InfoBubble* info_bubble) {
+    InfoBubble* info_bubble,
+    bool closed_by_escape) {
   info_bubble_ = NULL;
 }
 
diff --git a/chrome/browser/views/location_bar_view.h b/chrome/browser/views/location_bar_view.h
index c594d54..054c79b 100644
--- a/chrome/browser/views/location_bar_view.h
+++ b/chrome/browser/views/location_bar_view.h
@@ -253,7 +253,7 @@ class LocationBarView : public views::View,
     virtual bool OnMousePressed(const views::MouseEvent& event);
 
     // InfoBubbleDelegate
-    void InfoBubbleClosing(InfoBubble* info_bubble);
+    void InfoBubbleClosing(InfoBubble* info_bubble, bool closed_by_escape);
     bool CloseOnEscape() { return true; }
 
     void set_profile(Profile* profile) { profile_ = profile; }
diff --git a/chrome/browser/views/toolbar_star_toggle.cc b/chrome/browser/views/toolbar_star_toggle.cc
index 9a485ee..7e43b2e 100644
--- a/chrome/browser/views/toolbar_star_toggle.cc
+++ b/chrome/browser/views/toolbar_star_toggle.cc
@@ -83,7 +83,8 @@ SkBitmap ToolbarStarToggle::GetImageToPaint() {
   return Button::GetImageToPaint();
 }
 
-void ToolbarStarToggle::InfoBubbleClosing(InfoBubble* info_bubble) {
+void ToolbarStarToggle::InfoBubbleClosing(InfoBubble* info_bubble,
+                                          bool closed_by_escape) {
   is_bubble_showing_ = false;
   SchedulePaint();
   bubble_closed_time_ = TimeTicks::Now();
diff --git a/chrome/browser/views/toolbar_star_toggle.h b/chrome/browser/views/toolbar_star_toggle.h
index 9537506..0553894 100644
--- a/chrome/browser/views/toolbar_star_toggle.h
+++ b/chrome/browser/views/toolbar_star_toggle.h
@@ -43,7 +43,8 @@ class ToolbarStarToggle : public views::ToggleButton,
 
  private:
   // InfoBubbleDelegate.
-  virtual void InfoBubbleClosing(InfoBubble* info_bubble);
+  virtual void InfoBubbleClosing(InfoBubble* info_bubble,
+                                 bool closed_by_escape);
   virtual bool CloseOnEscape();
 
   // Contains us.