Reduce frequency of requesting bookmark clusters.

chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc

Index: chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
diff --git a/chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc b/chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
index e270c5356257974be9e464961bf984e4aa209e3e..df5ade76f676258ffcb95dbde1b5bc612b7251eb 100644
--- a/chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
+++ b/chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
@@ -37,8 +37,8 @@ void ChromeBookmarkServerClusterService::AddObserver(
     BookmarkServerServiceObserver* observer) {
   BookmarkServerClusterService::AddObserver(observer);
   if (sync_refresh_skipped_) {
-    sync_refresh_skipped_ = false;
     TriggerTokenRequest(false);
+    sync_refresh_skipped_ = false;
   }
 }
 
@@ -50,19 +50,55 @@ void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
   // The stars cluster API relies on the information in chrome-sync. Sending a
   // cluster request immediately after a bookmark is changed from the bookmark
   // observer notification will yield the wrong results. The request must be
-  // delayed until the sync cycle has completed. In fact, the ideal signal would
-  // be "bookmark changed by sync", but we don't have that yet, and this is a
-  // compromise.
+  // delayed until the sync cycle has completed.
   // Note that we will be skipping calling this cluster API if there is no
   // observer attached, because calling that is meaningless without UI to show.
-  if (model_->loaded()) {
+  // We also will avoid requesting for clusters if the bookmark data hasn't
+  // changed.
+  if (refreshes_needed_ > 0 && model_->loaded()) {

[noyau (Ping after 24h), 2014/10/29 10:23:04]

model_->loaded is now redundant as refreshes_needed_ greater than zero implies
that the model send notification that it can only send once the model is loaded.

Maybe move model_->loaded in a DCHECK() inside the condition instead?

[danduong, 2014/10/29 20:26:09]

Done.

[Yaron, 2014/10/29 23:07:56]

I agree with Eric that it should go inside this if, not before. Sync cycle can
happen independently of bookmark load. Actually, I don't see why it's even
necessary to have

     if (observers_.might_have_observers()) {
       TriggerTokenRequest(false);
       sync_refresh_skipped_ = false;
     } else {
       sync_refresh_skipped_ = true;
     }
+    --refreshes_needed_;
   }
 }
 
+void ChromeBookmarkServerClusterService::EnhancedBookmarkAdded(
+    const BookmarkNode* node) {
+  BookmarkServerClusterService::EnhancedBookmarkAdded(node);
+  InvalidateCache();
+}
+
+void ChromeBookmarkServerClusterService::EnhancedBookmarkRemoved(
+    const BookmarkNode* node) {
+  BookmarkServerClusterService::EnhancedBookmarkRemoved(node);
+  InvalidateCache();
+}
+
+void ChromeBookmarkServerClusterService::EnhancedBookmarkNodeChanged(
+    BookmarkModel* model,
+    const BookmarkNode* node) {
+  BookmarkServerClusterService::EnhancedBookmarkNodeChanged(model, node);
+  InvalidateCache();
+}
+
+void ChromeBookmarkServerClusterService::InvalidateCache() {
+  // Bookmark changes can happen locally or via sync. It is difficult to
+  // determine if a given SyncCycle contains all the local modifications.
+  //
+  // Consider the following sequence:
+  //  1. SyncCycleBeginning (bookmark version:1)
+  //  2. Bookmarks mutate locally (bookmark version:2)
+  //  3. SyncCycleCompleted (bookmark version:1)
+  //
+  // In this case, the bookmarks modified locally won't be sent to the server
+  // until the next SyncCycleCompleted.  Since we can't accurately determine
+  // if a bookmark change has been sent on a SyncCycleCompleted, we're always
+  // assuming that we need to wait for 2 sync cycles.
+  refreshes_needed_ = 2;
+}
+
 }  // namespace enhanced_bookmarks