Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2104173002:
Settings People Revamp: Cache the selected Sync individual data types. (Closed)
|
Created:
4 years, 5 months ago by tommycli Modified:
4 years, 5 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSettings People Revamp: Cache the selected Sync individual data types.
Checking and unchecking "Sync All" should not forget which individual
data types to sync were previously selected.
This patch fixes that bug.
BUG=623541, 563721
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0f202ab267491fb469ffcb321560d9f0cfccf825
Cr-Commit-Position: refs/heads/master@{#402880}
Patch Set 1 #
Total comments: 6
Patch Set 2 : address feedback #
Total comments: 6
Patch Set 3 : address comments #Messages
Total messages: 18 (7 generated)
Description was changed from ========== Settings People Revamp: Cache the selected Sync individual data types. Checking and unchecking "Sync All" should not forget which individual data types to sync were previously selected. This patch fixes that bug. BUG=623541,563721 ========== to ========== Settings People Revamp: Cache the selected Sync individual data types. Checking and unchecking "Sync All" should not forget which individual data types to sync were previously selected. This patch fixes that bug. BUG=623541,563721 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
tommycli@chromium.org changed reviewers: + dpapad@chromium.org
dpapad: PTAL, thanks!
The CQ bit was checked by tommycli@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/2104173002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2104173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.js:71: * be able to restore the selections after checking and unchecking Sync All. @private https://codereview.chromium.org/2104173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.js:199: var SyncPrefsIndividualDataTypes = [ Can this array be moved outside this function as a local constant to the surrounding function scope? Seems unnecessary to redefine this array every time onSyncAllataTypesChanged_ executes. https://codereview.chromium.org/2104173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.js:218: this.cachedSyncPrefs_[dataType] = this.syncPrefs[dataType]; What is the key/value pair of syncPrefs look like? Is this shallow copy sufficient? If some of the values are complex objects themselves, then the cachedSyncPrefs and syncPrefs will hold references to the exact same object. If all values are primitives, then let's add a comment explaining that the shallow copy to store prefs is good enough.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
Thanks! https://codereview.chromium.org/2104173002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2104173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.js:71: * be able to restore the selections after checking and unchecking Sync All. On 2016/06/28 22:23:32, dpapad wrote: > @private Done. https://codereview.chromium.org/2104173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.js:199: var SyncPrefsIndividualDataTypes = [ On 2016/06/28 22:23:33, dpapad wrote: > Can this array be moved outside this function as a local constant to the > surrounding function scope? Seems unnecessary to redefine this array every time > onSyncAllataTypesChanged_ executes. Done. https://codereview.chromium.org/2104173002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/sync_page.js:218: this.cachedSyncPrefs_[dataType] = this.syncPrefs[dataType]; On 2016/06/28 22:23:33, dpapad wrote: > What is the key/value pair of syncPrefs look like? Is this shallow copy > sufficient? If some of the values are complex objects themselves, then the > cachedSyncPrefs and syncPrefs will hold references to the exact same object. If > all values are primitives, then let's add a comment explaining that the shallow > copy to store prefs is good enough. Done. I added a comment.
LGTM. https://codereview.chromium.org/2104173002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2104173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:19: * settings.SyncPrefs when the user checks 'Sync All'. @type {!Array<string>} https://codereview.chromium.org/2104173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:213: * @param {Event} event !Event https://codereview.chromium.org/2104173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:217: if (event.target.checked) { Guessing that this file is not compiled, otherwise event.target would need to be type casted first.
thank you Demetrios, for the review. https://codereview.chromium.org/2104173002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/sync_page.js (right): https://codereview.chromium.org/2104173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:19: * settings.SyncPrefs when the user checks 'Sync All'. On 2016/06/29 01:00:35, dpapad wrote: > @type {!Array<string>} Done. https://codereview.chromium.org/2104173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:213: * @param {Event} event On 2016/06/29 01:00:35, dpapad wrote: > !Event Done. https://codereview.chromium.org/2104173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/sync_page.js:217: if (event.target.checked) { On 2016/06/29 01:00:35, dpapad wrote: > Guessing that this file is not compiled, otherwise event.target would need to be > type casted first. Hi, it is compiled. I think anything on event.target is exempt, since I can change that property "checked" to any value and it will still compile.
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2104173002/#ps40001 (title: "address comments")
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 #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Settings People Revamp: Cache the selected Sync individual data types. Checking and unchecking "Sync All" should not forget which individual data types to sync were previously selected. This patch fixes that bug. BUG=623541,563721 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Settings People Revamp: Cache the selected Sync individual data types. Checking and unchecking "Sync All" should not forget which individual data types to sync were previously selected. This patch fixes that bug. BUG=623541,563721 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0f202ab267491fb469ffcb321560d9f0cfccf825 Cr-Commit-Position: refs/heads/master@{#402880} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0f202ab267491fb469ffcb321560d9f0cfccf825 Cr-Commit-Position: refs/heads/master@{#402880} |
