Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501783003:
MD Settings: Hook up import data dialog. (Closed)
|
Created:
4 years, 1 month ago by dpapad Modified:
3 years, 9 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, Dan Beam, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, tfarina Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Hook up import data dialog.
BUG=659259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f
Cr-Commit-Position: refs/heads/master@{#432753}
Patch Set 1 : Fix compiler #Patch Set 2 : Nits. #Patch Set 3 : Fix disabling logic. #
Total comments: 27
Patch Set 4 : Address comments, remove iron-change listener. #
Total comments: 8
Patch Set 5 : Fix typo #Patch Set 6 : Use PrefsBehavior to access prefs in JS. #Patch Set 7 : Fix typo. #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 38 (23 generated)
Description was changed from ========== MD Settings: Hook up import data dialog. BUG=659259 ========== to ========== MD Settings: Hook up import data dialog. BUG=659259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:10004) has been deleted
Patchset #1 (id:50001) has been deleted
Patchset #1 (id:70001) has been deleted
The CQ bit was checked by dpapad@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 checked by dpapad@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...
dpapad@chromium.org changed reviewers: + tommycli@chromium.org
See short video (40sec), showing all expected interactions with the dialog at https://vid.me/CAAx. Sending this out while I am still writing tests, since this is a dev blocker and we are trying to make the Thursday deadline. I am planning to add tests either in this CL or in the next one (depending on how long this review takes), I hope that makes sense. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:67: this.closeDialog_(); FYI this is what the old Options UI did on error, https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/import_d..., which should be good enough for now (addressing the dev blocker).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm - basically all nits below: https://codereview.chromium.org/2501783003/diff/130001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/app/settings_st... chrome/app/settings_strings.grdp:2192: <message name="IDS_SETTINGS_IMPORT_READY" desc="Message displayed after settings and bookmarks have been imported"> nit: even if the text says "ready", it seems more obvious to make the IDS string say IDS_SETTINGS_IMPORT_SUCCESS https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:27: .icon-container { nit: Can we remove the need for this by giving the iron-icon a margin: auto? https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:31: iron-icon { Although there is only one iron-icon here, it may serve as a useful comment to tag it with a #success-icon id https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:91: </div> It looks like this whole div block is basically unchanged, but just tabbed over right? https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:95: <paper-spinner any situation in which it's not-hidden but inactive? I'm wondering why there are separate tests for IN_PROGRESS and SUCCEEDED. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:109: <paper-button id="actionButton" class="action-button" nit: Maybe call this confirmButton or importButton instead of actionButton? Since action-button is also a class used by the done button. Same with the on-tap and the text methods https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:67: this.closeDialog_(); On 2016/11/16 00:34:38, dpapad wrote: > FYI this is what the old Options UI did on error, > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/import_d..., > which should be good enough for now (addressing the dev blocker). Okay seems fine. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:77: onCheckboxChange_: function() { onImportedDataTypeChange_? https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:78: var checkboxes = this.root.querySelectorAll( Can we make the selector operate something like this? #importedDataTypes settings-checkbox:not([hidden])[checked] I ask because it would be good to never accidentally select a non-datatype checkbox. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:80: this.noCategorySelected_ = checkboxes.length == 0; I was initially confused. Maybe we should rename this to noDataTypesSelectedForImport_ or something. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:109: onChange_: function() { Since there is now a onCheckboxChange_ method, maybe rename to onBrowserProfileSelectionChange_? https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:131: disallowAction_: function() { isImportButtonDisabled_? https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_import_data_handler.cc (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:129: if (prefs->GetBoolean(prefs::kImportSearchEngine)) ^ nice...
https://codereview.chromium.org/2501783003/diff/130001/chrome/app/settings_st... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/app/settings_st... chrome/app/settings_strings.grdp:2192: <message name="IDS_SETTINGS_IMPORT_READY" desc="Message displayed after settings and bookmarks have been imported"> On 2016/11/16 at 17:23:52, tommycli wrote: > nit: even if the text says "ready", it seems more obvious to make the IDS string say IDS_SETTINGS_IMPORT_SUCCESS Done. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:27: .icon-container { On 2016/11/16 at 17:23:52, tommycli wrote: > nit: Can we remove the need for this by giving the iron-icon a margin: auto? Done. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:31: iron-icon { On 2016/11/16 at 17:23:53, tommycli wrote: > Although there is only one iron-icon here, it may serve as a useful comment to tag it with a #success-icon id Done. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:91: </div> On 2016/11/16 at 17:23:52, tommycli wrote: > It looks like this whole div block is basically unchanged, but just tabbed over right? It was just tabbed over, but I ended up undoing this change, since I am no longer listening for iron-change event (that was the only reason I had put a surrounding div). https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:95: <paper-spinner On 2016/11/16 at 17:23:52, tommycli wrote: > any situation in which it's not-hidden but inactive? I'm wondering why there are separate tests for IN_PROGRESS and SUCCEEDED. It is subtle. When you change active to false, paper-spinner does a fade out animation which takes some noticeable time to execute. This resulted in the spinner being briefly visible after this dialog transitions to the SUCCEEDED case. The hidden attribute ensures that the fade-out animation of the spinner is not visible at all, when in SUCCEEDED state. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:109: <paper-button id="actionButton" class="action-button" On 2016/11/16 at 17:23:52, tommycli wrote: > nit: Maybe call this confirmButton or importButton instead of actionButton? Since action-button is also a class used by the done button. > > Same with the on-tap and the text methods Removed "id" completely, since it is not needed yet (perhaps I'll re-add it in next CL where I add tests). BTW, "Confirm" button can be easily confused with the button that simply displays "Done" at line 118. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:77: onCheckboxChange_: function() { On 2016/11/16 at 17:23:53, tommycli wrote: > onImportedDataTypeChange_? No longer listening for iron-chage event, so this method has been removed. Instead I am using an observer, see prefsChanged_. The reason is recorded at crbug.com/665979. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:78: var checkboxes = this.root.querySelectorAll( On 2016/11/16 at 17:23:53, tommycli wrote: > Can we make the selector operate something like this? > > #importedDataTypes settings-checkbox:not([hidden])[checked] > > I ask because it would be good to never accidentally select a non-datatype checkbox. There are no other checkboxes on this dialog, so don't see much value in this (extra ID, longer selector). https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:80: this.noCategorySelected_ = checkboxes.length == 0; On 2016/11/16 at 17:23:53, tommycli wrote: > I was initially confused. Maybe we should rename this to noDataTypesSelectedForImport_ or something. Done. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:109: onChange_: function() { On 2016/11/16 at 17:23:53, tommycli wrote: > Since there is now a onCheckboxChange_ method, maybe rename to onBrowserProfileSelectionChange_? onCheckboxChange_ has been removed. I renamed anyway, so done. https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:131: disallowAction_: function() { On 2016/11/16 at 17:23:53, tommycli wrote: > isImportButtonDisabled_? Renamed to |shouldDisableImport_|. My issue with the |isImportButtonDisabled_| naming is that it reverses cause/effect. |isImportButtonDisabled_| is used to determine whether the button should be disabled, but just by reading the name I am expecting to find something as follows in the contents of that method isImportButtonDisabled_: function() { return this.importButton.disabled; } https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_import_data_handler.cc (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:129: if (prefs->GetBoolean(prefs::kImportSearchEngine)) On 2016/11/16 at 17:23:53, tommycli wrote: > ^ nice... Thanks! https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:80: !(this.prefs['import_history'].value && this.selected_.history) && Is it OK to access prefs this way? Or should I be using PrefsBehavior methods instead?
https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2501783003/diff/130001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:95: <paper-spinner On 2016/11/16 21:33:02, dpapad wrote: > On 2016/11/16 at 17:23:52, tommycli wrote: > > any situation in which it's not-hidden but inactive? I'm wondering why there > are separate tests for IN_PROGRESS and SUCCEEDED. > > It is subtle. When you change active to false, paper-spinner does a fade out > animation which takes some noticeable time to execute. This resulted in the > spinner being briefly visible after this dialog transitions to the SUCCEEDED > case. The hidden attribute ensures that the fade-out animation of the spinner is > not visible at all, when in SUCCEEDED state. Acknowledged. https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.html (right): https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.html:55: on-change="onBrowserProfileSelectiChange_"> typo on on-change handler? https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:80: !(this.prefs['import_history'].value && this.selected_.history) && On 2016/11/16 21:33:02, dpapad wrote: > Is it OK to access prefs this way? Or should I be using PrefsBehavior methods > instead? I bet it's harmless to do it this way, but I'd still recommend using PrefsBehavior unless we're sure we really can't afford to add the behavior... Just for cleanliness https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:115: onBrowserProfileSelectiChange_: function() { Same typo here, so I guess that's why it works?
https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:80: !(this.prefs['import_history'].value && this.selected_.history) && On 2016/11/16 21:33:02, dpapad wrote: > Is it OK to access prefs this way? Or should I be using PrefsBehavior methods > instead? wont compile, .get('prefs.import_history.value') will though ;)
https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:80: !(this.prefs['import_history'].value && this.selected_.history) && On 2016/11/16 at 21:45:15, Dan Beam wrote: > On 2016/11/16 21:33:02, dpapad wrote: > > Is it OK to access prefs this way? Or should I be using PrefsBehavior methods > > instead? > > wont compile, .get('prefs.import_history.value') will though ;) Tried this locally and compiled. If I remove the quote notation it doesn't though. I'll try the behavior in a sec. https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:115: onBrowserProfileSelectiChange_: function() { On 2016/11/16 at 21:43:40, tommycli wrote: > Same typo here, so I guess that's why it works? Already fixed.
https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/import_data_dialog.js (right): https://codereview.chromium.org/2501783003/diff/150001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/import_data_dialog.js:80: !(this.prefs['import_history'].value && this.selected_.history) && > Tried this locally and compiled. If I remove the quote notation it doesn't though. I'll try the behavior in a sec. Using PrefsBehavior in latest patch.
The CQ bit was checked by dpapad@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 checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/2501783003/#ps210001 (title: "Fix typo.")
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by dpapad@chromium.org
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
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by dbeam@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:210001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Hook up import data dialog. BUG=659259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Hook up import data dialog. BUG=659259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f Cr-Commit-Position: refs/heads/master@{#432753} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/411c9f114fd32cde3b27382c78fdaac5d4e0365f Cr-Commit-Position: refs/heads/master@{#432753}
Message was sent while issue was closed.
gab@chromium.org changed reviewers: + gab@chromium.org
Message was sent while issue was closed.
The prefs hooks in this CL are undesired, see below, thanks. https://codereview.chromium.org/2501783003/diff/210001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_import_data_handler.cc (left): https://codereview.chromium.org/2501783003/diff/210001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:133: if (args->GetString(4, &string_value) && string_value == "true") { Looks like import_data_overlay.js still has code to send these args even though they're no longer honored... String($('import-browsers').selectedIndex), String($('import-history').checked), String($('import-favorites').checked), String($('import-passwords').checked), String($('import-search').checked), String($('import-autofill-form-data').checked)]); that confused me when trying to figure out this code, please clean up. https://codereview.chromium.org/2501783003/diff/210001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/settings_import_data_handler.cc (right): https://codereview.chromium.org/2501783003/diff/210001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/settings_import_data_handler.cc:121: if (prefs->GetBoolean(prefs::kImportAutofillFormData)) Hooking onto those prefs is wrong. Those are driving first run auto import. Policies set for those shouldn't drive the import UI and even less honor first run auto import's policies... If you want prefs, you'll need new ones. But I don't even think that's necessary... this UI is expected to be used only once per user and I don't think the checkboxes have to be sticky, they're not settings on their own, they're controls for a manual import and each import is an independent action. Filed http://crbug.com/695611, please address. |
