Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104893003:
Adds a new checkbox for backup and restore in opt-in flow. (Closed)
|
Created:
4 years, 5 months ago by malaykeshav Modified:
4 years, 5 months ago CC:
arv+watch_chromium.org, chromium-reviews, davemoore+watch_chromium.org, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds a new checkbox for backup and restore in opt-in flow.
- Implements UI for opt-in flow to add checkbox for backup and
restore.
- Adds a new preference flag to store the user preference for backup
and restore. This will later on be sent to android via arc bridge
service.
- Adds strings to be displayed.
BUG=624868
COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/872d90082670d11e91a960c1783e1816873414a7
Cr-Commit-Position: refs/heads/master@{#403261}
Patch Set 1 : Adds a new checkbox for backup and restore in opt-in flow. #
Total comments: 13
Patch Set 2 : Adds a new checkbox for backup and restore in opt-in flow. #Patch Set 3 : Merge with ToT #
Total comments: 5
Patch Set 4 : Adds a new checkbox for backup and restore in opt-in flow. #Patch Set 5 : Preference path string changed as requested #
Total comments: 10
Patch Set 6 : Adds a new checkbox for backup and restore in opt-in flow. #
Total comments: 6
Patch Set 7 : Adds a new checkbox for backup and restore in opt-in flow. #Messages
Total messages: 56 (24 generated)
Description was changed from ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=b/28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference ========== to ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=b/28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=b/28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=b:28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by malaykeshav@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...
malaykeshav@chromium.org changed reviewers: + khmel@chromium.org, xiyuan@chromium.org
Description was changed from ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=b:28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. https://b.corp.google.com/u/0/issues/28704780 BUG=b:28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:107: static void RegisterPrefs(PrefRegistrySimple* registry); Should it be sync able? https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:93: setBackupRestoreMode(data.textBackupRestore, true); Remove true? Ideally we should register listener on this pref and update checkbox. Please see UMA impl for example https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:119: var url = 'https://support.google.com/chromebook?p=playapps'; Do we want the same help for both? https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:154: * @param {boolean=} isChecked Defines if backup and restore option is active Not sure if '=' is needed. https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:161: enableBackupRestoreEl.hidden = false; You always set it visible. I think this is not required. https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/main.css (right): https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:26: .checkbox-backup-restore, Can we use one class for this? for example: .checkbox-option. The same below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:107: static void RegisterPrefs(PrefRegistrySimple* registry); On 2016/06/28 at 23:09:54, khmel wrote: > Should it be sync able? No, this is for the non syncable prefs. https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:93: setBackupRestoreMode(data.textBackupRestore, true); On 2016/06/28 at 23:09:54, khmel wrote: > Remove true? Ideally we should register listener on this pref and update checkbox. Please see UMA impl for example Done. Not sure what you mean by UMA impl. I have used the metrics checkbox as a reference. https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/arc_su... https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:119: var url = 'https://support.google.com/chromebook?p=playapps'; On 2016/06/28 at 23:09:54, khmel wrote: > Do we want the same help for both? Yes. https://b.corp.google.com/u/0/issues/28704780#comment14 https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:154: * @param {boolean=} isChecked Defines if backup and restore option is active On 2016/06/28 at 23:09:54, khmel wrote: > Not sure if '=' is needed. No longer required. https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:161: enableBackupRestoreEl.hidden = false; On 2016/06/28 at 23:09:54, khmel wrote: > You always set it visible. I think this is not required. Done https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/main.css (right): https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/main.css:26: .checkbox-backup-restore, On 2016/06/28 at 23:09:54, khmel wrote: > Can we use one class for this? for example: .checkbox-option. The same below. Done
The CQ bit was checked by malaykeshav@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by malaykeshav@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.
malaykeshav@chromium.org changed reviewers: + yusukes@chromium.org
https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:93: setBackupRestoreMode(data.textBackupRestore, true); On 2016/06/29 01:29:06, malaykeshav wrote: > On 2016/06/28 at 23:09:54, khmel wrote: > > Remove true? Ideally we should register listener on this pref and update > checkbox. Please see UMA impl for example > > > Done. > Not sure what you mean by UMA impl. I have used the metrics checkbox as a > reference. > https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/arc_su... khmel@ means is to observe the pref change: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_support_... and update the UI accordingly. https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:134: registry->RegisterBooleanPref(prefs::kArcBackupRestoreEnabled, false); Think this should be part of user prefs and we should do it with ArcAuthService::RegisterProfilePrefs above. https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:27: class ProfileOAuth2TokenService; not used? https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:174: enableBackupRestoreEl.checked = true; We should take the on/off value passed in from ArcAuthService based on the pref value.
On 2016/06/29 at 21:36:38, xiyuan wrote: > https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/chromeos/arc_support/background.js (right): > > https://codereview.chromium.org/2104893003/diff/40001/chrome/browser/resource... > chrome/browser/resources/chromeos/arc_support/background.js:93: setBackupRestoreMode(data.textBackupRestore, true); > On 2016/06/29 01:29:06, malaykeshav wrote: > > On 2016/06/28 at 23:09:54, khmel wrote: > > > Remove true? Ideally we should register listener on this pref and update > > checkbox. Please see UMA impl for example > > > > > > Done. > > Not sure what you mean by UMA impl. I have used the metrics checkbox as a > > reference. > > https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/arc_su... > > khmel@ means is to observe the pref change: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/arc_support_... > > and update the UI accordingly. Done. As discussed offline, there is a two way binding for the preference and checkbox. > https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_auth_service.cc:134: registry->RegisterBooleanPref(prefs::kArcBackupRestoreEnabled, false); > Think this should be part of user prefs and we should do it with ArcAuthService::RegisterProfilePrefs above. Done > https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_auth_service.h (right): > > https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_auth_service.h:27: class ProfileOAuth2TokenService; > not used? Done > https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/arc_support/background.js (right): > > https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/arc_support/background.js:174: enableBackupRestoreEl.checked = true; > We should take the on/off value passed in from ArcAuthService based on the pref value. Done
The CQ bit was checked by malaykeshav@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...
LGTM but please sync with khmel@ before submitting.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
elijahtaylor@chromium.org changed reviewers: + elijahtaylor@chromium.org
top level: please remove the b.corp link from the CL description https://codereview.chromium.org/2104893003/diff/120001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6701: <message name="IDS_ARC_OPT_IN_DIALOG_BACKUP_RESTORE" desc="Message in the opt-in dialog for Android apps in case backup and restore is disabled on the device. User has an option to activate them using opt-in dialog"> the bit about "in case backup and restore is disabled on device" is a copy-paste mistake I think from above. It could simply be something like: Message in the opt-in dialog for users to enable Backup and Restore for Android apps" https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:150: localized_strings->SetBoolean( We want the default to be checked, and set the pref based on the result of the checkbox when the user agrees. We don't care about the previous state of this pref here (or its default state). This could be initialized in javascript and you wouldn't need kBackupRestoreEnabled and communication of this to the support extension.
Description was changed from ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. https://b.corp.google.com/u/0/issues/28704780 BUG=b:28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=b:28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:27: class ProfileOAuth2TokenService; On 2016/06/29 at 21:36:38, xiyuan wrote: > not used? Missed it while merging with ToT. Done https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2104893003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/arc_support/background.js:174: enableBackupRestoreEl.checked = true; On 2016/06/29 at 21:36:38, xiyuan wrote: > We should take the on/off value passed in from ArcAuthService based on the pref value. The default value if always checked. https://codereview.chromium.org/2104893003/diff/120001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:6701: <message name="IDS_ARC_OPT_IN_DIALOG_BACKUP_RESTORE" desc="Message in the opt-in dialog for Android apps in case backup and restore is disabled on the device. User has an option to activate them using opt-in dialog"> On 2016/06/30 at 01:59:06, elijahtaylor1 wrote: > the bit about "in case backup and restore is disabled on device" is a copy-paste mistake I think from above. It could simply be something like: Message in the opt-in dialog for users to enable Backup and Restore for Android apps" Done https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:150: localized_strings->SetBoolean( On 2016/06/30 at 01:59:06, elijahtaylor1 wrote: > We want the default to be checked, and set the pref based on the result of the checkbox when the user agrees. We don't care about the previous state of this pref here (or its default state). > > This could be initialized in javascript and you wouldn't need kBackupRestoreEnabled and communication of this to the support extension. What about the case where the opt-in fails. Do we not save the state when the user retries?
lgtm with fixes https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:150: localized_strings->SetBoolean( On 2016/06/30 02:46:19, malaykeshav wrote: > On 2016/06/30 at 01:59:06, elijahtaylor1 wrote: > > We want the default to be checked, and set the pref based on the result of the > checkbox when the user agrees. We don't care about the previous state of this > pref here (or its default state). > > > > This could be initialized in javascript and you wouldn't need > kBackupRestoreEnabled and communication of this to the support extension. > > What about the case where the opt-in fails. Do we not save the state when the > user retries? That's a good point, and I do think it's the right thing to preserve that selection. But we still need to change this so the default is true instead of false, which I think is the default for an unset boolean pref. https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/arc_support/background.js:156: function setBackupRestoreMode(text, isChecked) { nit: isChecked seems to indicate the current state of the checkbox. maybe just "checked" or "makeChecked"? https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/arc_support/background.js:159: enableBackupRestoreEl.checked = isChecked; this could be a one-liner: doc.getElementById('enable-backup-restore').checked = isChecked;
c/b/c/arc/ lgtm with comments: https://codereview.chromium.org/2104893003/diff/140001/chrome/app/chromeos_st... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2104893003/diff/140001/chrome/app/chromeos_st... chrome/app/chromeos_strings.grdp:1: <?xml version="1.0" encoding="utf-8"?> Please file a placeholder bug at crbug.com and replace BUG= line with the bug. Rietveld cannot update b/ bugs. Example: https://bugs.chromium.org/p/chromium/issues/detail?id=623205 https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:105: std::unique_ptr<base::DictionaryValue> localized_strings( nit: this is no longer "strings". what about renaming this to load_time_data or something like that?
Description was changed from ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=b:28704780 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=624868 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
On 2016/06/30 at 18:06:47, yusukes wrote: > c/b/c/arc/ lgtm with comments: > > https://codereview.chromium.org/2104893003/diff/140001/chrome/app/chromeos_st... > File chrome/app/chromeos_strings.grdp (right): > > https://codereview.chromium.org/2104893003/diff/140001/chrome/app/chromeos_st... > chrome/app/chromeos_strings.grdp:1: <?xml version="1.0" encoding="utf-8"?> > Please file a placeholder bug at crbug.com and replace BUG= line with the bug. Rietveld cannot update b/ bugs. > > Example: https://bugs.chromium.org/p/chromium/issues/detail?id=623205 Done > https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_support_host.cc (right): > > https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_support_host.cc:105: std::unique_ptr<base::DictionaryValue> localized_strings( > nit: this is no longer "strings". what about renaming this to load_time_data or something like that? Done
lgtm
https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:150: localized_strings->SetBoolean( On 2016/06/30 at 04:52:34, elijahtaylor1 wrote: > On 2016/06/30 02:46:19, malaykeshav wrote: > > On 2016/06/30 at 01:59:06, elijahtaylor1 wrote: > > > We want the default to be checked, and set the pref based on the result of the > > checkbox when the user agrees. We don't care about the previous state of this > > pref here (or its default state). > > > > > > This could be initialized in javascript and you wouldn't need > > kBackupRestoreEnabled and communication of this to the support extension. > > > > What about the case where the opt-in fails. Do we not save the state when the > > user retries? > > That's a good point, and I do think it's the right thing to preserve that selection. But we still need to change this so the default is true instead of false, which I think is the default for an unset boolean pref. This is already set when defining the boolean preference value here: https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/arc_support/background.js:156: function setBackupRestoreMode(text, isChecked) { On 2016/06/30 at 04:52:34, elijahtaylor1 wrote: > nit: isChecked seems to indicate the current state of the checkbox. maybe just "checked" or "makeChecked"? Renaming to defaultCheckValue https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/arc_support/background.js:159: enableBackupRestoreEl.checked = isChecked; On 2016/06/30 at 04:52:34, elijahtaylor1 wrote: > this could be a one-liner: > > doc.getElementById('enable-backup-restore').checked = isChecked; Done
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, elijahtaylor@chromium.org, yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2104893003/#ps160001 (title: "Adds a new checkbox for backup and restore in opt-in flow.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:150: localized_strings->SetBoolean( On 2016/06/30 18:25:46, malaykeshav wrote: > On 2016/06/30 at 04:52:34, elijahtaylor1 wrote: > > On 2016/06/30 02:46:19, malaykeshav wrote: > > > On 2016/06/30 at 01:59:06, elijahtaylor1 wrote: > > > > We want the default to be checked, and set the pref based on the result of > the > > > checkbox when the user agrees. We don't care about the previous state of > this > > > pref here (or its default state). > > > > > > > > This could be initialized in javascript and you wouldn't need > > > kBackupRestoreEnabled and communication of this to the support extension. > > > > > > What about the case where the opt-in fails. Do we not save the state when > the > > > user retries? > > > > That's a good point, and I do think it's the right thing to preserve that > selection. But we still need to change this so the default is true instead of > false, which I think is the default for an unset boolean pref. > > This is already set when defining the boolean preference value here: > https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... BTW, if OptIn fails, 'Get Started' page is not shown, retry starts from LSO page
https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:150: localized_strings->SetBoolean( On 2016/06/30 18:30:11, khmel wrote: > On 2016/06/30 18:25:46, malaykeshav wrote: > > On 2016/06/30 at 04:52:34, elijahtaylor1 wrote: > > > On 2016/06/30 02:46:19, malaykeshav wrote: > > > > On 2016/06/30 at 01:59:06, elijahtaylor1 wrote: > > > > > We want the default to be checked, and set the pref based on the result > of > > the > > > > checkbox when the user agrees. We don't care about the previous state of > > this > > > > pref here (or its default state). > > > > > > > > > > This could be initialized in javascript and you wouldn't need > > > > kBackupRestoreEnabled and communication of this to the support extension. > > > > > > > > What about the case where the opt-in fails. Do we not save the state when > > the > > > > user retries? > > > > > > That's a good point, and I do think it's the right thing to preserve that > > selection. But we still need to change this so the default is true instead of > > false, which I think is the default for an unset boolean pref. > > > > This is already set when defining the boolean preference value here: > > > https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... > > BTW, if OptIn fails, 'Get Started' page is not shown, retry starts from LSO page What about user disables ARC then re-enables it in settings? Think we should always get the pref value to init the opt-in page to be correct.
https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:150: localized_strings->SetBoolean( On 2016/06/30 at 18:34:22, xiyuan wrote: > On 2016/06/30 18:30:11, khmel wrote: > > On 2016/06/30 18:25:46, malaykeshav wrote: > > > On 2016/06/30 at 04:52:34, elijahtaylor1 wrote: > > > > On 2016/06/30 02:46:19, malaykeshav wrote: > > > > > On 2016/06/30 at 01:59:06, elijahtaylor1 wrote: > > > > > > We want the default to be checked, and set the pref based on the result > > of > > > the > > > > > checkbox when the user agrees. We don't care about the previous state of > > > this > > > > > pref here (or its default state). > > > > > > > > > > > > This could be initialized in javascript and you wouldn't need > > > > > kBackupRestoreEnabled and communication of this to the support extension. > > > > > > > > > > What about the case where the opt-in fails. Do we not save the state when > > > the > > > > > user retries? > > > > > > > > That's a good point, and I do think it's the right thing to preserve that > > > selection. But we still need to change this so the default is true instead of > > > false, which I think is the default for an unset boolean pref. > > > > > > This is already set when defining the boolean preference value here: > > > > > https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... > > > > BTW, if OptIn fails, 'Get Started' page is not shown, retry starts from LSO page > > What about user disables ARC then re-enables it in settings? Think we should always get the pref value to init the opt-in page to be correct. The user's choice persists across the disable-renable
https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2104893003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:150: localized_strings->SetBoolean( On 2016/06/30 18:50:59, malaykeshav wrote: > On 2016/06/30 at 18:34:22, xiyuan wrote: > > On 2016/06/30 18:30:11, khmel wrote: > > > On 2016/06/30 18:25:46, malaykeshav wrote: > > > > On 2016/06/30 at 04:52:34, elijahtaylor1 wrote: > > > > > On 2016/06/30 02:46:19, malaykeshav wrote: > > > > > > On 2016/06/30 at 01:59:06, elijahtaylor1 wrote: > > > > > > > We want the default to be checked, and set the pref based on the > result > > > of > > > > the > > > > > > checkbox when the user agrees. We don't care about the previous state > of > > > > this > > > > > > pref here (or its default state). > > > > > > > > > > > > > > This could be initialized in javascript and you wouldn't need > > > > > > kBackupRestoreEnabled and communication of this to the support > extension. > > > > > > > > > > > > What about the case where the opt-in fails. Do we not save the state > when > > > > the > > > > > > user retries? > > > > > > > > > > That's a good point, and I do think it's the right thing to preserve > that > > > > selection. But we still need to change this so the default is true > instead of > > > > false, which I think is the default for an unset boolean pref. > > > > > > > > This is already set when defining the boolean preference value here: > > > > > > > > https://codereview.chromium.org/2104893003/diff/140001/chrome/browser/chromeo... > > > > > > BTW, if OptIn fails, 'Get Started' page is not shown, retry starts from LSO > page > > > > What about user disables ARC then re-enables it in settings? Think we should > always get the pref value to init the opt-in page to be correct. > > The user's choice persists across the disable-renable I also still think that pref_change_registrar_.Add(prefs::kArcBackupRestoreEnabled) might make sense
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_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 malaykeshav@chromium.org
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.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=624868 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Adds a new checkbox for backup and restore in opt-in flow. - Implements UI for opt-in flow to add checkbox for backup and restore. - Adds a new preference flag to store the user preference for backup and restore. This will later on be sent to android via arc bridge service. - Adds strings to be displayed. BUG=624868 COMPONENT=CrOS Strings, Arc, Arc Opt-In UI, Preference CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/872d90082670d11e91a960c1783e1816873414a7 Cr-Commit-Position: refs/heads/master@{#403261} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/872d90082670d11e91a960c1783e1816873414a7 Cr-Commit-Position: refs/heads/master@{#403261} |
