Only set remote id during url node creation.

Folders should only have their id set by the server, and the clients
should only set the remote id on clips they created themselves.

Additionally, EnhancedBookmarkModel now monitors the remote id field for bookmarks, and initiates a de-duping protocol whenever two (or more) nodes with the same id are detected.
BUG=413876

Review URL: https://codereview.chromium.org/563363002

Cr-Commit-Position: refs/heads/master@{#295790}

components/enhanced_bookmarks.gypi

@@ -31,6 +31,7 @@
         'enhanced_bookmarks/bookmark_server_service.h',
         'enhanced_bookmarks/enhanced_bookmark_model.cc',
         'enhanced_bookmarks/enhanced_bookmark_model.h',
+        'enhanced_bookmarks/enhanced_bookmark_model_observer.h',
         'enhanced_bookmarks/enhanced_bookmark_utils.cc',
         'enhanced_bookmarks/enhanced_bookmark_utils.h',
         'enhanced_bookmarks/image_store.cc',

components/enhanced_bookmarks/bookmark_server_search_service.cc

@@ -4,7 +4,6 @@
 
 #include "components/enhanced_bookmarks/bookmark_server_search_service.h"
 
-#include "components/bookmarks/browser/bookmark_model.h"
 #include "components/enhanced_bookmarks/enhanced_bookmark_utils.h"
 #include "components/enhanced_bookmarks/proto/search.pb.h"
 #include "net/base/url_util.h"
@@ -21,11 +20,11 @@ BookmarkServerSearchService::BookmarkServerSearchService(
     scoped_refptr<net::URLRequestContextGetter> request_context_getter,
     ProfileOAuth2TokenService* token_service,
     SigninManagerBase* signin_manager,
-    BookmarkModel* bookmark_model)
+    EnhancedBookmarkModel* enhanced_bookmark_model)
     : BookmarkServerService(request_context_getter,
                             token_service,
                             signin_manager,
-                            bookmark_model) {
+                            enhanced_bookmark_model) {
 }
 
 BookmarkServerSearchService::~BookmarkServerSearchService() {
@@ -99,17 +98,19 @@ void BookmarkServerSearchService::CleanAfterFailure() {
   searches_.clear();
 }
 
-void BookmarkServerSearchService::BookmarkNodeAdded(BookmarkModel* model,
-                                                    const BookmarkNode* parent,
-                                                    int index) {
-  BookmarkServerService::BookmarkNodeAdded(model, parent, index);
+void BookmarkServerSearchService::EnhancedBookmarkAdded(
+    const BookmarkNode* node) {
   searches_.clear();
 }
 
-void BookmarkServerSearchService::BookmarkMetaInfoChanged(
-    BookmarkModel* model,
-    const BookmarkNode* node) {
-  BookmarkServerService::BookmarkMetaInfoChanged(model, node);
+void BookmarkServerSearchService::EnhancedBookmarkAllUserNodesRemoved() {
+  searches_.clear();
+}
+
+void BookmarkServerSearchService::EnhancedBookmarkRemoteIdChanged(
+    const BookmarkNode* node,
+    const std::string& old_remote_id,
+    const std::string& remote_id) {
   searches_.clear();
 }
 }  // namespace enhanced_bookmarks

components/enhanced_bookmarks/bookmark_server_search_service.h

@@ -8,13 +8,14 @@
 #include <string>
 #include <vector>
 
+#include "components/bookmarks/browser/base_bookmark_model_observer.h"
 #include "components/enhanced_bookmarks/bookmark_server_service.h"
 #include "net/url_request/url_fetcher.h"
 
-class BookmarkModel;
-
 namespace enhanced_bookmarks {
 
+class EnhancedBookmarkModel;
+
 // Sends requests to the bookmark server to search for bookmarks relevant to a
 // given query. Will handle one outgoing request at a time.
 class BookmarkServerSearchService : public BookmarkServerService {
@@ -23,7 +24,7 @@ class BookmarkServerSearchService : public BookmarkServerService {
       scoped_refptr<net::URLRequestContextGetter> request_context_getter,
       ProfileOAuth2TokenService* token_service,
       SigninManagerBase* signin_manager,
-      BookmarkModel* bookmark_model);
+      EnhancedBookmarkModel* bookmark_model);
   virtual ~BookmarkServerSearchService();
 
   // Triggers a search. The query must not be empty. A call to this method
@@ -45,12 +46,15 @@ class BookmarkServerSearchService : public BookmarkServerService {
 
   virtual void CleanAfterFailure() OVERRIDE;
 
-  // BookmarkModelObserver methods.
-  virtual void BookmarkNodeAdded(BookmarkModel* model,
-                                 const BookmarkNode* parent,
-                                 int index) OVERRIDE;
-  virtual void BookmarkMetaInfoChanged(BookmarkModel* model,
-                                       const BookmarkNode* node) OVERRIDE;
+  // EnhancedBookmarkModelObserver methods.
+  virtual void EnhancedBookmarkModelLoaded() OVERRIDE{};
+  virtual void EnhancedBookmarkAdded(const BookmarkNode* node) OVERRIDE;
+  virtual void EnhancedBookmarkRemoved(const BookmarkNode* node) OVERRIDE{};
+  virtual void EnhancedBookmarkAllUserNodesRemoved() OVERRIDE;
+  virtual void EnhancedBookmarkRemoteIdChanged(
+      const BookmarkNode* node,
+      const std::string& old_remote_id,
+      const std::string& remote_id) OVERRIDE;
 
  private:
   // The search data, a map from query to a vector of stars.id.

components/enhanced_bookmarks/bookmark_server_service.cc

@@ -5,9 +5,7 @@
 #include "components/enhanced_bookmarks/bookmark_server_service.h"
 
 #include "base/auto_reset.h"
-#include "components/bookmarks/browser/bookmark_model.h"
-#include "components/bookmarks/browser/bookmark_model_observer.h"
-#include "components/enhanced_bookmarks/metadata_accessor.h"
+#include "components/enhanced_bookmarks/enhanced_bookmark_model.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "google_apis/gaia/gaia_constants.h"
@@ -21,24 +19,21 @@ BookmarkServerService::BookmarkServerService(
     scoped_refptr<net::URLRequestContextGetter> request_context_getter,
     ProfileOAuth2TokenService* token_service,
     SigninManagerBase* signin_manager,
-    BookmarkModel* bookmark_model)
+    EnhancedBookmarkModel* enhanced_bookmark_model)
     : OAuth2TokenService::Consumer("bookmark_server_service"),
-      bookmark_model_(bookmark_model),
+      model_(enhanced_bookmark_model),
       token_service_(token_service),
       signin_manager_(signin_manager),
-      request_context_getter_(request_context_getter),
-      inhibit_change_notifications_(false) {
+      request_context_getter_(request_context_getter) {
   DCHECK(request_context_getter.get());
   DCHECK(token_service);
   DCHECK(signin_manager);
-  DCHECK(bookmark_model);
-  bookmark_model_->AddObserver(this);
-  if (bookmark_model_->loaded())
-    BuildIdMap();
+  DCHECK(enhanced_bookmark_model);
+  model_->AddObserver(this);
 }
 
 BookmarkServerService::~BookmarkServerService() {
-  bookmark_model_->RemoveObserver(this);
+  model_->RemoveObserver(this);
 }
 
 void BookmarkServerService::AddObserver(
@@ -51,23 +46,6 @@ void BookmarkServerService::RemoveObserver(
   observers_.RemoveObserver(observer);
 }
 
-void BookmarkServerService::BuildIdMap() {
-  ui::TreeNodeIterator<const BookmarkNode> iterator(
-      bookmark_model_->root_node());
-
-  while (iterator.has_next()) {
-    const BookmarkNode* bookmark = iterator.Next();
-    if (bookmark_model_->is_permanent_node(bookmark))
-      continue;
-    // RemoteIdFromBookmark() will create the ID if it doesn't exists yet.
-    std::string starid =
-        enhanced_bookmarks::RemoteIdFromBookmark(bookmark_model_, bookmark);
-    if (bookmark->is_url()) {
-      starsid_to_bookmark_[starid] = bookmark;
-    }
-  }
-}
-
 const BookmarkNode* BookmarkServerService::BookmarkForRemoteId(
     const std::string& remote_id) const {
   std::map<std::string, const BookmarkNode*>::const_iterator it =
@@ -79,7 +57,7 @@ const BookmarkNode* BookmarkServerService::BookmarkForRemoteId(
 
 const std::string BookmarkServerService::RemoteIDForBookmark(
     const BookmarkNode* bookmark) const {
-  return enhanced_bookmarks::RemoteIdFromBookmark(bookmark_model_, bookmark);
+  return model_->GetRemoteId(bookmark);
 }
 
 void BookmarkServerService::Notify() {
@@ -165,66 +143,8 @@ void BookmarkServerService::OnURLFetchComplete(const net::URLFetcher* source) {
     Notify();
 }
 
-//
-// BookmarkModelObserver methods.
-//
-void BookmarkServerService::BookmarkModelLoaded(BookmarkModel* model,
-                                                bool ids_reassigned) {
-  BuildIdMap();
-}
-
-void BookmarkServerService::BookmarkNodeAdded(BookmarkModel* model,
-                                              const BookmarkNode* parent,
-                                              int index) {
-  DCHECK(!inhibit_change_notifications_);
-  const BookmarkNode* bookmark = parent->GetChild(index);
-  if (!bookmark->is_url())
-    return;
-
-  base::AutoReset<bool> inhibitor(&inhibit_change_notifications_, true);
-  std::string starid =
-      enhanced_bookmarks::RemoteIdFromBookmark(model, bookmark);
-  starsid_to_bookmark_[starid] = bookmark;
-}
-
-void BookmarkServerService::BookmarkNodeRemoved(
-    BookmarkModel* model,
-    const BookmarkNode* parent,
-    int old_index,
-    const BookmarkNode* node,
-    const std::set<GURL>& removed_urls) {
-  DCHECK(!inhibit_change_notifications_);
-  if (!node->is_url())
-    return;
-  base::AutoReset<bool> inhibitor(&inhibit_change_notifications_, true);
-  std::string starid = enhanced_bookmarks::RemoteIdFromBookmark(model, node);
-  starsid_to_bookmark_.erase(starid);
-}
-
-void BookmarkServerService::OnWillChangeBookmarkMetaInfo(
-    BookmarkModel* model,
-    const BookmarkNode* node) {
-  if (!node->is_url() || inhibit_change_notifications_)
-    return;
-  base::AutoReset<bool> inhibitor(&inhibit_change_notifications_, true);
-  std::string starid = enhanced_bookmarks::RemoteIdFromBookmark(model, node);
-  starsid_to_bookmark_.erase(starid);
-}
-
-void BookmarkServerService::BookmarkMetaInfoChanged(BookmarkModel* model,
-                                                    const BookmarkNode* node) {
-  if (!node->is_url() || inhibit_change_notifications_)
-    return;
-
-  std::string starid = enhanced_bookmarks::RemoteIdFromBookmark(model, node);
-  starsid_to_bookmark_[starid] = node;
-}
-
-void BookmarkServerService::BookmarkAllUserNodesRemoved(
-    BookmarkModel* model,
-    const std::set<GURL>& removed_urls) {
-  DCHECK(!inhibit_change_notifications_);
-  starsid_to_bookmark_.clear();
+void BookmarkServerService::EnhancedBookmarkModelShuttingDown() {
+  NOTREACHED();
 }
 
 SigninManagerBase* BookmarkServerService::GetSigninManager() {

components/enhanced_bookmarks/bookmark_server_service.h

@@ -8,20 +8,21 @@
 #include <string>
 #include <vector>
 
-#include "components/bookmarks/browser/bookmark_model_observer.h"
+#include "components/enhanced_bookmarks/enhanced_bookmark_model_observer.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "google_apis/gaia/oauth2_token_service.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
 
-class BookmarkModel;
 class ProfileOAuth2TokenService;
 class SigninManagerBase;
+class BookmarkNode;
 
 namespace enhanced_bookmarks {
 
 class BookmarkServerService;
+class EnhancedBookmarkModel;
 
 class BookmarkServerServiceObserver {
  public:
@@ -36,14 +37,14 @@ class BookmarkServerServiceObserver {
 // BookmarkNodes. Subclasses just have to provide the right query and the
 // parsing of the response.
 class BookmarkServerService : protected net::URLFetcherDelegate,
-                              protected BookmarkModelObserver,
-                              private OAuth2TokenService::Consumer {
+                              private OAuth2TokenService::Consumer,
+                              public EnhancedBookmarkModelObserver {
  public:
   BookmarkServerService(
       scoped_refptr<net::URLRequestContextGetter> request_context_getter,
       ProfileOAuth2TokenService* token_service,
       SigninManagerBase* signin_manager,
-      BookmarkModel* bookmark_model);
+      EnhancedBookmarkModel* enhanced_bookmark_model);
   virtual ~BookmarkServerService();
 
   void AddObserver(BookmarkServerServiceObserver* observer);
@@ -75,52 +76,19 @@ class BookmarkServerService : protected net::URLFetcherDelegate,
   // If the token can't be retrieved or the query fails this method is called.
   virtual void CleanAfterFailure() = 0;
 
-  // BookmarkModelObserver methods.
-  virtual void BookmarkModelLoaded(BookmarkModel* model,
-                                   bool ids_reassigned) OVERRIDE;
-  virtual void BookmarkNodeMoved(BookmarkModel* model,
-                                 const BookmarkNode* old_parent,
-                                 int old_index,
-                                 const BookmarkNode* new_parent,
-                                 int new_index) OVERRIDE {};
-  virtual void BookmarkNodeAdded(BookmarkModel* model,
-                                 const BookmarkNode* parent,
-                                 int index) OVERRIDE;
-  virtual void BookmarkNodeRemoved(BookmarkModel* model,
-                                   const BookmarkNode* parent,
-                                   int old_index,
-                                   const BookmarkNode* node,
-                                   const std::set<GURL>& removed_urls) OVERRIDE;
-  virtual void BookmarkNodeChanged(BookmarkModel* model,
-                                   const BookmarkNode* node) OVERRIDE {};
-  virtual void OnWillChangeBookmarkMetaInfo(BookmarkModel* model,
-                                            const BookmarkNode* node) OVERRIDE;
-
-  virtual void BookmarkMetaInfoChanged(BookmarkModel* model,
-                                       const BookmarkNode* node) OVERRIDE;
-
-  virtual void BookmarkNodeFaviconChanged(BookmarkModel* model,
-                                          const BookmarkNode* node) OVERRIDE {};
-  virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
-                                             const BookmarkNode* node)
-      OVERRIDE {};
-  virtual void BookmarkAllUserNodesRemoved(
-      BookmarkModel* model,
-      const std::set<GURL>& removed_urls) OVERRIDE;
+  // EnhancedBookmarkModelObserver:
+  virtual void EnhancedBookmarkModelShuttingDown() OVERRIDE;
 
   SigninManagerBase* GetSigninManager();
 
   // Cached pointer to the bookmarks model.
-  BookmarkModel* bookmark_model_;  // weak
+  EnhancedBookmarkModel* model_;  // weak
 
  private:
   FRIEND_TEST_ALL_PREFIXES(BookmarkServerServiceTest, Cluster);
   FRIEND_TEST_ALL_PREFIXES(BookmarkServerServiceTest,
                            ClearClusterMapOnRemoveAllBookmarks);
 
-  // Once the model is ready this method fills in the starsid_to_bookmark_ map.
-  void BuildIdMap();
-
   // net::URLFetcherDelegate methods. Called when the query is finished.
   virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
 
@@ -145,9 +113,6 @@ class BookmarkServerService : protected net::URLFetcherDelegate,
   scoped_ptr<net::URLFetcher> url_fetcher_;
   // A map from stars.id to bookmark nodes. With no null entries.
   std::map<std::string, const BookmarkNode*> starsid_to_bookmark_;
-  // Set to true during the creation of a new bookmark in order to send only the
-  // proper notification.
-  bool inhibit_change_notifications_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkServerService);
 };

components/enhanced_bookmarks/enhanced_bookmark_model.h

@@ -5,9 +5,13 @@
 #ifndef COMPONENTS_ENHANCED_BOOKMARKS_ENHANCED_BOOKMARK_MODEL_H_
 #define COMPONENTS_ENHANCED_BOOKMARKS_ENHANCED_BOOKMARK_MODEL_H_
 
+#include <map>
 #include <string>
 
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "base/strings/string16.h"
+#include "components/bookmarks/browser/base_bookmark_model_observer.h"
 #include "components/bookmarks/browser/bookmark_node.h"
 #include "components/keyed_service/core/keyed_service.h"
 
@@ -16,17 +20,29 @@ class Time;
 }  // namespace base
 
 class BookmarkModel;
+class BookmarkNode;
 class GURL;
 
+FORWARD_DECLARE_TEST(EnhancedBookmarkModelTest, SetMultipleMetaInfo);
+
 namespace enhanced_bookmarks {
+
+class EnhancedBookmarkModelObserver;
+
 // Wrapper around BookmarkModel providing utility functions for enhanced
 // bookmarks.
-class EnhancedBookmarkModel : public KeyedService {
+class EnhancedBookmarkModel : public KeyedService,
+                              public BaseBookmarkModelObserver {
  public:
   EnhancedBookmarkModel(BookmarkModel* bookmark_model,
                         const std::string& version);
   virtual ~EnhancedBookmarkModel();
 
+  virtual void ShutDown();
+
+  void AddObserver(EnhancedBookmarkModelObserver* observer);
+  void RemoveObserver(EnhancedBookmarkModelObserver* observer);
+
   // Moves |node| to |new_parent| and inserts it at the given |index|.
   void Move(const BookmarkNode* node,
             const BookmarkNode* new_parent,
@@ -47,6 +63,10 @@ class EnhancedBookmarkModel : public KeyedService {
   // Returns the remote id for a bookmark |node|.
   std::string GetRemoteId(const BookmarkNode* node);
 
+  // Returns the bookmark node corresponding to the given |remote_id|, or NULL
+  // if there is no node with the id.
+  const BookmarkNode* BookmarkForRemoteId(const std::string& remote_id);
+
   // Sets the description of a bookmark |node|.
   void SetDescription(const BookmarkNode* node, const std::string& description);
 
@@ -104,22 +124,77 @@ class EnhancedBookmarkModel : public KeyedService {
   // Remove when that is actually the case.
   BookmarkModel* bookmark_model() { return bookmark_model_; }
 
+  // Returns true if the enhanced bookmark model is done loading.
+  bool loaded() { return loaded_; }
+
  private:
-  // Generates and sets a remote id for the given bookmark |node|.
-  // Returns the id set.
-  std::string SetRemoteId(const BookmarkNode* node);
+  FRIEND_TEST_ALL_PREFIXES(::EnhancedBookmarkModelTest, SetMultipleMetaInfo);
+
+  typedef std::map<std::string, const BookmarkNode*> IdToNodeMap;
+  typedef std::map<const BookmarkNode*, std::string> NodeToIdMap;
+
+  // BaseBookmarkModelObserver:
+  virtual void BookmarkModelChanged() OVERRIDE;
+  virtual void BookmarkModelLoaded(BookmarkModel* model,
+                                   bool ids_reassigned) OVERRIDE;
+  virtual void BookmarkNodeAdded(BookmarkModel* model,
+                                 const BookmarkNode* parent,
+                                 int index) OVERRIDE;
+  virtual void BookmarkNodeRemoved(BookmarkModel* model,
+                                   const BookmarkNode* parent,
+                                   int old_index,
+                                   const BookmarkNode* node,
+                                   const std::set<GURL>& removed_urls) OVERRIDE;
+  virtual void OnWillChangeBookmarkMetaInfo(BookmarkModel* model,
+                                            const BookmarkNode* node) OVERRIDE;
+  virtual void BookmarkMetaInfoChanged(BookmarkModel* model,
+                                       const BookmarkNode* node) OVERRIDE;
+  virtual void BookmarkAllUserNodesRemoved(
+      BookmarkModel* model,
+      const std::set<GURL>& removed_urls) OVERRIDE;
+
+  // Initialize the mapping from remote ids to nodes.
+  void InitializeIdMap();
+
+  // Adds a node to the id map if it has a (unique) remote id. Must be followed
+  // by a (Schedule)ResetDuplicateRemoteIds call when done adding nodes.
+  void AddToIdMap(const BookmarkNode* node);
+
+  // If there are nodes that needs to reset their remote ids, schedules
+  // ResetDuplicateRemoteIds to be run asynchronously.
+  void ScheduleResetDuplicateRemoteIds();
+
+  // Clears out any duplicate remote ids detected by AddToIdMap calls.
+  void ResetDuplicateRemoteIds();
 
   // Helper method for setting a meta info field on a node. Also updates the
-  // version and userEdits fields.
+  // version field.
   void SetMetaInfo(const BookmarkNode* node,
                    const std::string& field,
-                   const std::string& value,
-                   bool user_edit);
+                   const std::string& value);
+
+  // Helper method for setting multiple meta info fields at once. All the fields
+  // in |meta_info| will be set, but the method will not delete fields not
+  // present.
+  void SetMultipleMetaInfo(const BookmarkNode* node,
+                           BookmarkNode::MetaInfoMap meta_info);
 
   // Returns the version string to use when setting stars.version.
   std::string GetVersionString();
 
   BookmarkModel* bookmark_model_;
+  bool loaded_;
+
+  ObserverList<EnhancedBookmarkModelObserver> observers_;
+
+  base::WeakPtrFactory<EnhancedBookmarkModel> weak_ptr_factory_;
+
+  IdToNodeMap id_map_;
+  NodeToIdMap nodes_to_reset_;
+
+  // Caches the remote id of a node before its meta info changes.
+  std::string prev_remote_id_;
+
   std::string version_;
   std::string version_suffix_;
 };

components/enhanced_bookmarks/enhanced_bookmark_model_observer.h

+// Copyright 2014 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.
+
+#ifndef COMPONENTS_ENHANCED_BOOKMARKS_ENHANCED_BOOKMARK_MODEL_OBSERVER_H_
+#define COMPONENTS_ENHANCED_BOOKMARKS_ENHANCED_BOOKMARK_MODEL_OBSERVER_H_
+
+#include <string>
+
+class BookmarkNode;
+
+namespace enhanced_bookmarks {
+
+class EnhancedBookmarkModelObserver {
+ public:
+  // Called when the model has finished loading.
+  virtual void EnhancedBookmarkModelLoaded() = 0;
+
+  // Called from EnhancedBookmarkModel::ShutDown.
+  virtual void EnhancedBookmarkModelShuttingDown() = 0;
+
+  // Called when a node is added to the model.
+  virtual void EnhancedBookmarkAdded(const BookmarkNode* node) = 0;
+
+  // Called when a node is removed from the model.
+  virtual void EnhancedBookmarkRemoved(const BookmarkNode* node) = 0;
+
+  // Called when all user editable nodes are removed from the model.
+  virtual void EnhancedBookmarkAllUserNodesRemoved() = 0;
+
+  // Called when the remote id of a node changes. If |remote_id| is empty, the
+  // remote id has been cleared. This could happen if multiple nodes with the
+  // same remote id has been detected.
+  virtual void EnhancedBookmarkRemoteIdChanged(const BookmarkNode* node,
+                                               const std::string& old_remote_id,
+                                               const std::string& remote_id) {};
+
+ protected:
+  virtual ~EnhancedBookmarkModelObserver() {}
+};
+
+}  // namespace enhanced_bookmarks
+#endif  // COMPONENTS_ENHANCED_BOOKMARKS_ENHANCED_BOOKMARK_MODEL_OBSERVER_H_