Cancel bookmark editing when changes happen suddenly.

BUG=http://crbug.com/32896, http://crbug.com/29122,
	   http://crbug.com/34369, http://crbug.com/34513,
	   http://crbug.com/34522

TEST=1)
Add some bookmarks.
Go to one of them.
Open the bookmark manager so that the current item is visible.
Click STAR to open the bubble.
Right-click on the item in the bookmark manager and pick Delete.
Watch bubble go away safely.
2) 
Create a bookmark folder.
Open the bookmark manager so that the folder is visible.
In Browser, right-click on folder in bar and pick Rename (to open sheet).
Now right-click that folder in the bookmark manager to edit it.
  And edit it in there...
On "return", the Edit sheet in the main browser window goes away.
3)
Right click any existing folder on BMB and Choose "Rename" option
(Leave sheet open...)
Open other new window via menu or dock and "Delete" the folder which we tried to "rename"  on first window
See sheet close in 1st window

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

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

6 files changed, 219 insertions, 6 deletions

diff --git a/chrome/browser/cocoa/bookmark_bubble_controller.h b/chrome/browser/cocoa/bookmark_bubble_controller.h
index 1c48a98..1a95300 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller.h
+++ b/chrome/browser/cocoa/bookmark_bubble_controller.h
@@ -4,7 +4,8 @@
 
 #import <Cocoa/Cocoa.h>
 #import "base/cocoa_protocols_mac.h"
-#include "base/scoped_nsobject.h"
+#include "base/scoped_ptr.h"
+#import "chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h"
 
 class BookmarkModel;
 class BookmarkNode;
@@ -25,6 +26,9 @@ class BookmarkNode;
 
   BOOL alreadyBookmarked_;
 
+  // Ping me when things change out from under us.
+  scoped_ptr<BookmarkModelObserverForCocoa> observer_;
+
   IBOutlet NSTextField* bigTitle_;   // "Bookmark" or "Bookmark Added!"
   IBOutlet NSTextField* nameTextField_;
   IBOutlet NSPopUpButton* folderPopUpButton_;
diff --git a/chrome/browser/cocoa/bookmark_bubble_controller.mm b/chrome/browser/cocoa/bookmark_bubble_controller.mm
index 06170c1..be1fd30 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bubble_controller.mm
@@ -2,11 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#import "chrome/browser/cocoa/bookmark_bubble_controller.h"
 #include "app/l10n_util_mac.h"
 #include "base/mac_util.h"
 #include "base/sys_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
-#import "chrome/browser/cocoa/bookmark_bubble_controller.h"
 #include "chrome/browser/metrics/user_metrics.h"
 #include "grit/generated_resources.h"
 
@@ -67,6 +67,15 @@
   [super dealloc];
 }
 
+// Close the bookmark bubble without changing anything.  Unlike a
+// typical dialog's OK/Cancel, where Cancel is "do nothing", all
+// buttons on the bubble have the capacity to change the bookmark
+// model.  This is an IBOutlet-looking entry point to remove the
+// dialog without touching the model.
+- (void)dismissWithoutEditing:(id)sender {
+  [self close];
+}
+
 - (void)parentWindowWillClose:(NSNotification*)notification {
   [self close];
 }
@@ -74,6 +83,7 @@
 - (void)windowWillClose:(NSNotification*)notification {
   // We caught a close so we don't need to watch for the parent closing.
   [[NSNotificationCenter defaultCenter] removeObserver:self];
+  observer_.reset(NULL);
   [self autorelease];
 }
 
@@ -99,6 +109,15 @@
 
   [self fillInFolderList];
 
+  // Ping me when things change out from under us.  Unlike a normal
+  // dialog, the bookmark bubble's cancel: means "don't add this as a
+  // bookmark", not "cancel editing".  We must take extra care to not
+  // touch the bookmark in this selector.
+  observer_.reset(new BookmarkModelObserverForCocoa(
+                      node_, model_,
+                      self,
+                      @selector(dismissWithoutEditing:)));
+
   [window makeKeyAndOrderFront:self];
 }
 
diff --git a/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h b/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h
new file mode 100644
index 0000000..01acb69
--- /dev/null
+++ b/chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h
@@ -0,0 +1,110 @@
+// Copyright (c) 2010 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.
+
+// C++ bridge class to send a selector to a Cocoa object when the
+// bookmark model changes.  Some Cocoa objects edit the bookmark model
+// and temporarily save a copy of the state (e.g. bookmark button
+// editor).  As a fail-safe, these objects want an easy cancel if the
+// model changes out from under them.  For example, if you have the
+// bookmark button editor sheet open, then edit the bookmark in the
+// bookmark manager, we'd want to simply cancel the editor.
+//
+// This class is conservative and may result in notifications which
+// aren't strictly necessary.  For example, node removal only needs to
+// cancel an edit if the removed node is a folder (editors often have
+// a list of "new parents").  But, just to be sure, notification
+// happens on any removal.
+
+#ifndef CHROME_BROWSER_COCOA_BOOKMARK_MODEL_OBSERVER_FOR_COCOA_H
+#define CHROME_BROWSER_COCOA_BOOKMARK_MODEL_OBSERVER_FOR_COCOA_H
+
+#import <Cocoa/Cocoa.h>
+
+#include "base/basictypes.h"
+#include "base/scoped_nsobject.h"
+#include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/bookmarks/bookmark_model_observer.h"
+
+class BookmarkModelObserverForCocoa : public BookmarkModelObserver {
+ public:
+  // When |node| in |model| changes, send |selector| to |object|.
+  // Assumes |selector| is a selector that takes one arg, like an
+  // IBOutlet.  The arg passed is nil.
+  BookmarkModelObserverForCocoa(const BookmarkNode* node,
+                                BookmarkModel* model,
+                                NSObject* object,
+                                SEL selector) {
+    DCHECK(node && model);
+    node_ = node;
+    model_ = model;
+    object_.reset([object retain]);
+    selector_ = selector;
+    model_->AddObserver(this);
+  }
+  virtual ~BookmarkModelObserverForCocoa() {
+    model_->RemoveObserver(this);
+  }
+
+  virtual void BookmarkModelBeingDeleted(BookmarkModel* model) {
+    Notify();
+  }
+  virtual void BookmarkNodeMoved(BookmarkModel* model,
+                                 const BookmarkNode* old_parent,
+                                 int old_index,
+                                 const BookmarkNode* new_parent,
+                                 int new_index) {
+    // Editors often have a tree of parents, so movement of folders
+    // must cause a cancel.
+      Notify();
+  }
+  virtual void BookmarkNodeRemoved(BookmarkModel* model,
+                                   const BookmarkNode* parent,
+                                   int old_index,
+                                   const BookmarkNode* node) {
+    // See comment in BookmarkNodeMoved.
+    Notify();
+  }
+  virtual void BookmarkNodeChanged(BookmarkModel* model,
+                                   const BookmarkNode* node) {
+    if (node_ == node)
+      Notify();
+  }
+  virtual void BookmarkImportBeginning(BookmarkModel* model) {
+    // Be conservative.
+    Notify();
+  }
+
+  // Some notifications we don't care about, but by being pure virtual
+  // in the base class we must implement them.
+  virtual void Loaded(BookmarkModel* model) {
+  }
+  virtual void BookmarkNodeAdded(BookmarkModel* model,
+                                 const BookmarkNode* parent,
+                                 int index) {
+  }
+  virtual void BookmarkNodeFavIconLoaded(BookmarkModel* model,
+                                         const BookmarkNode* node) {
+  }
+  virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                             const BookmarkNode* node) {
+  }
+
+  virtual void BookmarkImportEnding(BookmarkModel* model) {
+  }
+
+ private:
+  const BookmarkNode* node_;  // Weak; owned by a BookmarkModel.
+  BookmarkModel* model_;  // Weak; it is owned by a Profile.
+  scoped_nsobject<NSObject> object_;
+  SEL selector_;
+
+  void Notify() {
+    [object_ performSelector:selector_ withObject:nil];
+  }
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkModelObserverForCocoa);
+};
+
+#endif  // CHROME_BROWSER_COCOA_BOOKMARK_MODEL_OBSERVER_FOR_COCOA_H
+
diff --git a/chrome/browser/cocoa/bookmark_model_observer_for_cocoa_unittest.mm b/chrome/browser/cocoa/bookmark_model_observer_for_cocoa_unittest.mm
new file mode 100644
index 0000000..dc80711
--- /dev/null
+++ b/chrome/browser/cocoa/bookmark_model_observer_for_cocoa_unittest.mm
@@ -0,0 +1,67 @@
+// Copyright (c) 2010 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.
+
+#import <Cocoa/Cocoa.h>
+
+#include "base/scoped_ptr.h"
+#include "base/scoped_nsobject.h"
+#import "chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h"
+#import "chrome/browser/cocoa/browser_test_helper.h"
+#import "chrome/browser/cocoa/cocoa_test_helper.h"
+
+// Keep track of bookmark pings.
+@interface ObserverPingTracker : NSObject {
+ @public
+  int pings;
+}
+@end
+
+@implementation ObserverPingTracker
+- (void)pingMe:(id)sender {
+  pings++;
+}
+@end
+
+namespace {
+
+class BookmarkModelObserverForCocoaTest : public CocoaTest {
+ public:
+  BrowserTestHelper helper_;
+
+  BookmarkModelObserverForCocoaTest() {}
+  virtual ~BookmarkModelObserverForCocoaTest() {}
+ private:
+  DISALLOW_COPY_AND_ASSIGN(BookmarkModelObserverForCocoaTest);
+};
+
+
+TEST_F(BookmarkModelObserverForCocoaTest, TestCallback) {
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+  const BookmarkNode* node = model->AddURL(model->GetBookmarkBarNode(),
+                                           0, L"super",
+                                           GURL("http://www.google.com"));
+
+  scoped_nsobject<ObserverPingTracker>
+      pingCount([[ObserverPingTracker alloc] init]);
+
+  scoped_ptr<BookmarkModelObserverForCocoa>
+      observer(new BookmarkModelObserverForCocoa(node, model,
+                                                 pingCount,
+                                                 @selector(pingMe:)));
+
+  EXPECT_EQ(0, pingCount.get()->pings);
+
+  model->SetTitle(node, L"duper");
+  EXPECT_EQ(1, pingCount.get()->pings);
+  model->SetURL(node, GURL("http://www.google.com/reader"));
+  EXPECT_EQ(2, pingCount.get()->pings);
+
+  model->Move(node, model->other_node(), 0);
+  EXPECT_EQ(3, pingCount.get()->pings);
+
+  model->Remove(node->GetParent(), 0);
+  EXPECT_EQ(4, pingCount.get()->pings);
+}
+
+}  // namespace
diff --git a/chrome/browser/cocoa/bookmark_name_folder_controller.h b/chrome/browser/cocoa/bookmark_name_folder_controller.h
index e75b0b8..4a0c75f 100644
--- a/chrome/browser/cocoa/bookmark_name_folder_controller.h
+++ b/chrome/browser/cocoa/bookmark_name_folder_controller.h
@@ -7,10 +7,12 @@
 
 #import <Cocoa/Cocoa.h>
 
-#include "base/scoped_ptr.h"
 #include "base/scoped_nsobject.h"
+#include "base/scoped_ptr.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 
+class BookmarkModelObserverForCocoa;
+
 // A controller for dialog to let the user create a new folder or
 // rename an existing folder.  Accessible from a context menu on a
 // bookmark button or the bookmark bar.
@@ -21,8 +23,12 @@
 
   NSWindow* parentWindow_;  // weak
   Profile* profile_;  // weak
-  const BookmarkNode* node_;  // weak; owned by the model
+  // Weak; owned by the model.  Can be NULL (see below).
+  const BookmarkNode* node_;
   scoped_nsobject<NSString> initialName_;
+
+  // Ping me when things change out from under us.
+  scoped_ptr<BookmarkModelObserverForCocoa> observer_;
 }
 // If |node| is NULL, this is an "add folder" request.
 // Else it is a "rename an existing folder" request.
diff --git a/chrome/browser/cocoa/bookmark_name_folder_controller.mm b/chrome/browser/cocoa/bookmark_name_folder_controller.mm
index f7aad05..d72033e 100644
--- a/chrome/browser/cocoa/bookmark_name_folder_controller.mm
+++ b/chrome/browser/cocoa/bookmark_name_folder_controller.mm
@@ -2,12 +2,13 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#import "chrome/browser/cocoa/bookmark_name_folder_controller.h"
 #include "app/l10n_util.h"
 #include "app/l10n_util_mac.h"
 #include "base/mac_util.h"
 #include "base/sys_string_conversions.h"
 #include "chrome/browser/profile.h"
-#import "chrome/browser/cocoa/bookmark_name_folder_controller.h"
+#include "chrome/browser/cocoa/bookmark_model_observer_for_cocoa.h"
 #include "grit/generated_resources.h"
 
 @implementation BookmarkNameFolderController
@@ -37,8 +38,13 @@
   [nameField_ setStringValue:initialName_.get()];
 }
 
-// TODO(jrg): consider NSModalSession.
 - (void)runAsModalSheet {
+  // Ping me when things change out from under us.
+  observer_.reset(new BookmarkModelObserverForCocoa(
+                    node_, profile_->GetBookmarkModel(),
+                    self,
+                    @selector(cancel:)));
+
   [NSApp beginSheet:[self window]
      modalForWindow:parentWindow_
       modalDelegate:self
@@ -72,6 +78,7 @@
          returnCode:(int)returnCode
         contextInfo:(void*)contextInfo {
   [[self window] orderOut:self];
+  observer_.reset(NULL);
   [self autorelease];
 }