Lands http://codereview.chromium.org/155128 for Thiago.

Description from Thiago:

Converting the history::StarredEntry::Type to a type defined in BookmarkNode.

BUG=NONE
TEST=NONE

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

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

6 files changed, 65 insertions, 48 deletions

diff --git a/chrome/browser/bookmarks/bookmark_codec.cc b/chrome/browser/bookmarks/bookmark_codec.cc
index c9c8366..4e4f5ed 100644
--- a/chrome/browser/bookmarks/bookmark_codec.cc
+++ b/chrome/browser/bookmarks/bookmark_codec.cc
@@ -138,7 +138,7 @@ Value* BookmarkCodec::EncodeNode(const BookmarkNode* node) {
   value->SetString(kNameKey, title);
   value->SetString(kDateAddedKey,
                    Int64ToWString(node->date_added().ToInternalValue()));
-  if (node->GetType() == history::StarredEntry::URL) {
+  if (node->GetType() == BookmarkNode::URL) {
     value->SetString(kTypeKey, kTypeURL);
     std::wstring url = UTF8ToWide(node->GetURL().possibly_invalid_spec());
     value->SetString(kURLKey, url);
@@ -203,8 +203,8 @@ bool BookmarkCodec::DecodeHelper(BookmarkNode* bb_node,
   // Need to reset the type as decoding resets the type to FOLDER. Similarly
   // we need to reset the title as the title is persisted and restored from
   // the file.
-  bb_node->SetType(history::StarredEntry::BOOKMARK_BAR);
-  other_folder_node->SetType(history::StarredEntry::OTHER);
+  bb_node->SetType(BookmarkNode::BOOKMARK_BAR);
+  other_folder_node->SetType(BookmarkNode::OTHER_NODE);
   bb_node->SetTitle(l10n_util::GetString(IDS_BOOMARK_BAR_FOLDER_NAME));
   other_folder_node->SetTitle(
       l10n_util::GetString(IDS_BOOMARK_BAR_OTHER_FOLDER_NAME));
@@ -266,7 +266,7 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value,
 
     if (parent)
       parent->Add(parent->GetChildCount(), node);
-    node->SetType(history::StarredEntry::URL);
+    node->SetType(BookmarkNode::URL);
     UpdateChecksumWithUrlNode(id_string, title, url_string);
   } else {
     std::wstring last_modified_date;
@@ -288,7 +288,7 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value,
       node->set_id(id);
     }
 
-    node->SetType(history::StarredEntry::USER_GROUP);
+    node->SetType(BookmarkNode::FOLDER);
     node->set_date_group_modified(Time::FromInternalValue(
         StringToInt64(WideToUTF16Hack(last_modified_date))));
 
@@ -296,6 +296,7 @@ bool BookmarkCodec::DecodeNode(const DictionaryValue& value,
       parent->Add(parent->GetChildCount(), node);
 
     UpdateChecksumWithFolderNode(id_string, title);
+
     if (!DecodeChildren(*static_cast<ListValue*>(child_values), node))
       return false;
   }
diff --git a/chrome/browser/bookmarks/bookmark_html_writer.cc b/chrome/browser/bookmarks/bookmark_html_writer.cc
index 9783d73..9348198 100644
--- a/chrome/browser/bookmarks/bookmark_html_writer.cc
+++ b/chrome/browser/bookmarks/bookmark_html_writer.cc
@@ -109,9 +109,9 @@ class Writer : public Task {
     IncrementIndent();
 
     if (!WriteNode(*static_cast<DictionaryValue*>(root_folder_value),
-                   history::StarredEntry::BOOKMARK_BAR) ||
+                   BookmarkNode::BOOKMARK_BAR) ||
         !WriteNode(*static_cast<DictionaryValue*>(other_folder_value),
-                   history::StarredEntry::OTHER)) {
+                   BookmarkNode::OTHER_NODE)) {
       return;
     }
 
@@ -204,7 +204,7 @@ class Writer : public Task {
 
   // Writes the node and all its children, returning true on success.
   bool WriteNode(const DictionaryValue& value,
-                 history::StarredEntry::Type folder_type) {
+                BookmarkNode::Type folder_type) {
     std::wstring title, date_added_string, type_string;
     if (!value.GetString(BookmarkCodec::kNameKey, &title) ||
         !value.GetString(BookmarkCodec::kDateAddedKey, &date_added_string) ||
@@ -245,7 +245,7 @@ class Writer : public Task {
       NOTREACHED();
       return false;
     }
-    if (folder_type != history::StarredEntry::OTHER) {
+    if (folder_type != BookmarkNode::OTHER_NODE) {
       // The other folder name is not written out. This gives the effect of
       // making the contents of the 'other folder' be a sibling to the bookmark
       // bar folder.
@@ -256,7 +256,7 @@ class Writer : public Task {
           !WriteTime(last_modified_date)) {
         return false;
       }
-      if (folder_type == history::StarredEntry::BOOKMARK_BAR) {
+      if (folder_type == BookmarkNode::BOOKMARK_BAR) {
         if (!Write(kBookmarkBar))
           return false;
         title = l10n_util::GetString(IDS_BOOMARK_BAR_FOLDER_NAME);
@@ -284,11 +284,11 @@ class Writer : public Task {
         return false;
       }
       if (!WriteNode(*static_cast<DictionaryValue*>(child_value),
-                     history::StarredEntry::USER_GROUP)) {
+                     BookmarkNode::FOLDER)) {
         return false;
       }
     }
-    if (folder_type != history::StarredEntry::OTHER) {
+    if (folder_type != BookmarkNode::OTHER_NODE) {
       // Close out the folder.
       DecrementIndent();
       if (!WriteIndent() ||
diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index 3a52ae6..5d87da1 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -44,17 +44,30 @@ void BookmarkNode::Initialize(int id) {
   id_ = id;
   loaded_favicon_ = false;
   favicon_load_handle_ = 0;
-  type_ = !url_.is_empty() ? history::StarredEntry::URL :
-          history::StarredEntry::BOOKMARK_BAR;
+  type_ = !url_.is_empty() ? URL : BOOKMARK_BAR;
   date_added_ = Time::Now();
 }
 
 void BookmarkNode::Reset(const history::StarredEntry& entry) {
-  DCHECK(entry.type != history::StarredEntry::URL ||
-         entry.url == url_);
+  DCHECK(entry.type != history::StarredEntry::URL || entry.url == url_);
 
   favicon_ = SkBitmap();
-  type_ = entry.type;
+  switch (entry.type) {
+    case history::StarredEntry::URL:
+      type_ = BookmarkNode::URL;
+      break;
+    case history::StarredEntry::USER_GROUP:
+      type_ = BookmarkNode::FOLDER;
+      break;
+    case history::StarredEntry::BOOKMARK_BAR:
+      type_ = BookmarkNode::BOOKMARK_BAR;
+      break;
+    case history::StarredEntry::OTHER:
+      type_ = BookmarkNode::OTHER_NODE;
+      break;
+    default:
+      NOTREACHED();
+  }
   date_added_ = entry.date_added;
   date_group_modified_ = entry.date_group_modified;
   SetTitle(entry.title);
@@ -287,7 +300,7 @@ const BookmarkNode* BookmarkModel::AddGroup(const BookmarkNode* parent,
                                             GURL());
   new_node->set_date_group_modified(Time::Now());
   new_node->SetTitle(title);
-  new_node->SetType(history::StarredEntry::USER_GROUP);
+  new_node->SetType(BookmarkNode::FOLDER);
 
   return AddNode(AsMutable(parent), index, new_node, false);
 }
@@ -318,7 +331,7 @@ const BookmarkNode* BookmarkModel::AddURLWithCreationTime(
   BookmarkNode* new_node = new BookmarkNode(generate_next_node_id(), url);
   new_node->SetTitle(title);
   new_node->set_date_added(creation_time);
-  new_node->SetType(history::StarredEntry::URL);
+  new_node->SetType(BookmarkNode::URL);
 
   {
     // Only hold the lock for the duration of the insert.
@@ -427,7 +440,7 @@ void BookmarkModel::RemoveNode(BookmarkNode* node,
     return;
   }
 
-  if (node->GetType() == history::StarredEntry::URL) {
+  if (node->GetType() == BookmarkNode::URL) {
     // NOTE: this is called in such a way that url_lock_ is already held. As
     // such, this doesn't explicitly grab the lock.
     NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(node);
@@ -558,7 +571,7 @@ BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
 
   index_->Add(node);
 
-  if (node->GetType() == history::StarredEntry::URL && !was_bookmarked) {
+  if (node->GetType() == BookmarkNode::URL && !was_bookmarked) {
     history::URLsStarredDetails details(true);
     details.changed_urls.insert(node->GetURL());
     NotificationService::current()->Notify(
@@ -578,7 +591,7 @@ const BookmarkNode* BookmarkModel::GetNodeByID(const BookmarkNode* node,
   if (node->id() == id)
     return node;
 
-  for (int i = 0; i < node->GetChildCount(); ++i) {
+  for (int i = 0, child_count = node->GetChildCount(); i < child_count; ++i) {
     const BookmarkNode* result = GetNodeByID(node->GetChild(i), id);
     if (result)
       return result;
@@ -592,7 +605,7 @@ bool BookmarkModel::IsValidIndex(const BookmarkNode* parent,
   return (parent && parent->is_folder() &&
           (index >= 0 && (index < parent->GetChildCount() ||
                           (allow_end && index == parent->GetChildCount()))));
-  }
+}
 
 void BookmarkModel::SetDateGroupModified(const BookmarkNode* parent,
                                          const Time time) {
@@ -648,7 +661,7 @@ void BookmarkModel::OnFavIconDataAvailable(
 }
 
 void BookmarkModel::LoadFavIcon(BookmarkNode* node) {
-  if (node->GetType() != history::StarredEntry::URL)
+  if (node->GetType() != BookmarkNode::URL)
     return;
 
   DCHECK(node->GetURL().is_valid());
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index 19b0ed0..9726624 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -44,6 +44,12 @@ class BookmarkNode : public TreeNode<BookmarkNode> {
   friend class BookmarkModel;
 
  public:
+  enum Type {
+    URL,
+    FOLDER,
+    BOOKMARK_BAR,
+    OTHER_NODE
+  };
   // Creates a new node with the specified url and id of 0
   explicit BookmarkNode(const GURL& url);
   // Creates a new node with the specified url and id.
@@ -62,8 +68,8 @@ class BookmarkNode : public TreeNode<BookmarkNode> {
   void set_id(int id) { id_ = id; }
 
   // Returns the type of this node.
-  history::StarredEntry::Type GetType() const { return type_; }
-  void SetType(history::StarredEntry::Type type) { type_ = type; }
+  BookmarkNode::Type GetType() const { return type_; }
+  void SetType(BookmarkNode::Type type) { type_ = type; }
 
   // Returns the time the bookmark/group was added.
   const base::Time& date_added() const { return date_added_; }
@@ -80,10 +86,10 @@ class BookmarkNode : public TreeNode<BookmarkNode> {
 
   // Convenience for testing if this nodes represents a group. A group is
   // a node whose type is not URL.
-  bool is_folder() const { return type_ != history::StarredEntry::URL; }
+  bool is_folder() const { return type_ != URL; }
 
   // Is this a URL?
-  bool is_url() const { return type_ == history::StarredEntry::URL; }
+  bool is_url() const { return type_ == URL; }
 
   // Returns the favicon. In nearly all cases you should use the method
   // BookmarkModel::GetFavicon rather than this. BookmarkModel::GetFavicon
@@ -140,8 +146,7 @@ class BookmarkNode : public TreeNode<BookmarkNode> {
   const GURL url_;
 
   // Type of node.
-  // TODO(sky): bug 1256202, convert this into a type defined here.
-  history::StarredEntry::Type type_;
+  BookmarkNode::Type type_;
 
   // Date we were created.
   base::Time date_added_;
diff --git a/chrome/browser/bookmarks/bookmark_model_test_utils.cc b/chrome/browser/bookmarks/bookmark_model_test_utils.cc
index e8e9d40..402c566 100644
--- a/chrome/browser/bookmarks/bookmark_model_test_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_model_test_utils.cc
@@ -18,7 +18,7 @@ void BookmarkModelTestUtils::AssertNodesEqual(const BookmarkNode* expected,
   EXPECT_EQ(expected->GetTitle(), actual->GetTitle());
   EXPECT_EQ(expected->GetType(), actual->GetType());
   EXPECT_TRUE(expected->date_added() == actual->date_added());
-  if (expected->GetType() == history::StarredEntry::URL) {
+  if (expected->GetType() == BookmarkNode::URL) {
     EXPECT_EQ(expected->GetURL(), actual->GetURL());
   } else {
     EXPECT_TRUE(expected->date_group_modified() ==
diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc
index 1a914be..05d0843 100644
--- a/chrome/browser/bookmarks/bookmark_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc
@@ -154,12 +154,12 @@ TEST_F(BookmarkModelTest, InitialState) {
   const BookmarkNode* bb_node = model.GetBookmarkBarNode();
   ASSERT_TRUE(bb_node != NULL);
   EXPECT_EQ(0, bb_node->GetChildCount());
-  EXPECT_EQ(history::StarredEntry::BOOKMARK_BAR, bb_node->GetType());
+  EXPECT_EQ(BookmarkNode::BOOKMARK_BAR, bb_node->GetType());
 
   const BookmarkNode* other_node = model.other_node();
   ASSERT_TRUE(other_node != NULL);
   EXPECT_EQ(0, other_node->GetChildCount());
-  EXPECT_EQ(history::StarredEntry::OTHER, other_node->GetType());
+  EXPECT_EQ(BookmarkNode::OTHER_NODE, other_node->GetType());
 
   EXPECT_TRUE(bb_node->id() != other_node->id());
 }
@@ -176,7 +176,7 @@ TEST_F(BookmarkModelTest, AddURL) {
   ASSERT_EQ(1, root->GetChildCount());
   ASSERT_EQ(title, new_node->GetTitle());
   ASSERT_TRUE(url == new_node->GetURL());
-  ASSERT_EQ(history::StarredEntry::URL, new_node->GetType());
+  ASSERT_EQ(BookmarkNode::URL, new_node->GetType());
   ASSERT_TRUE(new_node == model.GetMostRecentlyAddedNodeForURL(url));
 
   EXPECT_TRUE(new_node->id() != root->id() &&
@@ -193,7 +193,7 @@ TEST_F(BookmarkModelTest, AddGroup) {
 
   ASSERT_EQ(1, root->GetChildCount());
   ASSERT_EQ(title, new_node->GetTitle());
-  ASSERT_EQ(history::StarredEntry::USER_GROUP, new_node->GetType());
+  ASSERT_EQ(BookmarkNode::FOLDER, new_node->GetType());
 
   EXPECT_TRUE(new_node->id() != root->id() &&
               new_node->id() != model.other_node()->id());
@@ -474,7 +474,7 @@ TEST_F(BookmarkModelTest, NotifyURLsStarred) {
 namespace {
 
 // See comment in PopulateNodeFromString.
-typedef TreeNodeWithValue<history::StarredEntry::Type> TestNode;
+typedef TreeNodeWithValue<BookmarkNode::Type> TestNode;
 
 // Does the work of PopulateNodeFromString. index gives the index of the current
 // element in description to process.
@@ -492,7 +492,7 @@ static void PopulateNodeImpl(const std::vector<std::wstring>& description,
       static int next_group_id = 1;
       TestNode* new_node =
           new TestNode(IntToWString(next_group_id++),
-                       history::StarredEntry::USER_GROUP);
+                       BookmarkNode::FOLDER);
       parent->Add(parent->GetChildCount(), new_node);
       PopulateNodeImpl(description, index, new_node);
     } else if (element == L"]") {
@@ -506,7 +506,7 @@ static void PopulateNodeImpl(const std::vector<std::wstring>& description,
       DCHECK(element.find('[') == std::string::npos);
       DCHECK(element.find(']') == std::string::npos);
       parent->Add(parent->GetChildCount(),
-                  new TestNode(element, history::StarredEntry::URL));
+                  new TestNode(element, BookmarkNode::URL));
     }
   }
 }
@@ -542,7 +542,7 @@ static void PopulateBookmarkNode(TestNode* parent,
                                  const BookmarkNode* bb_node) {
   for (int i = 0; i < parent->GetChildCount(); ++i) {
     TestNode* child = parent->GetChild(i);
-    if (child->value == history::StarredEntry::USER_GROUP) {
+    if (child->value == BookmarkNode::FOLDER) {
       const BookmarkNode* new_bb_node =
           model->AddGroup(bb_node, i, child->GetTitle());
       PopulateBookmarkNode(child, model, new_bb_node);
@@ -578,17 +578,15 @@ class BookmarkModelTestWithProfile : public testing::Test,
       TestNode* expected_child = expected->GetChild(i);
       const BookmarkNode* actual_child = actual->GetChild(i);
       ASSERT_EQ(expected_child->GetTitle(), actual_child->GetTitle());
-      if (expected_child->value == history::StarredEntry::USER_GROUP) {
-        ASSERT_TRUE(actual_child->GetType() ==
-                    history::StarredEntry::USER_GROUP);
+      if (expected_child->value == BookmarkNode::FOLDER) {
+        ASSERT_TRUE(actual_child->GetType() == BookmarkNode::FOLDER);
         // Recurse throught children.
         VerifyModelMatchesNode(expected_child, actual_child);
         if (HasFatalFailure())
           return;
       } else {
         // No need to check the URL, just the title is enough.
-        ASSERT_TRUE(actual_child->GetType() ==
-                    history::StarredEntry::URL);
+        ASSERT_TRUE(actual_child->GetType() == BookmarkNode::URL);
       }
     }
   }
@@ -722,7 +720,7 @@ class BookmarkModelTestWithProfile2 : public BookmarkModelTestWithProfile {
     ASSERT_EQ(2, bbn->GetChildCount());
 
     const BookmarkNode* child = bbn->GetChild(0);
-    ASSERT_EQ(history::StarredEntry::URL, child->GetType());
+    ASSERT_EQ(BookmarkNode::URL, child->GetType());
     ASSERT_EQ(L"Google", child->GetTitle());
     ASSERT_TRUE(child->GetURL() == GURL("http://www.google.com"));
 
@@ -733,7 +731,7 @@ class BookmarkModelTestWithProfile2 : public BookmarkModelTestWithProfile {
 
     const BookmarkNode* parent = child;
     child = parent->GetChild(0);
-    ASSERT_EQ(history::StarredEntry::URL, child->GetType());
+    ASSERT_EQ(BookmarkNode::URL, child->GetType());
     ASSERT_EQ(L"Google Advertising", child->GetTitle());
     ASSERT_TRUE(child->GetURL() == GURL("http://www.google.com/intl/en/ads/"));
 
@@ -744,7 +742,7 @@ class BookmarkModelTestWithProfile2 : public BookmarkModelTestWithProfile {
 
     parent = child;
     child = parent->GetChild(0);
-    ASSERT_EQ(history::StarredEntry::URL, child->GetType());
+    ASSERT_EQ(BookmarkNode::URL, child->GetType());
     ASSERT_EQ(L"Google Business Solutions", child->GetTitle());
     ASSERT_TRUE(child->GetURL() == GURL("http://www.google.com/services/"));
 
@@ -757,7 +755,7 @@ class BookmarkModelTestWithProfile2 : public BookmarkModelTestWithProfile {
     ASSERT_EQ(0, child->GetChildCount());
 
     child = parent->GetChild(1);
-    ASSERT_EQ(history::StarredEntry::URL, child->GetType());
+    ASSERT_EQ(BookmarkNode::URL, child->GetType());
     ASSERT_EQ(L"About Google", child->GetTitle());
     ASSERT_TRUE(child->GetURL() ==
                 GURL("http://www.google.com/intl/en/about.html"));