Revert "Revert 84829 - Initial implementation of "Synced Bookmarks" folder."

chrome/browser/bookmarks/bookmark_model.cc

Index: chrome/browser/bookmarks/bookmark_model.cc
diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index b372855a4e25060f7ff65a2bdbb71d83609f642f..a4dd847da8da8c5fe4626871ced42e07b8aaf821 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -8,6 +8,7 @@
 #include <functional>
 
 #include "base/callback.h"
+#include "base/command_line.h"
 #include "base/memory/scoped_vector.h"
 #include "build/build_config.h"
 #include "chrome/browser/bookmarks/bookmark_index.h"
@@ -16,6 +17,7 @@
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/history/history_notifications.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/chrome_switches.h"
 #include "content/common/notification_service.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -61,6 +63,15 @@ void BookmarkNode::InvalidateFavicon() {
   favicon_ = SkBitmap();
 }
 
+bool BookmarkNode::IsVisible() const {

[sky, 2011/05/17 15:59:08]

Don't we want it visible if not empty too?

[Yaron, 2011/05/17 19:17:33]

I discussed this with Srikanth and the plan is to keep this as is for now
(namely, only make it visible if the flag is set).

[sky, 2011/05/17 20:00:05]

If we do that, couldn't that lead to weird states? For example, lets say you
enable the flag, bookmark google in the synced folder, unset the flag, relaunch
chrome, go to google, click the star, then the edit button. The sync folder
won't be shown, right? This scenario may cause logic problems in the editor and
other places.

[Yaron, 2011/05/17 21:38:37]

I think we'd be ok with some inconsistency with toggling the flag on and off,
since it's not really a user-supported option, but I am concerned about the
potential for a crash. Unsurprisingly, the situation you mentioned did crash so
I fixed it (see the diff in boomark_tree_model). Regardless, Srikanth said it's
fine to make it visible if it has children. I made the change and updated the
test.

[Yaron, 2011/05/17 21:40:13]

Err, it's in bookmark_editor_gtk.cc

+  if (type_ != BookmarkNode::SYNCED ||
+      CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableSyncedBookmarksFolder)) {
+    return true;
+  }
+  return false;
+}
+
 void BookmarkNode::Reset(const history::StarredEntry& entry) {
   DCHECK(entry.type != history::StarredEntry::URL || entry.url == url_);
 
@@ -75,6 +86,9 @@ void BookmarkNode::Reset(const history::StarredEntry& entry) {
     case history::StarredEntry::BOOKMARK_BAR:
       type_ = BookmarkNode::BOOKMARK_BAR;
       break;
+    case history::StarredEntry::SYNCED:
+        type_ = BookmarkNode::SYNCED;
+        break;
     case history::StarredEntry::OTHER:
       type_ = BookmarkNode::OTHER_NODE;
       break;
@@ -124,6 +138,7 @@ BookmarkModel::BookmarkModel(Profile* profile)
       root_(GURL()),
       bookmark_bar_node_(NULL),
       other_node_(NULL),
+      synced_node_(NULL),
       next_node_id_(1),
       observers_(ObserverList<BookmarkModelObserver>::NOTIFY_EXISTING_ONLY),
       loaded_signal_(TRUE, FALSE) {
@@ -259,7 +274,8 @@ void BookmarkModel::SetTitle(const BookmarkNode* node, const string16& title) {
   if (node->GetTitle() == title)
     return;
 
-  if (node == bookmark_bar_node_ || node == other_node_) {
+  if (node == bookmark_bar_node_ || node == other_node_ ||

[sky, 2011/05/17 15:59:08]

is_permanent_node

[Yaron, 2011/05/17 19:17:33]

Done.

+      node == synced_node_) {
     NOTREACHED();
     return;
   }
@@ -568,12 +584,14 @@ void BookmarkModel::DoneLoading(
   }
   bookmark_bar_node_ = details->release_bb_node();
   other_node_ = details->release_other_folder_node();
+  synced_node_ = details->release_synced_folder_node();
   index_.reset(details->release_index());
 
   // WARNING: order is important here, various places assume bookmark bar then
   // other node.
   root_.Add(bookmark_bar_node_, 0);
   root_.Add(other_node_, 1);
+  root_.Add(synced_node_, 2);
 
   {
     base::AutoLock url_lock(url_lock_);
@@ -719,14 +737,24 @@ BookmarkNode* BookmarkModel::CreateOtherBookmarksNode() {
   return CreateRootNodeFromStarredEntry(entry);
 }
 
+BookmarkNode* BookmarkModel::CreateSyncedBookmarksNode() {
+  history::StarredEntry entry;
+  entry.type = history::StarredEntry::SYNCED;
+  return CreateRootNodeFromStarredEntry(entry);
+}
+
 BookmarkNode* BookmarkModel::CreateRootNodeFromStarredEntry(
     const history::StarredEntry& entry) {
   DCHECK(entry.type == history::StarredEntry::BOOKMARK_BAR ||
-         entry.type == history::StarredEntry::OTHER);
+         entry.type == history::StarredEntry::OTHER ||
+         entry.type == history::StarredEntry::SYNCED);
   BookmarkNode* node = new BookmarkNode(generate_next_node_id(), GURL());
   node->Reset(entry);
   if (entry.type == history::StarredEntry::BOOKMARK_BAR) {
     node->set_title(l10n_util::GetStringUTF16(IDS_BOOMARK_BAR_FOLDER_NAME));
+  } else if (entry.type == history::StarredEntry::SYNCED) {
+    node->set_title(l10n_util::GetStringUTF16(
+        IDS_BOOMARK_BAR_SYNCED_FOLDER_NAME));
   } else {
     node->set_title(
         l10n_util::GetStringUTF16(IDS_BOOMARK_BAR_OTHER_FOLDER_NAME));
@@ -826,6 +854,8 @@ void BookmarkModel::SetFileChanged() {
 BookmarkLoadDetails* BookmarkModel::CreateLoadDetails() {
   BookmarkNode* bb_node = CreateBookmarkNode();
   BookmarkNode* other_folder_node = CreateOtherBookmarksNode();
+  BookmarkNode* synced_folder_node = CreateSyncedBookmarksNode();
   return new BookmarkLoadDetails(
-      bb_node, other_folder_node, new BookmarkIndex(profile()), next_node_id_);
+      bb_node, other_folder_node, synced_folder_node,
+      new BookmarkIndex(profile()), next_node_id_);
 }