Only set remote id during url node creation.

components/enhanced_bookmarks/bookmark_server_search_service.cc

 // 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.
 
 #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"
 #include "net/url_request/url_fetcher.h"
 
 namespace {
 const std::string kSearchUrl(
     "https://www.google.com/stars/search");
 }  // namespace
 
 namespace enhanced_bookmarks {
 
 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() {
 }
 
 void BookmarkServerSearchService::Search(const std::string& query) {
   DCHECK(query.length());
   current_query_ = query;
   TriggerTokenRequest(true);
 }
@@ skipping 55 matching lines @@
   current_query_.clear();
   return true;
 }
 
 void BookmarkServerSearchService::CleanAfterFailure() {
   searches_.clear();
 }
 
 void BookmarkServerSearchService::BookmarkNodeAdded(BookmarkModel* model,
                                                     const BookmarkNode* parent,
                                                     int index) {

[noyau (Ping after 24h), 2014/09/17 08:24:35]

The superclass used to register for bookmark model changes, you ripped that code
off, so this methods are no longer called.

Note that all the existing subclasses (this one and the one in
https://codereview.chromium.org/539173004/) do need the model change
notification.

But the most problematic issue is that during those notifications the
extended_bookmark_model may change the maps id/bookmarks but the order in which
the observer are called is random, this could lead to subtle bugs.

But these services only needs to know when the maps id/bookmark changes (and
only a subset: id changed, id removed). Instead of being a model observer this
should be an extended model observer, only notified when the map of id to
bookmarks changes.

[Rune Fevang, 2014/09/17 22:51:04]

Oops! Put the inheritance back in the superclass since the subclasses all need
it. I'm not convinced that the subclasses (at least this one) need these events
though, see below.
How does the order matter for this code? All I see is a clearing of a map, which
would execute the same regardless of the status of the id map in
EnhancedBookmarkModel.

As a side question, why clear the searches when the model updates? Any ids
returned in the search result returns should still be good matches, why not just
omit ids you can't find from the results? This case could happen anyway, as you
are not guaranteed that the model has received updates that the server may know
about in any case.
I agree that the extended bookmark model should have it's own observer class
(loaded needs to be emitted after building the id map, separate events for
different meta info changes would be useful), however I considered it out of
scope for this CL. I see you're triggering functionality on the loaded event in
the CL you linked to however, so added an observer class.

[noyau (Ping after 24h), 2014/09/18 08:43:21]

If one register for the model notification and do anything with the stars id by
calling into extended_bookmark_model the data they get back may be different
depending on the order the observers are called. You're right that this is not
an issue
for this existing code, but it is a potential minefield.
What about additions? By dropping the data, the query will be redone and the new
data
might be picked up.

[Rune Fevang, 2014/09/19 01:18:07]

Hmm, I think there's an additional issue with the bookmark model observers use,
if we update the model from a listener callback, events will become
unpredictable for other listeners. If we're called first they'll see two
WillChange* calls followed by two Changed calls. Changed the code so any
updating of the model will be done asynchronously. That means other code will
temporarily see remote ids that are about to be changed, but I think that's OK.
OK, I didn't realize there was any requerying going on, as the subset of the
code I've been looking at just cleared a map. Since I have no idea how this code
is being used I'll leave it to you :)

-  BookmarkServerService::BookmarkNodeAdded(model, parent, index);
   searches_.clear();
 }
 
 void BookmarkServerSearchService::BookmarkMetaInfoChanged(
     BookmarkModel* model,
     const BookmarkNode* node) {
-  BookmarkServerService::BookmarkMetaInfoChanged(model, node);
   searches_.clear();

[noyau (Ping after 24h), 2014/09/18 08:43:21]

In this case you're right, just swapping the id would be sufficient.

 }
 }  // namespace enhanced_bookmarks