Reduce frequency of requesting bookmark clusters.

chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_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 "chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h"
 
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "components/enhanced_bookmarks/enhanced_bookmark_model.h"
 
 namespace enhanced_bookmarks {
@@ skipping 19 matching lines @@
 
 ChromeBookmarkServerClusterService::~ChromeBookmarkServerClusterService() {
   if (sync_service_)
     sync_service_->RemoveObserver(this);
 }
 
 void ChromeBookmarkServerClusterService::AddObserver(
     BookmarkServerServiceObserver* observer) {
   BookmarkServerClusterService::AddObserver(observer);
   if (sync_refresh_skipped_) {
+    TriggerTokenRequest(false);
     sync_refresh_skipped_ = false;
-    TriggerTokenRequest(false);
   }
 }
 
 void ChromeBookmarkServerClusterService::OnStateChanged() {
   // Do nothing.
 }
 
 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()) {
     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();
+}

[Kibeom Kim (inactive), 2014/10/28 17:27:08]

Q: Mark, will listening BookmarkAdded and removed be sufficient? Full events are
here
https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar...

[Mark, 2014/10/28 18:17:00]

IMO, yes.  You could listen on deletes too.  The key thing is if you don't
listen to deletes (we don't on desktop) then you hide clusters that have nothing
in them.

+
+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;

[Yaron, 2014/10/28 17:48:27]

This seems a little too optimistic and tailored to the above case. In most
cases, adding a bookmark should only result in a single sync cycle. So if you
simply add a bookmark, you may not get new clusters until some other arbitrary
sync action. Doesn't this break the basic use case?

[danduong, 2014/10/28 17:50:18]

It actually means you'll (at most) fetch twice as much as you should.

[Yaron, 2014/10/28 17:52:23]

Ugh. Of course. For some reason I assumed it was a backoff counter

+}
+
 }  // namespace enhanced_bookmarks