Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289163003:
arc: Add arc enable sync pref initial set up. (Closed)
|
Created:
4 years, 3 months ago by lgcheng Modified:
4 years, 3 months ago Reviewers:
stevenjb CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, Matt Giuca Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Add arc enable sync pref initial set up.
Arc enable sync pref is false by default if user never modify sync settings. This
patch enables arc sync service to start when user never manually modify sync
settings.
BUG=642907
, http://b/31156650
TEST=Manual test on a machine that sync settings is never modified.
Committed: https://crrev.com/a9c8ea0daf53303475b32177b76938cf6b31c04d
Cr-Commit-Position: refs/heads/master@{#415748}
Patch Set 1 #Patch Set 2 #
Total comments: 8
Patch Set 3 #Messages
Total messages: 14 (6 generated)
Description was changed from ========== arc: Add enable sync pref initial set up. BUG= ========== to ========== arc: Add arc enable sync pref initial set up. Arc enable sync pref is false by default if user never modify sync settings. This patch enables arc sync service to start when user never manually modify sync settings. BUG= http://b/31156650 TEST=Manual test on a machine that sync settings is never modified. ==========
lgcheng@google.com changed reviewers: + stevenjb@chromium.org
Hi Steven, PTAL at the patch. This patch fix enable arc sync prefs initialization issue. Thanks! https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right): https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:78: // If device is set to sync everyghing, Arc package should be synced. If kSyncKeepEverythingSynced is true by default and never changed by user, both syncer::APPS and syncer::ARC_PACKAGE are false by default.
https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right): https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:76: bool GetEnableArcPackageSyncPref(Profile* profile) { This name is confusing since it may actually set something, we should call this something like "Validate" instead. https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:78: // If device is set to sync everyghing, Arc package should be synced. On 2016/08/31 17:53:16, lgcheng wrote: > If kSyncKeepEverythingSynced is true by default and never changed by user, both > syncer::APPS and syncer::ARC_PACKAGE are false by default. spelling: everything https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:83: sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::ARC_PACKAGE); Move this to where it is used
Thanks for comments! Issue addressed and PTAL. https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right): https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:76: bool GetEnableArcPackageSyncPref(Profile* profile) { On 2016/08/31 18:28:47, stevenjb wrote: > This name is confusing since it may actually set something, we should call this > something like "Validate" instead. Done. https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:78: // If device is set to sync everyghing, Arc package should be synced. On 2016/08/31 18:28:47, stevenjb wrote: > On 2016/08/31 17:53:16, lgcheng wrote: > > If kSyncKeepEverythingSynced is true by default and never changed by user, > both > > syncer::APPS and syncer::ARC_PACKAGE are false by default. > > spelling: everything Done. https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:83: sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::ARC_PACKAGE); On 2016/08/31 18:28:47, stevenjb wrote: > Move this to where it is used Done. But let me know if I misunderstand anything. like use "sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::ARC_PACKAGE)" directly and don't use a local pointer.
lgtm https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right): https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:83: sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::ARC_PACKAGE); On 2016/08/31 19:46:04, lgcheng wrote: > On 2016/08/31 18:28:47, stevenjb wrote: > > Move this to where it is used > > Done. > But let me know if I misunderstand anything. like use > "sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::ARC_PACKAGE)" directly > and don't use a local pointer. That is what I meant. It was just confusing here since it doesn't apply to the next line (but names are similar). Since we use it twice, a local pointer is fine.
On 2016/08/31 20:05:04, stevenjb wrote: > lgtm > > https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... > File chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc (right): > > https://codereview.chromium.org/2289163003/diff/20001/chrome/browser/ui/app_l... > chrome/browser/ui/app_list/arc/arc_package_syncable_service.cc:83: > sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::ARC_PACKAGE); > On 2016/08/31 19:46:04, lgcheng wrote: > > On 2016/08/31 18:28:47, stevenjb wrote: > > > Move this to where it is used > > > > Done. > > But let me know if I misunderstand anything. like use > > "sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::ARC_PACKAGE)" directly > > and don't use a local pointer. > > That is what I meant. It was just confusing here since it doesn't apply to the > next line (but names are similar). Since we use it twice, a local pointer is > fine. Thanks for review!
The CQ bit was checked by lgcheng@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== arc: Add arc enable sync pref initial set up. Arc enable sync pref is false by default if user never modify sync settings. This patch enables arc sync service to start when user never manually modify sync settings. BUG= http://b/31156650 TEST=Manual test on a machine that sync settings is never modified. ========== to ========== arc: Add arc enable sync pref initial set up. Arc enable sync pref is false by default if user never modify sync settings. This patch enables arc sync service to start when user never manually modify sync settings. BUG= http://b/31156650 TEST=Manual test on a machine that sync settings is never modified. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== arc: Add arc enable sync pref initial set up. Arc enable sync pref is false by default if user never modify sync settings. This patch enables arc sync service to start when user never manually modify sync settings. BUG= http://b/31156650 TEST=Manual test on a machine that sync settings is never modified. ========== to ========== arc: Add arc enable sync pref initial set up. Arc enable sync pref is false by default if user never modify sync settings. This patch enables arc sync service to start when user never manually modify sync settings. BUG= http://b/31156650 TEST=Manual test on a machine that sync settings is never modified. Committed: https://crrev.com/a9c8ea0daf53303475b32177b76938cf6b31c04d Cr-Commit-Position: refs/heads/master@{#415748} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a9c8ea0daf53303475b32177b76938cf6b31c04d Cr-Commit-Position: refs/heads/master@{#415748}
Message was sent while issue was closed.
Description was changed from ========== arc: Add arc enable sync pref initial set up. Arc enable sync pref is false by default if user never modify sync settings. This patch enables arc sync service to start when user never manually modify sync settings. BUG= http://b/31156650 TEST=Manual test on a machine that sync settings is never modified. Committed: https://crrev.com/a9c8ea0daf53303475b32177b76938cf6b31c04d Cr-Commit-Position: refs/heads/master@{#415748} ========== to ========== arc: Add arc enable sync pref initial set up. Arc enable sync pref is false by default if user never modify sync settings. This patch enables arc sync service to start when user never manually modify sync settings. BUG=642907, http://b/31156650 TEST=Manual test on a machine that sync settings is never modified. Committed: https://crrev.com/a9c8ea0daf53303475b32177b76938cf6b31c04d Cr-Commit-Position: refs/heads/master@{#415748} ========== |
