Reduce frequency of requesting bookmark clusters.

Reduce frequency of requesting bookmark clusters.

Currently, we fetch for bookmarks on SyncCycleCompleted.  We
actually just want to trigger a request for clusters when
bookmarks change due to a SyncCycle.  By watching for
ExtensiveChanges from the BookmarksModel and saving some
state, we can more accurately determine this.

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

[danduong, 2014-10-28 01:00:01]

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

[Kibeom Kim (inactive), 2014-10-28 05:31:31]

please hold review, there will be an update.

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

kkimlabs@chromium.org changed reviewers:
+ mcolbert@chromium.org

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

lgtm

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

https://codereview.chromium.org/682933002/diff/40001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:79:
}
Q: Mark, will listening BookmarkAdded and removed be sufficient? Full events are
here
https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar...

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

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

https://codereview.chromium.org/682933002/diff/40001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:94:
refreshes_needed_ = 2;
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]

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

https://codereview.chromium.org/682933002/diff/40001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:94:
refreshes_needed_ = 2;
On 2014/10/28 17:48:27, Yaron wrote:
> 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?

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

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

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

https://codereview.chromium.org/682933002/diff/40001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:94:
refreshes_needed_ = 2;
On 2014/10/28 17:50:18, danduong wrote:
> On 2014/10/28 17:48:27, Yaron wrote:
> > 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?
> 
> It actually means you'll (at most) fetch twice as much as you should.

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

[Mark, 2014-10-28 18:17:01]

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

https://codereview.chromium.org/682933002/diff/40001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:79:
}
On 2014/10/28 17:27:08, Kibeom Kim wrote:
> Q: Mark, will listening BookmarkAdded and removed be sufficient? Full events
are
> here
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar...

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.

[noyau (Ping after 24h), 2014-10-29 10:23:05]

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

https://codereview.chromium.org/682933002/diff/60001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:58:
if (refreshes_needed_ > 0 && model_->loaded()) {
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?

https://codereview.chromium.org/682933002/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model_observer.h (right):

https://codereview.chromium.org/682933002/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model_observer.h:31: const
BookmarkNode* node) = 0;
Why passing the bookmarkModel as an argument here? This is inconsistent with the
other methods in this file.

Either remove the argument, or pass the EnhancedBookmarkModel instead in all the
methods.

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

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

https://codereview.chromium.org/682933002/diff/60001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:58:
if (refreshes_needed_ > 0 && model_->loaded()) {
On 2014/10/29 10:23:04, noyau wrote:
> 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?

Done.

https://codereview.chromium.org/682933002/diff/60001/components/enhanced_book...
File components/enhanced_bookmarks/enhanced_bookmark_model_observer.h (right):

https://codereview.chromium.org/682933002/diff/60001/components/enhanced_book...
components/enhanced_bookmarks/enhanced_bookmark_model_observer.h:31: const
BookmarkNode* node) = 0;
On 2014/10/29 10:23:05, noyau wrote:
> Why passing the bookmarkModel as an argument here? This is inconsistent with
the
> other methods in this file.
> 
> Either remove the argument, or pass the EnhancedBookmarkModel instead in all
the
> methods.

Done.

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

lgtm

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

https://codereview.chromium.org/682933002/diff/60001/chrome/browser/enhanced_...
chrome/browser/enhanced_bookmarks/chrome_bookmark_server_cluster_service.cc:58:
if (refreshes_needed_ > 0 && model_->loaded()) {
On 2014/10/29 20:26:09, danduong wrote:
> On 2014/10/29 10:23:04, noyau wrote:
> > 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?
> 
> Done.

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

[noyau (Ping after 24h), 2014-10-30 10:22:58]

lgtm

[noyau (Ping after 24h), 2014-10-30 10:23:04]

The CQ bit was checked by noyau@chromium.org

[commit-bot: I haz the power, 2014-10-30 10:23:30]

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

[commit-bot: I haz the power, 2014-10-30 10:34:11]

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

[commit-bot: I haz the power, 2014-10-30 10:34:12]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[danduong, 2014-10-30 17:35:40]

The CQ bit was checked by danduong@chromium.org

[commit-bot: I haz the power, 2014-10-30 17:36:26]

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

[commit-bot: I haz the power, 2014-10-30 18:21:28]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-10-30 18:22:00]

Patchset 8 (id:??) landed as
https://crrev.com/2d0c8cfc2d67eea473132d8582430c21529ce897
Cr-Commit-Position: refs/heads/master@{#302111}

[csharp, 2014-10-30 18:45:43]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/693513003/ by csharp@chromium.org.

The reason for reverting is: This appears to have broken Chromium Linux on
Android Arm64 Builder (dbg).

FAILED: /mnt/data/b/build/goma/gomacc
/mnt/data/b/build/slave/Android_Arm64_Builder__dbg_/build/src/third_party/android_tools/ndk//toolchains/aarch64-linux-android-4.9/prebuilt/linux-x86_64/bin/aarch64-linux-android-g++
-MMD -MF
obj/chrome/browser/android/enhanced_bookmarks/browser.enhanced_bookmarks_bridge.o.d
-DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DDISABLE_NACL
-DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_WEBRTC=1
-DUSE_PROPRIETARY_CODECS -DENABLE_BROWSER_CDMS -DENABLE_CONFIGURATION_POLICY
-DDISCARDABLE_MEMORY_ALWAYS_SUPPORTED_NATIVELY
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_AUTOFILL_DIALOG=1
-DCLD_VERSION=1 -DENABLE_PRINTING=1 -DENABLE_MANAGED_USERS=1 -DVIDEO_HOLE=1
-DENABLE_LOAD_COMPLETION_HACKS=1 -DNACL_WINDOWS=0 -DNACL_LINUX=1 -DNACL_OSX=0
-DNACL_ANDROID=1 -DFULL_SAFE_BROWSING -DMOJO_USE_SYSTEM_IMPL -DPROTOBUF_USE_DLLS
-DGOOGLE_PROTOBUF_NO_RTTI -DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER
-DAUTOFILL_ENABLE_SYNC -DPASSWORD_MANAGER_ENABLE_SYNC
'-DPRECACHE_CONFIG_SETTINGS_URL="https://www.gstatic.com/chrome/wifiprefetch/precache_config"'
'-DPRECACHE_MANIFEST_URL_PREFIX="https://www.gstatic.com/chrome/wifiprefetch/precache_manifest_"'
-DAPPCACHE_USE_SIMPLE_CACHE -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1
'-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"'
-DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT
-DSK_FM_NEW_MATCH_FAMILY_STYLE_CHARACTER=1 -DSK_SUPPORT_LEGACY_TEXTRENDERMODE
-DSK_LEGACY_NO_DISTANCE_FIELD_PATHS -DSK_BUILD_FOR_ANDROID
-DSK_USE_POSIX_THREADS -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0
-DU_STATIC_IMPLEMENTATION -DFEATURE_ENABLE_SSL -DFEATURE_ENABLE_VOICEMAIL
-DEXPAT_RELATIVE_PATH -DGTEST_RELATIVE_PATH -DNO_MAIN_THREAD_WRAPPING
-DNO_SOUND_SYSTEM -DANDROID -DPOSIX -DWEBRTC_POSIX -DLIBXML_STATIC
-DPOSIX_AVOID_MMAP -DMEDIA_DISABLE_LIBVPX -DXML_STATIC -DWEBRTC_CHROMIUM_BUILD
-DWEBRTC_LINUX -DWEBRTC_ANDROID -DWEBRTC_ANDROID_OPENSLES
-DMESA_EGL_NO_X11_HEADERS -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__GNU_SOURCE=1 -DUSE_STLPORT=1
-D_STLP_USE_PTR_SPECIALIZATIONS=1 '-DCHROME_BUILD_ID=""' -DHAVE_SYS_UIO_H
-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_DEBUG -Igen
-I../.. -Iobj/chrome/browser.gen -I../../third_party/khronos -I../../gpu
-I../../skia/config -I../../third_party/WebKit/Source -Igen/angle
-Igen/protoc_out -I../../third_party/protobuf -I../../third_party/protobuf/src
-I../../third_party/dom_distiller_js/package/proto_gen -Igen/chrome
-Igen/components/strings -I../../third_party/WebKit
-I../../third_party/skia/src/core -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf
-I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy
-I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../skia/ext -I../../third_party/cacheinvalidation/overrides
-I../../third_party/cacheinvalidation/src
-I../../third_party/cacheinvalidation/google/cacheinvalidation
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-I../../third_party/webrtc/overrides -I../../third_party/libjingle/overrides
-I../../third_party/libjingle/source -I../../testing/gtest/include
-I../../third_party -I../../third_party/libxml/linux/include
-I../../third_party/libxml/src/include -I../../third_party/zlib
-Igen/ui/resources -I../../third_party/re2 -Igen/content/app/resources/
-I../../third_party/expat/files/lib
-I../../third_party/leveldatabase/src/include
-I../../third_party/leveldatabase/src -I../../third_party/leveldatabase
-I../../third_party/libyuv/include -I../../third_party/libyuv
-I../../third_party/npapi -I../../third_party/npapi/bindings -Igen/ui/gl
-I../../third_party/mesa/src/include -I../../v8/include -Igen/policy
--param=ssp-buffer-size=4 -Werror -fno-strict-aliasing -Wall
-Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe
-fPIC -Wno-unused-local-typedefs -ffunction-sections -funwind-tables -g
-fno-short-enums -finline-limit=64 -Wa,--noexecstack
--sysroot=/mnt/data/b/build/slave/Android_Arm64_Builder__dbg_/build/src/third_party/android_tools/ndk//platforms/android-21/arch-arm64
-isystem/mnt/data/b/build/slave/Android_Arm64_Builder__dbg_/build/src/third_party/android_tools/ndk//sources/cxx-stl/stlport/stlport
-Os -g -gdwarf-4 -fdata-sections -ffunction-sections -funwind-tables -g1
-fno-exceptions -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden
-Wsign-compare -std=gnu++11 -Wno-narrowing -Wno-literal-suffix  -c
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc -o
obj/chrome/browser/android/enhanced_bookmarks/browser.enhanced_bookmarks_bridge.o
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc: In
constructor
'enhanced_bookmarks::android::EnhancedBookmarksBridge::EnhancedBookmarksBridge(JNIEnv*,
jobject, Profile*)':
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:52:67:error:
invalid new-expression of abstract class type
'enhanced_bookmarks::BookmarkServerSearchService'
       EnhancedBookmarkModelFactory::GetForBrowserContext(profile_)));
                                                                   ^
In file included from
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:12:0,
                 from
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:5:
../../components/enhanced_bookmarks/bookmark_server_search_service.h:21:7: note:
  because the following virtual functions are pure within
'enhanced_bookmarks::BookmarkServerSearchService':
 class BookmarkServerSearchService : public BookmarkServerService {
       ^
In file included from
../../components/enhanced_bookmarks/bookmark_server_service.h:11:0,
                 from
../../components/enhanced_bookmarks/bookmark_server_search_service.h:12,
                 from
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:12,
                 from
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:5:
../../components/enhanced_bookmarks/enhanced_bookmark_model_observer.h:29:16:
note: 	virtual void
enhanced_bookmarks::EnhancedBookmarkModelObserver::EnhancedBookmarkNodeChanged(const
BookmarkNode*)
   virtual void EnhancedBookmarkNodeChanged(const BookmarkNode* node) = 0;
                ^
In file included from
../../storage/browser/fileapi/plugin_private_file_system_backend.h:13:0,
                 from ../../storage/browser/fileapi/file_system_context.h:20,
                 from ../../content/public/browser/content_browser_client.h:30,
                 from ../../chrome/browser/profiles/profile.h:17,
                 from
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.h:10,
                 from
../../chrome/browser/android/enhanced_bookmarks/enhanced_bookmarks_bridge.cc:5:
../../storage/browser/fileapi/file_system_backend.h: At global scope:
../../storage/browser/fileapi/file_system_backend.h:43:13:error:
'storage::kMaximumLength' defined but not used [-Werror=unused-variable]
 const int64 kMaximumLength = std::numeric_limits<int64>::max();.