Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127113002:
Policy to control ARC Backup&Restore. (Closed)
|
Created:
4 years, 5 months ago by Sergey Poromov Modified:
4 years, 5 months ago CC:
vkuzkokov, Yusuke Sato, elijahtaylor1, chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, asvitkine+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, tnagel+watch_chromium.org, davemoore+watch_chromium.org, Thiemo Nagel Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPolicy to control ARC Backup&Restore.
Design doc: https://docs.google.com/document/d/1kOi7GBYwsXNehL7S6LT78mDHZRrU1MGrmyf8h9LFWpY/edit
Buganizer bug: b/29813045
BUG=627101
Committed: https://crrev.com/1df20035a88b32610597f0bf915ddcbb674e9720
Cr-Commit-Position: refs/heads/master@{#407458}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Policy to control ARC Backup&Restore. #Patch Set 3 : Fix build. #
Total comments: 10
Patch Set 4 : Remove unnecessary arc_backup_restore_managed pref. #Patch Set 5 : Remove unnecessary arc_backup_restore_managed pref. #
Total comments: 8
Patch Set 6 : Remove default_for_enterprise_users #Patch Set 7 : Remove default_for_enterprise_users #
Total comments: 4
Patch Set 8 : lhchavez comments. #Patch Set 9 : lhchavez comments. #
Total comments: 12
Patch Set 10 : bartfab comments. #
Total comments: 5
Patch Set 11 : bartfab nits. #
Messages
Total messages: 62 (34 generated)
Description was changed from ========== Policy to control ARC Backup&Restore. BUG=b/29813045 ========== to ========== Policy to control ARC Backup&Restore. BUG=b/29813045 ==========
poromov@chromium.org changed reviewers: + bartfab@google.com, elijahtaylor@chromium.org, vkuzkokov@chromium.org, yusukes@chromium.org
https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.cc (right): https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.cc:17: base::Value::TYPE_BOOLEAN) {} nit: s/{}/= default;/ https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.cc:19: ArcBackupRestorePolicyHandler::~ArcBackupRestorePolicyHandler() {} same https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.cc:27: value->GetAsBoolean(&backup_restore_enabled); Can you initialize backup_restore_enabled (with false) and/or check the return value of GetAsBoolean()? Doing neither of them seems a bit scary to me. https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.cc:233: if (!pref_service->GetBoolean(prefs::kArcBackupRestoreManaged)) { nit: omit {} to be consistent with other code in this file.
https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.h (right): https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. one more nit: can you file a placeholder crbug.com bug and use the bug# in the BUG= line? Rietveld cannot update b/ bugs.
Description was changed from ========== Policy to control ARC Backup&Restore. BUG=b/29813045 ========== to ========== Policy to control ARC Backup&Restore. Buganizer bug: b/29813045 BUG=627101 ==========
poromov@chromium.org changed reviewers: + bartfab@chromium.org
https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.cc (right): https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.cc:17: base::Value::TYPE_BOOLEAN) {} On 2016/07/11 03:01:06, Yusuke Sato wrote: > nit: s/{}/= default;/ It's not possible for this constructor :( https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.cc:19: ArcBackupRestorePolicyHandler::~ArcBackupRestorePolicyHandler() {} On 2016/07/11 03:01:06, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.cc:27: value->GetAsBoolean(&backup_restore_enabled); On 2016/07/11 03:01:06, Yusuke Sato wrote: > Can you initialize backup_restore_enabled (with false) and/or check the return > value of GetAsBoolean()? Doing neither of them seems a bit scary to me. Done. https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.h (right): https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_backup_restore_policy_handler.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/07/11 03:03:40, Yusuke Sato wrote: > one more nit: can you file a placeholder http://crbug.com bug and use the bug# in the > BUG= line? > Rietveld cannot update b/ bugs. Done. https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2127113002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.cc:233: if (!pref_service->GetBoolean(prefs::kArcBackupRestoreManaged)) { On 2016/07/11 03:01:06, Yusuke Sato wrote: > nit: omit {} to be consistent with other code in this file. Done.
poromov@chromium.org changed reviewers: + atwilson@chromium.org, tnagel@chromium.org
Sergey, I'd suggest to minimize the redundancy between reviewers. Adding atwilson, bartfab, bartfab1 and myself could easily lead to duplication of effort. Just pick one. (And please don't send reviews to @google.com accounts.)
poromov@chromium.org changed reviewers: - atwilson@chromium.org, bartfab@google.com
https://codereview.chromium.org/2127113002/diff/40001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2127113002/diff/40001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:864: handlers->AddHandler(base::WrapUnique( Do you really need a handler here? Why not follow the example of ArcEnabled policy that gets along without a handler? https://codereview.chromium.org/2127113002/diff/40001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2127113002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.h:21: extern const char kArcBackupRestoreManaged[]; I'm not sure what's the purpose of that pref. I'd assume that there is a simpler way than creating a dedicated pref. https://codereview.chromium.org/2127113002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8758: 'caption': '''Enable Backup and Restore for ARC''', From our docs, it seems to me that Android Backup Service is the official name of this feature. If this is correct, please change the name of the policy and update the documentation. https://support.google.com/nexus/answer/2819582?hl=en https://codereview.chromium.org/2127113002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8761: '''When this policy is set to True, ARC app data will be uploaded s/True/true Nit: I'd suggest to use present tense as that is better style. https://codereview.chromium.org/2127113002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8762: to Android Backup servers and restored from them upon app installations. Maybe replacing "installations" by "re-installation" makes things clearer?
https://codereview.chromium.org/2127113002/diff/40001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2127113002/diff/40001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:864: handlers->AddHandler(base::WrapUnique( On 2016/07/12 15:02:40, Thiemo Nagel wrote: > Do you really need a handler here? Why not follow the example of ArcEnabled > policy that gets along without a handler? Done. https://codereview.chromium.org/2127113002/diff/40001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/2127113002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.h:21: extern const char kArcBackupRestoreManaged[]; On 2016/07/12 15:02:40, Thiemo Nagel wrote: > I'm not sure what's the purpose of that pref. I'd assume that there is a > simpler way than creating a dedicated pref. I didn't know that it's easy to know whether pref is managed by checking the pref itself. Removed the pref and large part of the change as well. https://codereview.chromium.org/2127113002/diff/40001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8758: 'caption': '''Enable Backup and Restore for ARC''', On 2016/07/12 15:02:40, Thiemo Nagel wrote: > From our docs, it seems to me that Android Backup Service is the official name > of this feature. If this is correct, please change the name of the policy and > update the documentation. > > https://support.google.com/nexus/answer/2819582?hl=en Done. Also changed histograms.xml https://codereview.chromium.org/2127113002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8761: '''When this policy is set to True, ARC app data will be uploaded On 2016/07/12 15:02:40, Thiemo Nagel wrote: > s/True/true > > Nit: I'd suggest to use present tense as that is better style. Done. https://codereview.chromium.org/2127113002/diff/40001/components/policy/resou... components/policy/resources/policy_templates.json:8762: to Android Backup servers and restored from them upon app installations. On 2016/07/12 15:02:40, Thiemo Nagel wrote: > Maybe replacing "installations" by "re-installation" makes things clearer? Done.
Description was changed from ========== Policy to control ARC Backup&Restore. Buganizer bug: b/29813045 BUG=627101 ========== to ========== Policy to control ARC Backup&Restore. Design doc: https://docs.google.com/document/d/1kOi7GBYwsXNehL7S6LT78mDHZRrU1MGrmyf8h9LFW... Buganizer bug: b/29813045 BUG=627101 ==========
tnagel@chromium.org changed reviewers: + pbond@chromium.org
Polina, could you please do a pass over this CL?
https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8756: 'id': 337, 'default_for_enterprise_users' set means that it's mandatory policy for enterprise users and always configured. Does this patch pass CloudPolicyTest browser test built for Chrome OS?
https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8767: Backup Service on and off in Android Settings app.''', Please document that there's an exception for enterprise-managed users.
https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8756: 'id': 337, On 2016/07/15 12:36:50, Polina Bondarenko wrote: > 'default_for_enterprise_users' set means that it's mandatory policy for > enterprise users and always configured. Does this patch pass CloudPolicyTest > browser test built for Chrome OS? The test passes successfully. I also ran all *Policy* tests. https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8767: Backup Service on and off in Android Settings app.''', On 2016/07/15 13:01:39, Thiemo Nagel (ooo) wrote: > Please document that there's an exception for enterprise-managed users. I'm not sure which one. That it's false by default for enterprise?
https://codereview.chromium.org/2127113002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2127113002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72466: + label="Disable Certificate Transparency enforcement for a list of URLs"/> "... these sites". A assume this is a rebase issue.
https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8756: 'id': 337, On 2016/07/18 10:10:56, Sergey Poromov wrote: > On 2016/07/15 12:36:50, Polina Bondarenko wrote: > > 'default_for_enterprise_users' set means that it's mandatory policy for > > enterprise users and always configured. Does this patch pass CloudPolicyTest > > browser test built for Chrome OS? > > The test passes successfully. I also ran all *Policy* tests. No, I was incorrectly building tests. Removed this value here. https://codereview.chromium.org/2127113002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2127113002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:72466: + label="Disable Certificate Transparency enforcement for a list of URLs"/> On 2016/07/18 10:26:01, vkuzkokov wrote: > "... these sites". A assume this is a rebase issue. Done.
Friendly ping on this change. We would like to merge it also into M-53 as Backup is enabled there, but not under policy control.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
chrome/browser/chromeos/arc/arc_support_host.cc lgtm (on-behalf-of elijahtaylor1, yusukes) https://codereview.chromium.org/2127113002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2127113002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:253: if (!pref_service->IsManagedPreference(prefs::kArcBackupRestoreEnabled)) nit: if (pref_service->IsManagedPreference(prefs::kArcBackupRestoreEnabled)) return; pref_service->SetBoolean(prefs::kArcBackupRestoreEnabled, is_enabled); https://codereview.chromium.org/2127113002/diff/120001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2127113002/diff/120001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3991: base::WrapUnique(new base::FundamentalValue(false)), nullptr); nit: Prefer base::MakeUnique<T>(...) over base::WrapUnique(new T(...)). The former uses one less allocation and is shorter.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2127113002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2127113002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:253: if (!pref_service->IsManagedPreference(prefs::kArcBackupRestoreEnabled)) On 2016/07/18 16:15:14, Luis Héctor Chávez wrote: > nit: > > if (pref_service->IsManagedPreference(prefs::kArcBackupRestoreEnabled)) > return; > > pref_service->SetBoolean(prefs::kArcBackupRestoreEnabled, is_enabled); Done. https://codereview.chromium.org/2127113002/diff/120001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2127113002/diff/120001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3991: base::WrapUnique(new base::FundamentalValue(false)), nullptr); On 2016/07/18 16:15:14, Luis Héctor Chávez wrote: > nit: Prefer base::MakeUnique<T>(...) over base::WrapUnique(new T(...)). The > former uses one less allocation and is shorter. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:253: if (pref_service->IsManagedPreference(prefs::kArcBackupRestoreEnabled)) It is safe to set the pref value even when it is managed. The policy value will trump the user setting anyway. https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3982: // ARC Backup&Restore is switched on by default. Nit: Here and below: s/&/ & / https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3985: prefs::kArcBackupRestoreEnabled)->IsManaged()); Nit: Here and below: Indent four spaces, not two. https://codereview.chromium.org/2127113002/diff/160001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:8757: 'caption': '''Enable Android Backup Service for ARC''', Nit: No need for "for ARC" - "Android" implies it already. https://codereview.chromium.org/2127113002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:8760: '''When this policy is set to true, ARC app data is uploaded Nit 1: Do not line-wrap in this file. Nit 2: s/ARC/Android/ https://codereview.chromium.org/2127113002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:8765: If this setting is not configured then users are able to turn Android What if the policy is set to false?
https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:253: if (pref_service->IsManagedPreference(prefs::kArcBackupRestoreEnabled)) On 2016/07/19 13:55:52, bartfab (slow) wrote: > It is safe to set the pref value even when it is managed. The policy value will > trump the user setting anyway. Done. https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3982: // ARC Backup&Restore is switched on by default. On 2016/07/19 13:55:52, bartfab (slow) wrote: > Nit: Here and below: s/&/ & / Done. https://codereview.chromium.org/2127113002/diff/160001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3985: prefs::kArcBackupRestoreEnabled)->IsManaged()); On 2016/07/19 13:55:52, bartfab (slow) wrote: > Nit: Here and below: Indent four spaces, not two. Done. https://codereview.chromium.org/2127113002/diff/160001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:8757: 'caption': '''Enable Android Backup Service for ARC''', On 2016/07/19 13:55:52, bartfab (slow) wrote: > Nit: No need for "for ARC" - "Android" implies it already. Done. https://codereview.chromium.org/2127113002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:8760: '''When this policy is set to true, ARC app data is uploaded On 2016/07/19 13:55:52, bartfab (slow) wrote: > Nit 1: Do not line-wrap in this file. > Nit 2: s/ARC/Android/ Done. https://codereview.chromium.org/2127113002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:8765: If this setting is not configured then users are able to turn Android On 2016/07/19 13:55:52, bartfab (slow) wrote: > What if the policy is set to false? Done.
poromov@chromium.org changed reviewers: + holte@chromium.org
poromov@chromium.org changed reviewers: - elijahtaylor@chromium.org, vkuzkokov@chromium.org, yusukes@chromium.org
poromov@chromium.org changed reviewers: + isherman@chromium.org - holte@chromium.org
poromov@chromium.org changed reviewers: - tnagel@chromium.org
histograms.xml lgtm
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits. https://codereview.chromium.org/2127113002/diff/180001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/180001/components/policy/reso... components/policy/resources/policy_templates.json:8760: '''When this policy is set to true, Android app data is uploaded to Android Backup servers and restored from them upon app re-installations. Nit: It may be worth saying "for compatible apps" so that nobody expects B&R to work for all apps. https://codereview.chromium.org/2127113002/diff/180001/components/policy/reso... components/policy/resources/policy_templates.json:8766: If this setting is not configured then users are able to turn Android Backup Service on and off in Android Settings app.''', Nit: s/in/in the/ https://codereview.chromium.org/2127113002/diff/180001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2127113002/diff/180001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:72825: + <int value="337" label="Enable Android Backup Service for ARC"/> Nit: Please keep this in sync with the policy caption in policy_templates.json.
tnagel@chromium.org changed reviewers: + tnagel@chromium.org
https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/80001/components/policy/resou... components/policy/resources/policy_templates.json:8767: Backup Service on and off in Android Settings app.''', On 2016/07/18 10:10:56, Sergey Poromov wrote: > On 2016/07/15 13:01:39, Thiemo Nagel (ooo) wrote: > > Please document that there's an exception for enterprise-managed users. > > I'm not sure which one. That it's false by default for enterprise? Yes, that's what I meant, but you seem to have removed that part in the mean time.
https://codereview.chromium.org/2127113002/diff/180001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2127113002/diff/180001/components/policy/reso... components/policy/resources/policy_templates.json:8760: '''When this policy is set to true, Android app data is uploaded to Android Backup servers and restored from them upon app re-installations. On 2016/07/22 15:07:51, bartfab (slow) wrote: > Nit: It may be worth saying "for compatible apps" so that nobody expects B&R to > work for all apps. Done. https://codereview.chromium.org/2127113002/diff/180001/components/policy/reso... components/policy/resources/policy_templates.json:8766: If this setting is not configured then users are able to turn Android Backup Service on and off in Android Settings app.''', On 2016/07/22 15:07:51, bartfab (slow) wrote: > Nit: s/in/in the/ Done.
The CQ bit was checked by poromov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by poromov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, bartfab@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2127113002/#ps200001 (title: "bartfab nits.")
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 ========== Policy to control ARC Backup&Restore. Design doc: https://docs.google.com/document/d/1kOi7GBYwsXNehL7S6LT78mDHZRrU1MGrmyf8h9LFW... Buganizer bug: b/29813045 BUG=627101 ========== to ========== Policy to control ARC Backup&Restore. Design doc: https://docs.google.com/document/d/1kOi7GBYwsXNehL7S6LT78mDHZRrU1MGrmyf8h9LFW... Buganizer bug: b/29813045 BUG=627101 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Policy to control ARC Backup&Restore. Design doc: https://docs.google.com/document/d/1kOi7GBYwsXNehL7S6LT78mDHZRrU1MGrmyf8h9LFW... Buganizer bug: b/29813045 BUG=627101 ========== to ========== Policy to control ARC Backup&Restore. Design doc: https://docs.google.com/document/d/1kOi7GBYwsXNehL7S6LT78mDHZRrU1MGrmyf8h9LFW... Buganizer bug: b/29813045 BUG=627101 Committed: https://crrev.com/1df20035a88b32610597f0bf915ddcbb674e9720 Cr-Commit-Position: refs/heads/master@{#407458} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/1df20035a88b32610597f0bf915ddcbb674e9720 Cr-Commit-Position: refs/heads/master@{#407458}
Message was sent while issue was closed.
On 2016/07/25 at 13:04:42, commit-bot wrote: > Patchset 11 (id:??) landed as https://crrev.com/1df20035a88b32610597f0bf915ddcbb674e9720 > Cr-Commit-Position: refs/heads/master@{#407458} I think this patch may be causing a build error: gen/policy/policy_constants.cc:957:12: error: unused variable 'kArcBackupRestoreEnabled' [-Werror,-Wunused-const-variable] const char kArcBackupRestoreEnabled[] = "ArcBackupRestoreEnabled"; ^ 1 error generated.
Message was sent while issue was closed.
On 2016/07/25 at 15:54:23, pdr. wrote: > On 2016/07/25 at 13:04:42, commit-bot wrote: > > Patchset 11 (id:??) landed as https://crrev.com/1df20035a88b32610597f0bf915ddcbb674e9720 > > Cr-Commit-Position: refs/heads/master@{#407458} > > I think this patch may be causing a build error: > gen/policy/policy_constants.cc:957:12: error: unused variable 'kArcBackupRestoreEnabled' [-Werror,-Wunused-const-variable] > const char kArcBackupRestoreEnabled[] = "ArcBackupRestoreEnabled"; > ^ > 1 error generated. If anyone else sees this bug, can you comment here? This should be chromeos-only and shouldn't affect vanilla chrome builds. I'm investigating my local build setup to see if it's something on my end. I confirmed that reverting this patch fixes the build.
Message was sent while issue was closed.
On 2016/07/25 at 16:28:21, pdr. wrote: > On 2016/07/25 at 15:54:23, pdr. wrote: > > On 2016/07/25 at 13:04:42, commit-bot wrote: > > > Patchset 11 (id:??) landed as https://crrev.com/1df20035a88b32610597f0bf915ddcbb674e9720 > > > Cr-Commit-Position: refs/heads/master@{#407458} > > > > I think this patch may be causing a build error: > > gen/policy/policy_constants.cc:957:12: error: unused variable 'kArcBackupRestoreEnabled' [-Werror,-Wunused-const-variable] > > const char kArcBackupRestoreEnabled[] = "ArcBackupRestoreEnabled"; > > ^ > > 1 error generated. > > If anyone else sees this bug, can you comment here? This should be chromeos-only and shouldn't affect vanilla chrome builds. > > I'm investigating my local build setup to see if it's something on my end. I confirmed that reverting this patch fixes the build. I'm sorry for the noise. I think this was some sort of stale state in my out directory. Blowing away out/Debug and rebuilding has fixed this issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
