Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1924443003:
Precaching should wait for sync service backend to initialize (Closed)
DescriptionPrecaching should wait for sync service backend to initialize
SyncStateChangedListener is used to wait until sync service is
initialized to start precaching. If the sync service state does not
get initialized after a timeout, precaching is cancelled.
BUG=606230
Committed: https://crrev.com/7651f94989178a20a2d74d17142493ee67e73892
Cr-Commit-Position: refs/heads/master@{#392278}
Patch Set 1 #Patch Set 2 : Wait for sync configuration done #
Total comments: 4
Patch Set 3 : Addressed zea comments #
Total comments: 6
Patch Set 4 : Addressed nits, fixed tests #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== Precaching should wait for sync service backend to initialize SyncStateChangedListener is used to wait until sync service is initialized to start precaching. If the sync service state does not get initialized after a timeout, precaching is cancelled. BUG=606230 ========== to ========== Precaching should wait for sync service backend to initialize SyncStateChangedListener is used to wait until sync service is initialized to start precaching. If the sync service state does not get initialized after a timeout, precaching is cancelled. BUG=606230 ==========
rajendrant@chromium.org changed reviewers: + bengr@chromium.org, twifkak@chromium.org
PTAL
rajendrant@chromium.org changed reviewers: + yfriedman@chromium.org
rajendrant@chromium.org changed reviewers: + zea@chromium.org
PTAL
On 2016/04/27 04:20:02, Raj wrote: > PTAL zea: chrome/browser/sync/* chrome/android/java/src/org/chromium/chrome/browser/sync/* yaron: ptal chrome/android/java/src/org/chromium/chrome/browser/precache/* Thanks
https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java (right): https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:60: if (mSyncService.isBackendInitialized() && mSyncService.isConfigurationDone()) { I don't think you really need to be checking isConfigurationDone. isSyncActive coupled with getActiveDataTypes to check the specific types you care about is a more reliable way to go (and doesn't require plumbing through a new method). It's probably good to have this class listen to a particular set of types anyways. https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:66: mSyncService.removeSyncStateChangedListener(this); Does it make sense to also listen to a "Sync stopped" event, in case the user disables sync?
PTAL https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java (right): https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:60: if (mSyncService.isBackendInitialized() && mSyncService.isConfigurationDone()) { On 2016/04/27 16:24:45, Nicolas Zea wrote: > I don't think you really need to be checking isConfigurationDone. isSyncActive > coupled with getActiveDataTypes to check the specific types you care about is a > more reliable way to go (and doesn't require plumbing through a new method). > It's probably good to have this class listen to a particular set of types > anyways. Done. https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:66: mSyncService.removeSyncStateChangedListener(this); On 2016/04/27 16:24:45, Nicolas Zea wrote: > Does it make sense to also listen to a "Sync stopped" event, in case the user > disables sync? Precache runs as a periodic task, without user intervention. So mostly user won't be changing sync settings, and I think timing-out is fine. That brings us to another question. When chrome starts as part of periodic task, do we need to continue with sync to finish, which takes ~5 seconds in my test, and 10-15 seconds from UMA histograms. Should precache task stop/cancel the sync. WDYT.
On 2016/04/27 23:40:16, Raj wrote: > PTAL > > https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java > (right): > > https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:60: > if (mSyncService.isBackendInitialized() && mSyncService.isConfigurationDone()) { > On 2016/04/27 16:24:45, Nicolas Zea wrote: > > I don't think you really need to be checking isConfigurationDone. isSyncActive > > coupled with getActiveDataTypes to check the specific types you care about is > a > > more reliable way to go (and doesn't require plumbing through a new method). > > It's probably good to have this class listen to a particular set of types > > anyways. > > Done. > > https://codereview.chromium.org/1924443003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:66: > mSyncService.removeSyncStateChangedListener(this); > On 2016/04/27 16:24:45, Nicolas Zea wrote: > > Does it make sense to also listen to a "Sync stopped" event, in case the user > > disables sync? > > Precache runs as a periodic task, without user intervention. > So mostly user won't be changing sync settings, and I think timing-out is fine. > > That brings us to another question. When chrome starts as part of periodic task, > do we need to continue with sync to finish, which takes ~5 seconds in my test, > and 10-15 seconds from UMA histograms. > Should precache task stop/cancel the sync. WDYT. Maybe for a follow-up CL. Since we've identified a good time to use the network, it might make sense to run sync as well.
Sync usage LGTM with some nits https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java (right): https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:69: if (mSyncService.isBackendInitialized() && mSyncService.isSyncActive() nit: isSyncActive already checks backend initialized. https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:75: private void onSyncDataTypeConfigurationDone() { nit: it would probably be more accurate to call this something else. ConfigurationDone has a specific meaning within sync. Maybe something like OnDataTypesActive would be better, and less likely to confuse.
lgtm https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java (right): https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:366: }, MAX_SYNC_SERVICE_INIT_TIMOUT_SECONDS * 1000); nit: move * 1000 to the constant
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Addressed nits https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java (right): https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/PrecacheController.java:366: }, MAX_SYNC_SERVICE_INIT_TIMOUT_SECONDS * 1000); On 2016/05/04 15:48:41, Yaron wrote: > nit: move * 1000 to the constant Done. https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java (right): https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:69: if (mSyncService.isBackendInitialized() && mSyncService.isSyncActive() On 2016/05/03 18:58:03, Nicolas Zea wrote: > nit: isSyncActive already checks backend initialized. Done. https://codereview.chromium.org/1924443003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/precache/SyncServiceInitializedNotifier.java:75: private void onSyncDataTypeConfigurationDone() { On 2016/05/03 18:58:03, Nicolas Zea wrote: > nit: it would probably be more accurate to call this something else. > ConfigurationDone has a specific meaning within sync. Maybe something like > OnDataTypesActive would be better, and less likely to confuse. Done.
Addressed nits
The CQ bit was checked by rajendrant@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yfriedman@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/1924443003/#ps100001 (title: "Addressed nits, fixed tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924443003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924443003/100001
Message was sent while issue was closed.
Description was changed from ========== Precaching should wait for sync service backend to initialize SyncStateChangedListener is used to wait until sync service is initialized to start precaching. If the sync service state does not get initialized after a timeout, precaching is cancelled. BUG=606230 ========== to ========== Precaching should wait for sync service backend to initialize SyncStateChangedListener is used to wait until sync service is initialized to start precaching. If the sync service state does not get initialized after a timeout, precaching is cancelled. BUG=606230 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Precaching should wait for sync service backend to initialize SyncStateChangedListener is used to wait until sync service is initialized to start precaching. If the sync service state does not get initialized after a timeout, precaching is cancelled. BUG=606230 ========== to ========== Precaching should wait for sync service backend to initialize SyncStateChangedListener is used to wait until sync service is initialized to start precaching. If the sync service state does not get initialized after a timeout, precaching is cancelled. BUG=606230 Committed: https://crrev.com/7651f94989178a20a2d74d17142493ee67e73892 Cr-Commit-Position: refs/heads/master@{#392278} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7651f94989178a20a2d74d17142493ee67e73892 Cr-Commit-Position: refs/heads/master@{#392278} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
