Skip enhanced bookmarks cluster requests if it's not being used.

Skip enhanced bookmarks cluster requests if it's not being used.

BUG=424254
Committed: https://crrev.com/dbd94aa27449ebbc81d44daa8b791a204fbfae4b
Cr-Commit-Position: refs/heads/master@{#300410}

[Kibeom Kim (inactive), 2014-10-20 17:44:27]

kkimlabs@chromium.org changed reviewers:
+ danduong@chromium.org, mcolbert@chromium.org, noyau@chromium.org,
yfriedman@chromium.org

[noyau (Ping after 24h), 2014-10-20 18:38:42]

lgtm

[Mark, 2014-10-20 18:40:43]

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h
(right):

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:30:
virtual void AddObserver(BookmarkServerServiceObserver* observer);
I take it this still needs to be wired up somewhere in order to make this new
functionality work?  Sorry, I don't have the full context on these changes.

[danduong, 2014-10-20 18:45:39]

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:54:
if (model_->loaded()) {
Did we want to move this to the end of Extensive Change rather than sync cycle
completion?

Also, considering updating the comment to reflect that we want to avoid calling
this frequently.

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h
(right):

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:30:
virtual void AddObserver(BookmarkServerServiceObserver* observer);
On 2014/10/20 18:40:43, Mark wrote:
> I take it this still needs to be wired up somewhere in order to make this new
> functionality work?  Sorry, I don't have the full context on these changes.

Do we want an override keyword here?

https://codereview.chromium.org/665133004/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_server_service.h (right):

https://codereview.chromium.org/665133004/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_server_service.h:52: bool
MightHaveObservers();
Any reason we don't just expose observers as protected?

[Kibeom Kim (inactive), 2014-10-20 20:05:47]

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:54:
if (model_->loaded()) {
On 2014/10/20 18:45:38, danduong wrote:
> Did we want to move this to the end of Extensive Change rather than sync cycle
> completion?

noyau@: do you have any comment about this?

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h
(right):

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.h:30:
virtual void AddObserver(BookmarkServerServiceObserver* observer);
On 2014/10/20 18:45:38, danduong wrote:
> On 2014/10/20 18:40:43, Mark wrote:
> > I take it this still needs to be wired up somewhere in order to make this
new
> > functionality work?  Sorry, I don't have the full context on these changes.
> 
> Do we want an override keyword here?

override Done.

Yes we're calling |AddObserver(...)| here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/and...
only when we launch the bookmark UI. So it's already hooked up.

https://codereview.chromium.org/665133004/diff/1/components/enhanced_bookmark...
File components/enhanced_bookmarks/bookmark_server_service.h (right):

https://codereview.chromium.org/665133004/diff/1/components/enhanced_bookmark...
components/enhanced_bookmarks/bookmark_server_service.h:52: bool
MightHaveObservers();
On 2014/10/20 18:45:38, danduong wrote:
> Any reason we don't just expose observers as protected?

Done.

[danduong, 2014-10-20 21:05:20]

https://codereview.chromium.org/665133004/diff/20001/chrome/browser/enhanced_...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/665133004/diff/20001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:49:
void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
I guess my real concern with this is that we'll check for updates to clusters
even if there haven't been any changes to bookmarks.  As far as I understand,
SyncCycleCompleted will happen for all sync cycle events. Which may or may not
be triggered by actual changes to bookmarks (there is a periodic timer that
polls for changes for instance).

I think we'd want an event is more directly related to bookmark changes from
sync. But even as the code is, it is already better than what we had previously.

[noyau (Ping after 24h), 2014-10-20 22:19:54]

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/665133004/diff/1/chrome/browser/enhanced_book...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:54:
if (model_->loaded()) {
On 2014/10/20 20:05:47, Kibeom Kim wrote:
> On 2014/10/20 18:45:38, danduong wrote:
> > Did we want to move this to the end of Extensive Change rather than sync
cycle
> > completion?
> 
> noyau@: do you have any comment about this?

How would you know that an extensive change was actually triggered by sync and
not by something else?

The problem is that we lack a good notification. This code wants to know when
sync finished and there was bookmark updates. AFAIK there is no good API to do
this. OnSyncCycleCompleted() fires for any sync data and there is no guarantees
that an extensive change was done by sync.

[Kibeom Kim (inactive), 2014-10-21 00:40:33]

https://codereview.chromium.org/665133004/diff/20001/chrome/browser/enhanced_...
File chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc
(right):

https://codereview.chromium.org/665133004/diff/20001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:49:
void ChromeBookmarkServerClusterService::OnSyncCycleCompleted() {
On 2014/10/20 21:05:20, danduong wrote:
> I guess my real concern with this is that we'll check for updates to clusters
> even if there haven't been any changes to bookmarks.  As far as I understand,
> SyncCycleCompleted will happen for all sync cycle events. Which may or may not
> be triggered by actual changes to bookmarks (there is a periodic timer that
> polls for changes for instance).
> 
> I think we'd want an event is more directly related to bookmark changes from
> sync. But even as the code is, it is already better than what we had
previously.

discussed offline, and we decided to just comment it for now.

[Kibeom Kim (inactive), 2014-10-21 00:40:37]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:42:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665133004/60001

[commit-bot: I haz the power, 2014-10-21 02:45:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:45:40]

Exceeded time limit waiting for builds to trigger.

[Kibeom Kim (inactive), 2014-10-21 02:46:24]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:49:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665133004/60001

[commit-bot: I haz the power, 2014-10-21 03:27:40]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-21 03:32:42]

Patchset 4 (id:??) landed as
https://crrev.com/dbd94aa27449ebbc81d44daa8b791a204fbfae4b
Cr-Commit-Position: refs/heads/master@{#300410}