From ef7626467606a6993b1d3470f6c4688cd16368df Mon Sep 17 00:00:00 2001
From: "sky@google.com" <sky@google.com@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Thu, 5 Mar 2009 16:30:25 +0000
Subject: Changes bookmark sorting to sort folders first.

BUG=8338
TEST=covered by unit tests

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@10993 0039d316-1c4b-4281-b951-d872f2087c98
---
 chrome/browser/bookmarks/bookmark_model.cc         | 41 ++++++++++++++++++++--
 .../browser/bookmarks/bookmark_model_unittest.cc   | 15 +++++---
 2 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index 1ec048a..bedbac3 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -59,6 +59,35 @@ void BookmarkNode::Reset(const history::StarredEntry& entry) {
 
 // BookmarkModel --------------------------------------------------------------
 
+namespace {
+
+// Comparator used when sorting bookmarks. Folders are sorted first, then
+  // bookmarks.
+class SortComparator : public std::binary_function<BookmarkNode*,
+                                                   BookmarkNode*,
+                                                   bool> {
+ public:
+  explicit SortComparator(Collator* collator) : collator_(collator) { }
+
+  // Returns true if lhs preceeds rhs.
+  bool operator() (BookmarkNode* n1, BookmarkNode* n2) {
+    if (n1->GetType() == n2->GetType()) {
+      // Types are the same, compare the names.
+      if (!collator_)
+        return n1->GetTitle() < n2->GetTitle();
+      return l10n_util::CompareStringWithCollator(collator_, n1->GetTitle(),
+                                                  n2->GetTitle()) == UCOL_LESS;
+    }
+    // Types differ, sort such that folders come first.
+    return n1->is_folder();
+  }
+
+ private:
+  Collator* collator_;
+};
+
+}  // namespace
+
 BookmarkModel::BookmarkModel(Profile* profile)
     : profile_(profile),
       loaded_(false),
@@ -305,9 +334,15 @@ void BookmarkModel::SortChildren(BookmarkNode* parent) {
     return;
   }
 
-  l10n_util::SortStringsUsingMethod(g_browser_process->GetApplicationLocale(),
-                                    &(parent->children()),
-                                    &BookmarkNode::GetTitle);
+  UErrorCode error = U_ZERO_ERROR;
+  scoped_ptr<Collator> collator(
+      Collator::createInstance(
+          Locale(WideToUTF8(g_browser_process->GetApplicationLocale()).c_str()),
+          error));
+  if (U_FAILURE(error))
+    collator.reset(NULL);
+  std::sort(parent->children().begin(), parent->children().end(),
+            SortComparator(collator.get()));
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeChildrenReordered(this, parent));
diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc
index 0523ca2..974e618 100644
--- a/chrome/browser/bookmarks/bookmark_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc
@@ -842,11 +842,17 @@ TEST_F(BookmarkModelTestWithProfile2, RemoveNotification) {
 
 TEST_F(BookmarkModelTest, Sort) {
   // Populate the bookmark bar node with nodes for 'B', 'a', 'd' and 'C'.
+  // 'C' and 'a' are folders.
   TestNode bbn;
-  PopulateNodeFromString(L"B a d C", &bbn);
+  PopulateNodeFromString(L"B [ a ] d [ a ]", &bbn);
   BookmarkNode* parent = model.GetBookmarkBarNode();
   PopulateBookmarkNode(&bbn, &model, parent);
 
+  parent->GetChild(1)->SetTitle(L"a");
+  delete parent->GetChild(1)->Remove(0);
+  parent->GetChild(3)->SetTitle(L"C");
+  delete parent->GetChild(3)->Remove(0);
+
   ClearCounts();
 
   // Sort the children of the bookmark bar node.
@@ -855,9 +861,10 @@ TEST_F(BookmarkModelTest, Sort) {
   // Make sure we were notified.
   AssertObserverCount(0, 0, 0, 0, 1);
 
-  // Make sure the order matches.
+  // Make sure the order matches (remember, 'a' and 'C' are folders and
+  // come first).
   EXPECT_TRUE(parent->GetChild(0)->GetTitle() == L"a");
-  EXPECT_TRUE(parent->GetChild(1)->GetTitle() == L"B");
-  EXPECT_TRUE(parent->GetChild(2)->GetTitle() == L"C");
+  EXPECT_TRUE(parent->GetChild(1)->GetTitle() == L"C");
+  EXPECT_TRUE(parent->GetChild(2)->GetTitle() == L"B");
   EXPECT_TRUE(parent->GetChild(3)->GetTitle() == L"d");
 }
-- 
cgit v1.1