Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2179803002:
arc: Use location service consent. (Closed)
|
Created:
4 years, 5 months ago by khmel Modified:
4 years, 4 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Use location service consent.
This adds option to use location service into OptIn flow and fix
similar backup and restore consent.
BUG=631264
TEST=Manually on device together with Android side CL. On fresh device
go through OptIn flow and set use location service off. Start
Play Store and install Google Maps app. On start, Google Maps
asks permission to activate usage of Location service. Disable
and re-enable Arc and go throug OptIn flow but set this feature
on and repeat Google Maps installation. On start, Google maps
app does not ask permission to modify usage Location service.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/905787aa98feb38d969bcf03b0214a9d1a5a4c43
Cr-Commit-Position: refs/heads/master@{#407650}
Patch Set 1 #Patch Set 2 : rebae #
Total comments: 22
Patch Set 3 : comments addressed #
Total comments: 1
Patch Set 4 : discard unwanted change #
Total comments: 3
Messages
Total messages: 22 (7 generated)
Description was changed from
==========
arc: Use location service consent.
This adds option to use location service into OptIn flow and fix
similar backup and restore consent.
BUG=b/30308480
BUG=b/29331215
TEST=Manually on device together with Android side CL. On fresh device
go through OptIn flow and set use location service off. Start
Play Store and install Google Maps app. On start, Google Maps
asks permission to activate usage of Location service. Disable
and re-enable Arc and go throug OptIn flow but set this feature
on and repeat Google Maps installation. On start, Google maps
app does not ask permission to modify usage Location service.
==========
to
==========
arc: Use location service consent.
This adds option to use location service into OptIn flow and fix
similar backup and restore consent.
BUG=b/30308480
BUG=b/29331215
TEST=Manually on device together with Android side CL. On fresh device
go through OptIn flow and set use location service off. Start
Play Store and install Google Maps app. On start, Google Maps
asks permission to activate usage of Location service. Disable
and re-enable Arc and go throug OptIn flow but set this feature
on and repeat Google Maps installation. On start, Google maps
app does not ask permission to modify usage Location service.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
khmel@chromium.org changed reviewers: + xiyuan@chromium.org
Hi Xiyuan, PTAL https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:211: void ArcSettingsServiceImpl::SyncInitialSettings() const { We have backup&restore and location service options that have no control interface in Chrome and potentially can be changed on Android side. Our current policy for this case is to sync this settings on Android initial start only. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:212: SyncBackupEnabled(); Backup option was not synced at all. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/main.css (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:40: width: 640px; This new option reduces available size for ToS. We decided to extended horizontal content dimension to reduce lines in options. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:47: .checkbox-container { Please see more details in .html https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:52: .checkbox-option { More details in .html https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:146: padding: 10px 0 8px 0; Change required to allow more space for ToS https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/main.html (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.html:66: <div class="checkbox-container"> I found weird behavior when size of checkbox depends on number of lines in text description. This makes checkbox with declared 16px in css file appeared as 12px checkbox for 2 lines and 8px for 3 lines text description (https://drive.google.com/a/google.com/file/d/0B63tZNwdjs-qejg3dEMwek9TLVk/vie...). It was also not possible to apply padding because it was ignored. I found workaround by wrapping checkbox by div.
https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service.cc (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:211: void ArcSettingsServiceImpl::SyncInitialSettings() const { On 2016/07/25 15:36:05, khmel wrote: > We have backup&restore and location service options that have no control > interface in Chrome and potentially can be changed on Android side. Our current > policy for this case is to sync this settings on Android initial start only. Acknowledged. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:212: SyncBackupEnabled(); On 2016/07/25 15:36:05, khmel wrote: > Backup option was not synced at all. Acknowledged. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:496: sendNativeMessage('setLocationService', { nit: wrong indent https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/main.css (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:40: width: 640px; On 2016/07/25 15:36:05, khmel wrote: > This new option reduces available size for ToS. We decided to extended > horizontal content dimension to reduce lines in options. Acknowledged. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:47: .checkbox-container { On 2016/07/25 15:36:05, khmel wrote: > Please see more details in .html Acknowledged. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:52: .checkbox-option { On 2016/07/25 15:36:05, khmel wrote: > More details in .html Acknowledged. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/main.html (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.html:66: <div class="checkbox-container"> On 2016/07/25 15:36:05, khmel wrote: > I found weird behavior when size of checkbox depends on number of lines in text > description. This makes checkbox with declared 16px in css file appeared as 12px > checkbox for 2 lines and 8px for 3 lines text description > (https://drive.google.com/a/google.com/file/d/0B63tZNwdjs-qejg3dEMwek9TLVk/vie...). > It was also not possible to apply padding because it was ignored. I found > workaround by wrapping checkbox by div. Probably due to polymer's "layout horizontal" trying to something smart. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.html:70: <p class="checkbox-text" id="text-metrics"></p> While you are changing this file, can we change this to a label and associated with corresponding checkbox. Otherwise, user has to click on the checkbox to toggle it, kind of hard to aim. i.e. replace this <p> with <label class="checkbox-text" id="text-metrics" for="enable-metrics"></label> or wrap the <input> and <p> with <label> https://codereview.chromium.org/2179803002/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:30: const char kArcLocationServiceEnabled[] = "arc.location_service.enabled"; nit: alpha sort and document it ? https://codereview.chromium.org/2179803002/diff/20001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.h:25: extern const char kArcLocationServiceEnabled[]; nit: keep the section alpha-sorted ?
Thanks Xiyuan for comments. PTAL https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:496: sendNativeMessage('setLocationService', { On 2016/07/25 18:03:40, xiyuan wrote: > nit: wrong indent Done. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/main.html (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.html:66: <div class="checkbox-container"> On 2016/07/25 18:03:40, xiyuan wrote: > On 2016/07/25 15:36:05, khmel wrote: > > I found weird behavior when size of checkbox depends on number of lines in > text > > description. This makes checkbox with declared 16px in css file appeared as > 12px > > checkbox for 2 lines and 8px for 3 lines text description > > > (https://drive.google.com/a/google.com/file/d/0B63tZNwdjs-qejg3dEMwek9TLVk/vie...). > > It was also not possible to apply padding because it was ignored. I found > > workaround by wrapping checkbox by div. > > Probably due to polymer's "layout horizontal" trying to something smart. Yes, did some experiments and can confirm that problem because of flex layout. Once I set flex layout checkbox is affected. I googled and found similar problem here (webkit bug?): http://stackoverflow.com/questions/34220553/flexbox-resizes-a-checkbox-too-small I tried to use different workaround and it seems current approach with wrapping <input> is more convenient. https://codereview.chromium.org/2179803002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.html:70: <p class="checkbox-text" id="text-metrics"></p> On 2016/07/25 18:03:40, xiyuan wrote: > While you are changing this file, can we change this to a label and associated > with corresponding checkbox. Otherwise, user has to click on the checkbox to > toggle it, kind of hard to aim. > > i.e. replace this <p> with > <label class="checkbox-text" id="text-metrics" for="enable-metrics"></label> > > or wrap the <input> and <p> with <label> presubmit is not happy with with 'for'. So wrapping whole block. That is more user friendly indeed. Thanks for idea! https://codereview.chromium.org/2179803002/diff/20001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.cc:30: const char kArcLocationServiceEnabled[] = "arc.location_service.enabled"; On 2016/07/25 18:03:40, xiyuan wrote: > nit: alpha sort and document it ? Done, I remember I did it :) probably was lost. https://codereview.chromium.org/2179803002/diff/20001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2179803002/diff/20001/chrome/common/pref_name... chrome/common/pref_names.h:25: extern const char kArcLocationServiceEnabled[]; On 2016/07/25 18:03:40, xiyuan wrote: > nit: keep the section alpha-sorted ? Done. https://codereview.chromium.org/2179803002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2179803002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:152: var enableMetricsContainer = doc.getElementById('enable-metrics-container'); One we use wrapping we need to hide div, to avoid unwanted padding
lgtm
The CQ bit was checked by khmel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
arc: Use location service consent.
This adds option to use location service into OptIn flow and fix
similar backup and restore consent.
BUG=b/30308480
BUG=b/29331215
TEST=Manually on device together with Android side CL. On fresh device
go through OptIn flow and set use location service off. Start
Play Store and install Google Maps app. On start, Google Maps
asks permission to activate usage of Location service. Disable
and re-enable Arc and go throug OptIn flow but set this feature
on and repeat Google Maps installation. On start, Google maps
app does not ask permission to modify usage Location service.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
arc: Use location service consent.
This adds option to use location service into OptIn flow and fix
similar backup and restore consent.
BUG=631264
TEST=Manually on device together with Android side CL. On fresh device
go through OptIn flow and set use location service off. Start
Play Store and install Google Maps app. On start, Google Maps
asks permission to activate usage of Location service. Disable
and re-enable Arc and go throug OptIn flow but set this feature
on and repeat Google Maps installation. On start, Google maps
app does not ask permission to modify usage Location service.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply the patch.
Message was sent while issue was closed.
Description was changed from
==========
arc: Use location service consent.
This adds option to use location service into OptIn flow and fix
similar backup and restore consent.
BUG=631264
TEST=Manually on device together with Android side CL. On fresh device
go through OptIn flow and set use location service off. Start
Play Store and install Google Maps app. On start, Google Maps
asks permission to activate usage of Location service. Disable
and re-enable Arc and go throug OptIn flow but set this feature
on and repeat Google Maps installation. On start, Google maps
app does not ask permission to modify usage Location service.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
to
==========
arc: Use location service consent.
This adds option to use location service into OptIn flow and fix
similar backup and restore consent.
BUG=631264
TEST=Manually on device together with Android side CL. On fresh device
go through OptIn flow and set use location service off. Start
Play Store and install Google Maps app. On start, Google Maps
asks permission to activate usage of Location service. Disable
and re-enable Arc and go throug OptIn flow but set this feature
on and repeat Google Maps installation. On start, Google maps
app does not ask permission to modify usage Location service.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/905787aa98feb38d969bcf03b0214a9d1a5a4c43
Cr-Commit-Position: refs/heads/master@{#407650}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/905787aa98feb38d969bcf03b0214a9d1a5a4c43 Cr-Commit-Position: refs/heads/master@{#407650}
Message was sent while issue was closed.
poromov@chromium.org changed reviewers: + poromov@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service.cc (left): https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:221: SyncBackupEnabled(); Looks like because of removing these lines, backup pref won't be synced when changed because of policy change. Please, revert these lines.
Message was sent while issue was closed.
https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service.cc (left): https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:221: SyncBackupEnabled(); On 2016/08/02 15:20:41, Sergey Poromov wrote: > Looks like because of removing these lines, backup pref won't be synced when > changed because of policy change. > Please, revert these lines. This code does not make sense. kArcBackupRestoreEnabled is local pref and can be changed during Arc OptIn flow (and it is true by default). Even if it has been changed in OptIn Arc is not started at this moment and IPC message is not delivered. Please see SyncInitialSettings for backup fix.
Message was sent while issue was closed.
https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_settings_service.cc (left): https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_settings_service.cc:221: SyncBackupEnabled(); On 2016/08/02 16:22:44, khmel wrote: > On 2016/08/02 15:20:41, Sergey Poromov wrote: > > Looks like because of removing these lines, backup pref won't be synced when > > changed because of policy change. > > Please, revert these lines. > > This code does not make sense. kArcBackupRestoreEnabled is local pref and can be > changed during Arc OptIn flow (and it is true by default). Even if it has been > changed in OptIn Arc is not started at this moment and IPC message is not > delivered. > > Please see SyncInitialSettings for backup fix. What if the pref changes after OptIn, because of policy change? How should we transmit the change to Android side?
Message was sent while issue was closed.
Message was sent while issue was closed.
On 2016/08/02 17:43:49, Sergey Poromov wrote: > https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_settings_service.cc (left): > > https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_settings_service.cc:221: SyncBackupEnabled(); > On 2016/08/02 16:22:44, khmel wrote: > > On 2016/08/02 15:20:41, Sergey Poromov wrote: > > > Looks like because of removing these lines, backup pref won't be synced when > > > changed because of policy change. > > > Please, revert these lines. > > > > This code does not make sense. kArcBackupRestoreEnabled is local pref and can > be > > changed during Arc OptIn flow (and it is true by default). Even if it has been > > changed in OptIn Arc is not started at this moment and IPC message is not > > delivered. > > > > Please see SyncInitialSettings for backup fix. > > What if the pref changes after OptIn, because of policy change? How should we > transmit the change to Android side? Do we expect this? As I understand policy can be changed on Android side, not on Chrome side. On Chrome we don't expose any interface for changing this. Also consider situation when user change B&R on Android. We don't have bi-directional sync for this case. Based on this this option is applied only on Android first run.
Message was sent while issue was closed.
On 2016/08/02 17:47:34, khmel wrote: > On 2016/08/02 17:43:49, Sergey Poromov wrote: > > > https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... > > File chrome/browser/chromeos/arc/arc_settings_service.cc (left): > > > > > https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_settings_service.cc:221: SyncBackupEnabled(); > > On 2016/08/02 16:22:44, khmel wrote: > > > On 2016/08/02 15:20:41, Sergey Poromov wrote: > > > > Looks like because of removing these lines, backup pref won't be synced > when > > > > changed because of policy change. > > > > Please, revert these lines. > > > > > > This code does not make sense. kArcBackupRestoreEnabled is local pref and > can > > be > > > changed during Arc OptIn flow (and it is true by default). Even if it has > been > > > changed in OptIn Arc is not started at this moment and IPC message is not > > > delivered. > > > > > > Please see SyncInitialSettings for backup fix. > > > > What if the pref changes after OptIn, because of policy change? How should we > > transmit the change to Android side? > > Do we expect this? As I understand policy can be changed on Android side, not on > Chrome side. On Chrome we don't expose any interface for changing this. Also > consider situation when user change B&R on Android. We don't have bi-directional > sync for this case. Based on this this option is applied only on Android first > run. Policy is controlled on Chrome side: http://crrev.com/2127113002 We don't need to transfer and sync the setting from Android side, as any change that comes from Chrome side overrides it. There are two ways: 1) User set/unset checkbox on OptIn - it transfers to Android side immediately on start. 2) Policy changes to control B&R (on or off) - it should be transferred with "managed"=true, Android will disable ability to change it from Android setting. First case already works after this change. For second I assume that OnPrefChanged() will be called here as kArcBackupRestoreEnabled is observed by registrar.
Message was sent while issue was closed.
On 2016/08/02 17:59:21, Sergey Poromov wrote: > On 2016/08/02 17:47:34, khmel wrote: > > On 2016/08/02 17:43:49, Sergey Poromov wrote: > > > > > > https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/arc/arc_settings_service.cc (left): > > > > > > > > > https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_settings_service.cc:221: > SyncBackupEnabled(); > > > On 2016/08/02 16:22:44, khmel wrote: > > > > On 2016/08/02 15:20:41, Sergey Poromov wrote: > > > > > Looks like because of removing these lines, backup pref won't be synced > > when > > > > > changed because of policy change. > > > > > Please, revert these lines. > > > > > > > > This code does not make sense. kArcBackupRestoreEnabled is local pref and > > can > > > be > > > > changed during Arc OptIn flow (and it is true by default). Even if it has > > been > > > > changed in OptIn Arc is not started at this moment and IPC message is not > > > > delivered. > > > > > > > > Please see SyncInitialSettings for backup fix. > > > > > > What if the pref changes after OptIn, because of policy change? How should > we > > > transmit the change to Android side? > > > > Do we expect this? As I understand policy can be changed on Android side, not > on > > Chrome side. On Chrome we don't expose any interface for changing this. Also > > consider situation when user change B&R on Android. We don't have > bi-directional > > sync for this case. Based on this this option is applied only on Android first > > run. > > Policy is controlled on Chrome side: http://crrev.com/2127113002 > We don't need to transfer and sync the setting from Android side, as any change > that comes from Chrome side overrides it. There are two ways: > 1) User set/unset checkbox on OptIn - it transfers to Android side immediately > on start. > 2) Policy changes to control B&R (on or off) - it should be transferred with > "managed"=true, Android will disable ability to change it from Android setting. > > First case already works after this change. For second I assume that > OnPrefChanged() will be called here as kArcBackupRestoreEnabled is observed by > registrar. For second case I will create additional CL that handle changes (for B&R and Location services) in prefs (just to confirm that local prefs are ok to be controlled by policy. Or we need to declare it synced?). Could you please also check if we need to make Location Services also controlled by policy. Also solution for this is not just un-commenting 2 lines to handle pref changes. There is the case when pref can be changed when Arc is not available. In this case pref change notification will be discarded. Reapplying it each time on Arc boot is also not good because it may override Android settings (in case it is not manageable). I created bug for this: b/30595172. Please note that we also need to update OptIn UI in this case and this is requires legacy team approval and mocks.
Message was sent while issue was closed.
On 2016/08/02 18:13:08, khmel wrote: > On 2016/08/02 17:59:21, Sergey Poromov wrote: > > On 2016/08/02 17:47:34, khmel wrote: > > > On 2016/08/02 17:43:49, Sergey Poromov wrote: > > > > > > > > > > https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... > > > > File chrome/browser/chromeos/arc/arc_settings_service.cc (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2179803002/diff/60001/chrome/browser/chromeos... > > > > chrome/browser/chromeos/arc/arc_settings_service.cc:221: > > SyncBackupEnabled(); > > > > On 2016/08/02 16:22:44, khmel wrote: > > > > > On 2016/08/02 15:20:41, Sergey Poromov wrote: > > > > > > Looks like because of removing these lines, backup pref won't be > synced > > > when > > > > > > changed because of policy change. > > > > > > Please, revert these lines. > > > > > > > > > > This code does not make sense. kArcBackupRestoreEnabled is local pref > and > > > can > > > > be > > > > > changed during Arc OptIn flow (and it is true by default). Even if it > has > > > been > > > > > changed in OptIn Arc is not started at this moment and IPC message is > not > > > > > delivered. > > > > > > > > > > Please see SyncInitialSettings for backup fix. > > > > > > > > What if the pref changes after OptIn, because of policy change? How should > > we > > > > transmit the change to Android side? > > > > > > Do we expect this? As I understand policy can be changed on Android side, > not > > on > > > Chrome side. On Chrome we don't expose any interface for changing this. Also > > > consider situation when user change B&R on Android. We don't have > > bi-directional > > > sync for this case. Based on this this option is applied only on Android > first > > > run. > > > > Policy is controlled on Chrome side: http://crrev.com/2127113002 > > We don't need to transfer and sync the setting from Android side, as any > change > > that comes from Chrome side overrides it. There are two ways: > > 1) User set/unset checkbox on OptIn - it transfers to Android side immediately > > on start. > > 2) Policy changes to control B&R (on or off) - it should be transferred with > > "managed"=true, Android will disable ability to change it from Android > setting. > > > > First case already works after this change. For second I assume that > > OnPrefChanged() will be called here as kArcBackupRestoreEnabled is observed by > > registrar. > > For second case I will create additional CL that handle changes (for B&R and > Location services) in prefs (just to confirm that local prefs are ok to be > controlled by policy. Or we need to declare it synced?). Could you please also > check if we need to make Location Services also controlled by policy. Also > solution for this is not just un-commenting 2 lines to handle pref changes. > There is the case when pref can be changed when Arc is not available. In this > case pref change notification will be discarded. Reapplying it each time on Arc > boot is also not good because it may override Android settings (in case it is > not manageable). I created bug for this: b/30595172. Please note that we also > need to update OptIn UI in this case and this is requires legacy team approval > and mocks. Yes, local prefs are sufficient for policy control. They do not need to be synced. I e-mailed you and Philipp, who is working on bringing policy control to the opt-in screen. It is best if you sync up to avoid any duplicate work. |
