From 4e187ef6569e197a6fd446fa5c2733ae7f0138b4 Mon Sep 17 00:00:00 2001
From: "mrossetti@chromium.org"
 <mrossetti@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Thu, 11 Mar 2010 05:21:35 +0000
Subject: Add bookmark copying while dragging by holding down the <option> key.

BUG=35969
TEST=Hold down the <option> key while dragging a bookmark to a new location. Repeat with the following: 1) From bookmark bar to bookmark bar, 2) from bookmark bar into a folder, 3) from within a folder to a different place within the same folder, 4) from within a folder to the bookmark bar, 5) from within a folder to a different folder. Check to insure that the original bookmark (and its contents if it was a folder) are in both the original location and the new location.

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41252 0039d316-1c4b-4281-b951-d872f2087c98
---
 chrome/browser/bookmarks/bookmark_drag_data.h      |  3 +
 chrome/browser/bookmarks/bookmark_model.cc         | 27 ++++++++
 chrome/browser/bookmarks/bookmark_model.h          |  5 ++
 .../browser/bookmarks/bookmark_model_unittest.cc   | 80 ++++++++++++++++++++++
 chrome/browser/bookmarks/bookmark_utils.cc         |  3 +
 chrome/browser/bookmarks/bookmark_utils.h          |  3 +
 chrome/browser/cocoa/bookmark_bar_controller.mm    | 16 +++--
 .../cocoa/bookmark_bar_controller_unittest.mm      | 42 +++++++++++-
 .../cocoa/bookmark_bar_folder_controller.mm        | 16 +++--
 chrome/browser/cocoa/bookmark_bar_folder_view.mm   |  6 +-
 .../cocoa/bookmark_bar_folder_view_unittest.mm     |  4 +-
 chrome/browser/cocoa/bookmark_bar_view.mm          |  6 +-
 chrome/browser/cocoa/bookmark_bar_view_unittest.mm |  2 +-
 chrome/browser/cocoa/bookmark_button.h             |  9 ++-
 14 files changed, 205 insertions(+), 17 deletions(-)

diff --git a/chrome/browser/bookmarks/bookmark_drag_data.h b/chrome/browser/bookmarks/bookmark_drag_data.h
index 9d72456..e26c362 100644
--- a/chrome/browser/bookmarks/bookmark_drag_data.h
+++ b/chrome/browser/bookmarks/bookmark_drag_data.h
@@ -22,6 +22,9 @@ class OSExchangeData;
 class Pickle;
 class Profile;
 
+// TODO(mrossetti): Rename BookmarkDragData to BookmarkNodeData, update comment.
+// See: http://crbug.com/37891
+
 // BookmarkDragData is used to represent the following:
 //
 // . A single URL.
diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index ad180b2..73fc5ea 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -207,6 +207,33 @@ void BookmarkModel::Move(const BookmarkNode* node,
                                       new_parent, index));
 }
 
+void BookmarkModel::Copy(const BookmarkNode* node,
+                         const BookmarkNode* new_parent,
+                         int index) {
+  if (!loaded_ || !node || !IsValidIndex(new_parent, index, true) ||
+      is_root(new_parent) || is_permanent_node(node)) {
+    NOTREACHED();
+    return;
+  }
+
+  if (new_parent->HasAncestor(node)) {
+    // Can't make an ancestor of the node be a child of the node.
+    NOTREACHED();
+    return;
+  }
+
+  SetDateGroupModified(new_parent, Time::Now());
+  BookmarkDragData drag_data_(node);
+  std::vector<BookmarkDragData::Element> elements(drag_data_.elements);
+  bookmark_utils::CloneDragData(this, elements, new_parent, index);
+
+  if (store_.get())
+    store_->ScheduleSave();
+
+  FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
+                    BookmarkNodeAdded(this, new_parent, index));
+}
+
 const SkBitmap& BookmarkModel::GetFavIcon(const BookmarkNode* node) {
   DCHECK(node);
   if (!node->is_favicon_loaded()) {
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index 312d04c..391792f 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -222,6 +222,11 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
             const BookmarkNode* new_parent,
             int index);
 
+  // Duplicates a bookmark node and inserts it at a new location.
+  void Copy(const BookmarkNode* node,
+            const BookmarkNode* new_parent,
+            int index);
+
   // Returns the favicon for |node|. If the favicon has not yet been
   // loaded it is loaded and the observer of the model notified when done.
   const SkBitmap& GetFavIcon(const BookmarkNode* node);
diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc
index 2200f4e..2f7f585 100644
--- a/chrome/browser/bookmarks/bookmark_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc
@@ -36,6 +36,24 @@ void SwapDateAdded(BookmarkNode* n1, BookmarkNode* n2) {
   n2->set_date_added(tmp);
 }
 
+// Helper function used to verify node arrangements.
+std::wstring ModelStringFromNode(const BookmarkNode* node) {
+  // Since the children of the node are not available as a vector,
+  // we'll just have to do it the hard way.
+  int childCount = node->GetChildCount();
+  std::wstring childString;
+  for (int i = 0; i < childCount; ++i) {
+    const BookmarkNode* child = node->GetChild(i);
+    if (child->is_folder()) {
+      childString += child->GetTitle() + L":[ " + ModelStringFromNode(child)
+                     + L"] ";
+    } else {
+      childString += child->GetTitle() + L" ";
+    }
+  }
+  return childString;
+}
+
 }  // anonymous namespace
 
 class BookmarkModelTest : public testing::Test, public BookmarkModelObserver {
@@ -306,6 +324,68 @@ TEST_F(BookmarkModelTest, Move) {
   EXPECT_EQ(0, root->GetChildCount());
 }
 
+TEST_F(BookmarkModelTest, Copy) {
+  const BookmarkNode* root = model.GetBookmarkBarNode();
+  model.AddURL(root, 0, L"a", GURL("http://a.com"));
+  const BookmarkNode* node1 = model.AddGroup(root, 1, L"1");
+  model.AddURL(node1, 0, L"b", GURL("http://b.com"));
+  model.AddURL(node1, 1, L"c", GURL("http://c.com"));
+  model.AddURL(root, 2, L"d", GURL("http://d.com"));
+  const BookmarkNode* node2 = model.AddGroup(root, 3, L"2");
+  model.AddURL(node2, 0, L"e", GURL("http://e.com"));
+  model.AddURL(node2, 1, L"f", GURL("http://f.com"));
+  model.AddURL(node2, 2, L"g", GURL("http://g.com"));
+  model.AddURL(root, 4, L"h", GURL("http://d.com"));
+
+  // Validate initial model.
+  std::wstring actualModelString = ModelStringFromNode(root);
+  EXPECT_EQ(L"a 1:[ b c ] d 2:[ e f g ] h ", ModelStringFromNode(root));
+
+  // Copy 'd' to be after '1:b': URL item from bar to folder.
+  const BookmarkNode* nodeToCopy = root->GetChild(2);
+  const BookmarkNode* destination = root->GetChild(1);
+  model.Copy(nodeToCopy, destination, 1);
+  actualModelString = ModelStringFromNode(root);
+  EXPECT_EQ(L"a 1:[ b d c ] d 2:[ e f g ] h ", actualModelString);
+
+  // Copy '1:d' to be after 'a': URL item from folder to bar.
+  const BookmarkNode* group = root->GetChild(1);
+  nodeToCopy = group->GetChild(1);
+  model.Copy(nodeToCopy, root, 1);
+  actualModelString = ModelStringFromNode(root);
+  EXPECT_EQ(L"a d 1:[ b d c ] d 2:[ e f g ] h ", actualModelString);
+
+  // Copy '1' to be after '2:e': Folder from bar to folder.
+  nodeToCopy = root->GetChild(2);
+  destination = root->GetChild(4);
+  model.Copy(nodeToCopy, destination, 1);
+  actualModelString = ModelStringFromNode(root);
+  EXPECT_EQ(L"a d 1:[ b d c ] d 2:[ e 1:[ b d c ] f g ] h ", actualModelString);
+
+  // Copy '2:1' to be after '2:f': Folder within same folder.
+  group = root->GetChild(4);
+  nodeToCopy = group->GetChild(1);
+  model.Copy(nodeToCopy, group, 3);
+  actualModelString = ModelStringFromNode(root);
+  EXPECT_EQ(L"a d 1:[ b d c ] d 2:[ e 1:[ b d c ] f 1:[ b d c ] g ] h ",
+            actualModelString);
+
+  // Copy first 'd' to be after 'h': URL item within the bar.
+  nodeToCopy = root->GetChild(1);
+  model.Copy(nodeToCopy, root, 6);
+  actualModelString = ModelStringFromNode(root);
+  EXPECT_EQ(L"a d 1:[ b d c ] d 2:[ e 1:[ b d c ] f 1:[ b d c ] g ] h d ",
+            actualModelString);
+
+  // Copy '2' to be after 'a': Folder within the bar.
+  nodeToCopy = root->GetChild(4);
+  model.Copy(nodeToCopy, root, 1);
+  actualModelString = ModelStringFromNode(root);
+  EXPECT_EQ(L"a 2:[ e 1:[ b d c ] f 1:[ b d c ] g ] d 1:[ b d c ] "
+             "d 2:[ e 1:[ b d c ] f 1:[ b d c ] g ] h d ",
+            actualModelString);
+}
+
 // Tests that adding a URL to a folder updates the last modified time.
 TEST_F(BookmarkModelTest, ParentForNewNodes) {
   ASSERT_EQ(model.GetBookmarkBarNode(), model.GetParentForNewNodes());
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc b/chrome/browser/bookmarks/bookmark_utils.cc
index e43c89d..c31b821 100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -83,6 +83,9 @@ class NewBrowserPageNavigator : public PageNavigator {
   DISALLOW_COPY_AND_ASSIGN(NewBrowserPageNavigator);
 };
 
+// TODO(mrossetti): Rename CloneDragDataImpl to CloneBookmarkNodeImpl.
+// See: http://crbug.com/37891
+
 void CloneDragDataImpl(BookmarkModel* model,
                        const BookmarkDragData::Element& element,
                        const BookmarkNode* parent,
diff --git a/chrome/browser/bookmarks/bookmark_utils.h b/chrome/browser/bookmarks/bookmark_utils.h
index b63c848..b001a69 100644
--- a/chrome/browser/bookmarks/bookmark_utils.h
+++ b/chrome/browser/bookmarks/bookmark_utils.h
@@ -64,6 +64,9 @@ bool IsValidDropLocation(Profile* profile,
                          const BookmarkNode* drop_parent,
                          int index);
 
+// TODO(mrossetti): Rename CloneDragData to CloneBookmarkNode.
+// See: http://crbug.com/37891
+
 // Clones drag data, adding newly created nodes to |parent| starting at
 // |index_to_add_at|.
 void CloneDragData(BookmarkModel* model,
diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm
index 0e96495..d80a1cfe 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller.mm
@@ -708,7 +708,9 @@ const NSTimeInterval kBookmarkBarAnimationDuration = 0.12;
   return [buttons_ count];
 }
 
-- (BOOL)dragButton:(BookmarkButton*)sourceButton to:(NSPoint)point {
+- (BOOL)dragButton:(BookmarkButton*)sourceButton
+                to:(NSPoint)point
+              copy:(BOOL)copy {
   DCHECK([sourceButton isKindOfClass:[BookmarkButton class]]);
 
   const BookmarkNode* sourceNode = [sourceButton bookmarkNode];
@@ -717,9 +719,15 @@ const NSTimeInterval kBookmarkBarAnimationDuration = 0.12;
   int destIndex = [self indexForDragOfButton:sourceButton toPoint:point];
   if (destIndex >= 0 && sourceNode) {
     // Our destination parent is not sourceNode->GetParent()!
-    bookmarkModel_->Move(sourceNode,
-                         bookmarkModel_->GetBookmarkBarNode(),
-                         destIndex);
+    if (copy) {
+      bookmarkModel_->Copy(sourceNode,
+                           bookmarkModel_->GetBookmarkBarNode(),
+                           destIndex);
+    } else {
+      bookmarkModel_->Move(sourceNode,
+                           bookmarkModel_->GetBookmarkBarNode(),
+                           destIndex);
+    }
   } else {
     NOTREACHED();
   }
diff --git a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
index 9b08e90..a6fe547 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
@@ -901,21 +901,57 @@ TEST_F(BookmarkBarControllerTest, TestDragButton) {
   EXPECT_EQ([[bar_ buttons] count], arraysize(titles));
   EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:0] title] isEqual:@"a"]);
 
-  [bar_ dragButton:[[bar_ buttons] objectAtIndex:2] to:NSMakePoint(0, 0)];
+  [bar_ dragButton:[[bar_ buttons] objectAtIndex:2]
+                to:NSMakePoint(0, 0)
+              copy:NO];
   EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:0] title] isEqual:@"c"]);
+  // Make sure a 'copy' did not happen.
+  EXPECT_EQ([[bar_ buttons] count], arraysize(titles));
 
-  [bar_ dragButton:[[bar_ buttons] objectAtIndex:1] to:NSMakePoint(1000, 0)];
+  [bar_ dragButton:[[bar_ buttons] objectAtIndex:1]
+                to:NSMakePoint(1000, 0)
+              copy:NO];
   EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:0] title] isEqual:@"c"]);
   EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:1] title] isEqual:@"b"]);
   EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:2] title] isEqual:@"a"]);
+  EXPECT_EQ([[bar_ buttons] count], arraysize(titles));
 
   // Finally, a drop of the 1st between the next 2
   CGFloat x = NSMinX([[[bar_ buttons] objectAtIndex:2] frame]);
   x += [[bar_ view] frame].origin.x;
-  [bar_ dragButton:[[bar_ buttons] objectAtIndex:0] to:NSMakePoint(x, 0)];
+  [bar_ dragButton:[[bar_ buttons] objectAtIndex:0]
+                to:NSMakePoint(x, 0)
+              copy:NO];
   EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:0] title] isEqual:@"b"]);
   EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:1] title] isEqual:@"c"]);
   EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:2] title] isEqual:@"a"]);
+  EXPECT_EQ([[bar_ buttons] count], arraysize(titles));
+}
+
+TEST_F(BookmarkBarControllerTest, TestCopyButton) {
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+
+  GURL gurls[] = { GURL("http://www.google.com/a"),
+                   GURL("http://www.google.com/b"),
+                   GURL("http://www.google.com/c") };
+  std::wstring titles[] = { L"a", L"b", L"c" };
+  for (unsigned i = 0; i < arraysize(titles); i++) {
+    model->SetURLStarred(gurls[i], titles[i], true);
+  }
+  EXPECT_EQ([[bar_ buttons] count], arraysize(titles));
+  EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:0] title] isEqual:@"a"]);
+
+  // Drag 'a' between 'b' and 'c'.
+  CGFloat x = NSMinX([[[bar_ buttons] objectAtIndex:2] frame]);
+  x += [[bar_ view] frame].origin.x;
+  [bar_ dragButton:[[bar_ buttons] objectAtIndex:0]
+                to:NSMakePoint(x, 0)
+              copy:YES];
+  EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:0] title] isEqual:@"a"]);
+  EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:1] title] isEqual:@"b"]);
+  EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:2] title] isEqual:@"a"]);
+  EXPECT_TRUE([[[[bar_ buttons] objectAtIndex:3] title] isEqual:@"c"]);
+  EXPECT_EQ([[bar_ buttons] count], 4U);
 }
 
 // Fake a theme with colored text.  Apply it and make sure bookmark
diff --git a/chrome/browser/cocoa/bookmark_bar_folder_controller.mm b/chrome/browser/cocoa/bookmark_bar_folder_controller.mm
index f000750c..2dca753 100644
--- a/chrome/browser/cocoa/bookmark_bar_folder_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bar_folder_controller.mm
@@ -458,7 +458,9 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 
 // TODO(jrg): ARGH more code dup.
 // http://crbug.com/35966
-- (BOOL)dragButton:(BookmarkButton*)sourceButton to:(NSPoint)point {
+- (BOOL)dragButton:(BookmarkButton*)sourceButton
+                to:(NSPoint)point
+              copy:(BOOL)copy {
   DCHECK([sourceButton isKindOfClass:[BookmarkButton class]]);
 
   const BookmarkNode* sourceNode = [sourceButton bookmarkNode];
@@ -466,9 +468,15 @@ static BOOL ValueInRangeInclusive(CGFloat low, CGFloat value, CGFloat high) {
 
   int destIndex = [self indexForDragOfButton:sourceButton toPoint:point];
   if (destIndex >= 0 && sourceNode) {
-    [parentController_ bookmarkModel]->Move(sourceNode,
-                                            [parentButton_ bookmarkNode],
-                                            destIndex);
+    if (copy) {
+      [parentController_ bookmarkModel]->Copy(sourceNode,
+                                              [parentButton_ bookmarkNode],
+                                              destIndex);
+    } else {
+      [parentController_ bookmarkModel]->Move(sourceNode,
+                                              [parentButton_ bookmarkNode],
+                                              destIndex);
+    }
   } else {
     NOTREACHED();
   }
diff --git a/chrome/browser/cocoa/bookmark_bar_folder_view.mm b/chrome/browser/cocoa/bookmark_bar_folder_view.mm
index e8e91dd..6f0db17 100644
--- a/chrome/browser/cocoa/bookmark_bar_folder_view.mm
+++ b/chrome/browser/cocoa/bookmark_bar_folder_view.mm
@@ -158,7 +158,11 @@
   if (data && [info draggingSource]) {
     BookmarkButton* button = nil;
     [data getBytes:&button length:sizeof(button)];
-    doDrag = [[self controller] dragButton:button to:[info draggingLocation]];
+    BOOL copy =
+      [info draggingSourceOperationMask] & NSDragOperationMove ? NO : YES;
+    doDrag = [[self controller] dragButton:button
+                                        to:[info draggingLocation]
+                                      copy:copy];
   }
   return doDrag;
 }
diff --git a/chrome/browser/cocoa/bookmark_bar_folder_view_unittest.mm b/chrome/browser/cocoa/bookmark_bar_folder_view_unittest.mm
index a549538..c39ba86 100644
--- a/chrome/browser/cocoa/bookmark_bar_folder_view_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bar_folder_view_unittest.mm
@@ -71,7 +71,9 @@
   closedAll_ = YES;
 }
 
-- (BOOL)dragButton:(BookmarkButton*)sourceButton to:(NSPoint)point {
+- (BOOL)dragButton:(BookmarkButton*)sourceButton
+                to:(NSPoint)point
+              copy:(BOOL)copy {
   return NO;
 }
 
diff --git a/chrome/browser/cocoa/bookmark_bar_view.mm b/chrome/browser/cocoa/bookmark_bar_view.mm
index b1099b2..9e47614 100644
--- a/chrome/browser/cocoa/bookmark_bar_view.mm
+++ b/chrome/browser/cocoa/bookmark_bar_view.mm
@@ -220,7 +220,11 @@
   if (data && [info draggingSource]) {
     BookmarkButton* button = nil;
     [data getBytes:&button length:sizeof(button)];
-    rtn = [controller_ dragButton:button to:[info draggingLocation]];
+    BOOL copy =
+      [info draggingSourceOperationMask] & NSDragOperationMove ? NO : YES;
+    rtn = [controller_ dragButton:button
+                               to:[info draggingLocation]
+                             copy:copy];
   }
   return rtn;
 }
diff --git a/chrome/browser/cocoa/bookmark_bar_view_unittest.mm b/chrome/browser/cocoa/bookmark_bar_view_unittest.mm
index 600c91d..85b4f2d 100644
--- a/chrome/browser/cocoa/bookmark_bar_view_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bar_view_unittest.mm
@@ -65,7 +65,7 @@ const CGFloat kFakeIndicatorPos = 7.0;
 }
 
 // Fake a controller for callback ponging
-- (BOOL)dragButton:(BookmarkButton*)button to:(NSPoint)point {
+- (BOOL)dragButton:(BookmarkButton*)button to:(NSPoint)point copy:(BOOL)copy {
   pong_ = YES;
   return YES;
 }
diff --git a/chrome/browser/cocoa/bookmark_button.h b/chrome/browser/cocoa/bookmark_button.h
index 4401c10..0bc1994 100644
--- a/chrome/browser/cocoa/bookmark_button.h
+++ b/chrome/browser/cocoa/bookmark_button.h
@@ -55,8 +55,13 @@ class ThemeProvider;
 // Perform the actual DnD of a bookmark button.
 
 // |point| is in the base coordinate system of the destination window;
-// |it comes from an id<NSDraggingInfo>.
-- (BOOL)dragButton:(BookmarkButton*)sourceButton to:(NSPoint)point;
+// |it comes from an id<NSDraggingInfo>. |copy| is YES if a copy is to be
+// made and inserted into the new location while leaving the bookmark in
+// the old location, otherwise move the bookmark by removing from its old
+// location and inserting into the new location.
+- (BOOL)dragButton:(BookmarkButton *)sourceButton
+                to:(NSPoint)point
+              copy:(BOOL)copy;
 
 // Return YES if we should show the drop indicator, else NO.  In some
 // cases (e.g. hover open) we don't want to show the drop indicator.
-- 
cgit v1.1