Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1418803004:
MD Settings: Migrating C++ handlers for profile reset page. (Closed)
|
Created:
5 years, 1 month ago by dpapad Modified:
5 years, 1 month ago Reviewers:
Dan Beam CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_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. |
DescriptionMD Settings: Migrating C++ handlers for profile reset page.
- Hooking up the "reset" button to actually trigger profile reset.
- Showing spinner until reset is done, then dismissing the dialog.
- Showing settings that will be send back to server, if user decides
to send feedback.
BUG=546840
Committed: https://crrev.com/30956f2dbc14e5c41d27636f1092699721c13869
Cr-Commit-Position: refs/heads/master@{#358966}
Patch Set 1 #Patch Set 2 : hide/show settings feedback #Patch Set 3 : remove unused #
Total comments: 10
Patch Set 4 : remove usage of max-width to fix dialog positioning. #
Total comments: 12
Patch Set 5 : address comments #Patch Set 6 : address more comments #Patch Set 7 : fix horizontal, vertical centering #
Total comments: 6
Patch Set 8 : add TODO #
Total comments: 16
Patch Set 9 : address comments #Patch Set 10 : Remove table and css. #Patch Set 11 : add missing tag #Patch Set 12 : rebase #Patch Set 13 : renaming handler to to fix GN build #Dependent Patchsets: Messages
Total messages: 33 (11 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:120001) has been deleted
Description was changed from ========== MD Settings: Migrating C++ handlers for profile reset page, WIP. BUG=546840 ========== to ========== MD Settings: Migrating C++ handlers for profile reset page. - Hooking up the "reset" button to actually trigger profile reset. - Showing spinner until reset is done, then dismissing the dialog. - Showing settings that will be send back to server, if user decides to send feedback. BUG=546840 ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
Screenshots at http://imgur.com/a/FByfx. There are resizing issues I am facing with paper-dialog, which I will attempt to address in follow-up CL. Specifically resizing the window does not re-center the modal dialog.
https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc (left): https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:56: web_ui()->CallJavascriptFunction("ResetProfileSettingsBanner.show"); so this seems to re-show the banner on refresh. do we not need to handle that case for new settings? https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:189: UpdateFeedbackUI(); why did you remove this? https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:68: NOTREACHED(); should this be NOTREACHED() + return?
sorry, skipped the HTML/CSS/JS before https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.html (right): https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:18: <paper-dialog modal id="resetDialog"> we should dom-if this out when a reset is not in progress, or move this to a separate dom-module, i.e. settings-reset-dialog or something, and instantiate dynamically https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:39: <template is="dom-repeat" items="[[feedbackInfo]]"> nit: can this be private? i.e. feedbackInfo_ https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.js (right): https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.js:70: this.$.settings.setAttribute('hidden', true); this.$.settings.hidden = !this.$.sendSettings.checked; https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:45: AddSettingsPageUIHandler(new ResetProfileSettingsHandler(web_ui)); nit: alphabetize?
https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc (left): https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:56: web_ui()->CallJavascriptFunction("ResetProfileSettingsBanner.show"); On 2015/11/06 23:52:23, Dan Beam wrote: > so this seems to re-show the banner on refresh. do we not need to handle that > case for new settings? I have removed anything related to the banner until Alan comes back with mocks for it. Note that "reset banner" != "reset dialog". The banner is this http://imgur.com/YaPRvOc. https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:189: UpdateFeedbackUI(); On 2015/11/06 23:52:23, Dan Beam wrote: > why did you remove this? I could not justify what it was doing. It was calling UpdateFeedbackUI twice (once as a callback from the previous line, and once because of the direct call here). I am fine with restoring this line if there is a good reason. https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:68: NOTREACHED(); On 2015/11/06 23:52:23, Dan Beam wrote: > should this be NOTREACHED() + return? Adding a return statement here will cause the profile to not be reset in a Release build, if that case happens (somehow), where as current behavior will just keep going with send_settings=false. I think the latter is a better behavior (and matches old settings). https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.html (right): https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:18: <paper-dialog modal id="resetDialog"> On 2015/11/07 00:07:06, Dan Beam wrote: > we should dom-if this out when a reset is not in progress, or move this to a > separate dom-module, i.e. settings-reset-dialog or something, and instantiate > dynamically I am fine with making this its own polymer element if you think it is better. But if this is for performance reasons, then we should think about this across the whole new settings page and probably not start from here. For example, the entire <settings-advanced-page> is in the DOM when in the "basic" view, with display set to "none", similarly with this dialog. If we can prove that this makes things better, we should probably start by dynamically adding the advanced page itself which is more heavy than this dialog. https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:39: <template is="dom-repeat" items="[[feedbackInfo]]"> On 2015/11/07 00:07:05, Dan Beam wrote: > nit: can this be private? i.e. feedbackInfo_ My understanding is that in order to use data binding, feedbackInfo needs to be added in the properties section of the polymer element. Does the concept of private apply to the properties section? https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.js (right): https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.js:70: this.$.settings.setAttribute('hidden', true); On 2015/11/07 00:07:06, Dan Beam wrote: > this.$.settings.hidden = !this.$.sendSettings.checked; Done. https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:45: AddSettingsPageUIHandler(new ResetProfileSettingsHandler(web_ui)); On 2015/11/07 00:07:06, Dan Beam wrote: > nit: alphabetize? Done.
https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc (left): https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:189: UpdateFeedbackUI(); On 2015/11/07 00:31:08, dpapad wrote: > On 2015/11/06 23:52:23, Dan Beam wrote: > > why did you remove this? > > I could not justify what it was doing. It was calling UpdateFeedbackUI twice > (once as a callback from the previous line, and once because of the direct call > here). I am fine with restoring this line if there is a good reason. should this be removed from options/ as well, then? https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:68: NOTREACHED(); On 2015/11/07 00:31:08, dpapad wrote: > On 2015/11/06 23:52:23, Dan Beam wrote: > > should this be NOTREACHED() + return? > > Adding a return statement here will cause the profile to not be reset in a > Release build, if that case happens (somehow), where as current behavior will > just keep going with send_settings=false. I think the latter is a better > behavior (and matches old settings). there are only 2 states AFAICT: the parameter is optional, in which case we shouldn't NOTREACHED(), or it is required and we should AND return. feel free to update the old options/ version as well if you want behaviors to match. https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.html (right): https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:18: <paper-dialog modal id="resetDialog"> On 2015/11/07 00:31:08, dpapad wrote: > On 2015/11/07 00:07:06, Dan Beam wrote: > > we should dom-if this out when a reset is not in progress, or move this to a > > separate dom-module, i.e. settings-reset-dialog or something, and instantiate > > dynamically > > I am fine with making this its own polymer element if you think it is better. > But if this is for performance reasons, then we should think about this across > the whole new settings page and probably not start from here. > > For example, the entire <settings-advanced-page> is in the DOM when in the > "basic" view, with display set to "none", similarly with this dialog. If we can > prove that this makes things better, we should probably start by dynamically > adding the advanced page itself which is more heavy than this dialog. then do that next ;) but we should be hiding as much UI as possible until it's needed. i think this is a perfect example of something that will be used like 0.01% of the time users visit the settings page, and therefore shouldn't be shown if possible this doesn't need to block this CL as it's not hitting real users yet, but I'd make these type of followups a priority https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:39: <template is="dom-repeat" items="[[feedbackInfo]]"> On 2015/11/07 00:31:08, dpapad wrote: > On 2015/11/07 00:07:05, Dan Beam wrote: > > nit: can this be private? i.e. feedbackInfo_ > > My understanding is that in order to use data binding, feedbackInfo needs to be > added in the properties section of the polymer element. Does the concept of > private apply to the properties section? yes
https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc (left): https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:189: UpdateFeedbackUI(); On 2015/11/07 00:52:00, Dan Beam wrote: > On 2015/11/07 00:31:08, dpapad wrote: > > On 2015/11/06 23:52:23, Dan Beam wrote: > > > why did you remove this? > > > > I could not justify what it was doing. It was calling UpdateFeedbackUI twice > > (once as a callback from the previous line, and once because of the direct > call > > here). I am fine with restoring this line if there is a good reason. > > should this be removed from options/ as well, then? Actually after further inspection, I decided to add it back here. settings_snapshot_ is already populated at line 185. The async call is simply adding some extra data into the settings_snapshot object, therefore the direct UpdateFeedbackUI call is meaningful. https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc (right): https://codereview.chromium.org/1418803004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/reset_profile_settings_handler.cc:68: NOTREACHED(); On 2015/11/07 00:52:00, Dan Beam wrote: > On 2015/11/07 00:31:08, dpapad wrote: > > On 2015/11/06 23:52:23, Dan Beam wrote: > > > should this be NOTREACHED() + return? > > > > Adding a return statement here will cause the profile to not be reset in a > > Release build, if that case happens (somehow), where as current behavior will > > just keep going with send_settings=false. I think the latter is a better > > behavior (and matches old settings). > > there are only 2 states AFAICT: the parameter is optional, in which case we > shouldn't NOTREACHED(), or it is required and we should AND return. feel free > to update the old options/ version as well if you want behaviors to match. Resolved per chat discussion. https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.html (right): https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:18: <paper-dialog modal id="resetDialog"> On 2015/11/07 00:52:00, Dan Beam wrote: > On 2015/11/07 00:31:08, dpapad wrote: > > On 2015/11/07 00:07:06, Dan Beam wrote: > > > we should dom-if this out when a reset is not in progress, or move this to a > > > separate dom-module, i.e. settings-reset-dialog or something, and > instantiate > > > dynamically > > > > I am fine with making this its own polymer element if you think it is better. > > But if this is for performance reasons, then we should think about this across > > the whole new settings page and probably not start from here. > > > > For example, the entire <settings-advanced-page> is in the DOM when in the > > "basic" view, with display set to "none", similarly with this dialog. If we > can > > prove that this makes things better, we should probably start by dynamically > > adding the advanced page itself which is more heavy than this dialog. > > then do that next ;) but we should be hiding as much UI as possible until it's > needed. i think this is a perfect example of something that will be used like > 0.01% of the time users visit the settings page, and therefore shouldn't be > shown if possible > > this doesn't need to block this CL as it's not hitting real users yet, but I'd > make these type of followups a priority Just to clarify, it is not shown unless user clicks on the "reset profile" row. You probably meant that it should not be placed in the DOM at all (which triggers all of the lifecycle methods of Polymer element, created, ready, attached unnecessarily). I am fine with converting this (and others to add elements to the DOM programatically) in follow ups, but also, we should mention it during our WebUI MD standups so that everyone is on board with it. https://codereview.chromium.org/1418803004/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:39: <template is="dom-repeat" items="[[feedbackInfo]]"> On 2015/11/07 00:52:00, Dan Beam wrote: > On 2015/11/07 00:31:08, dpapad wrote: > > On 2015/11/07 00:07:05, Dan Beam wrote: > > > nit: can this be private? i.e. feedbackInfo_ > > > > My understanding is that in order to use data binding, feedbackInfo needs to > be > > added in the properties section of the polymer element. Does the concept of > > private apply to the properties section? > > yes Done.
looks pretty good to me https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.css (right): https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:40: table-layout: fixed; whaaaa https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.html (right): https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:37: <table id="settings"> you're probably porting this over from the old stuff, but a) settings is not a good ID for this, imo ;) b) <table>? yuck https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.js (right): https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.js:70: onCheckboxChange_: function() { TODO(dpapad): find out what should actually show this information. dbeam@ votes a bubble.
https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.css (right): https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:40: table-layout: fixed; On 2015/11/07 03:19:26, Dan Beam wrote: > whaaaa This achieves displaying the table as in the old options, and it is taken from https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res.... https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.html (right): https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:37: <table id="settings"> On 2015/11/07 03:19:26, Dan Beam wrote: > you're probably porting this over from the old stuff, but > > a) settings is not a good ID for this, imo ;) > b) <table>? yuck a) Aren't IDs local to this shadow DOM? If that is true, then within this Shadow DOM the ID seems good enough. If not, then I agree that there could be collisions with other elements and the ID should be made more specific. b) Yes, I am porting this from old settings, while waiting for final mocks (no point of coming up with my own solution that is not using <table>). https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.js (right): https://codereview.chromium.org/1418803004/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.js:70: onCheckboxChange_: function() { On 2015/11/07 03:19:26, Dan Beam wrote: > TODO(dpapad): find out what should actually show this information. dbeam@ votes > a bubble. Done. Added TODO.
https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.css (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:38: font-size: 12px; i think we generally use scale-able units (i.e. %) https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:39: line-height: initial; what is this doing? https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:47: text-align: right; RTL https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:55: } i would do one of 2 things: a) don't port over the "settings" (i.e. things sent when you reset) until we have mocks b) don't attempt to make it look user-facing at all if you are dead set on porting that part https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.html (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.html:36: checked on-change="onCheckboxChange_"></paper-checkbox> this should be onSendSettingsChange_ https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.js (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.js:70: onCheckboxChange_: function() { arguable nit: alphabetize these private methods
https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.css (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:38: font-size: 12px; On 2015/11/09 at 23:05:18, Dan Beam wrote: > i think we generally use scale-able units (i.e. %) Done. https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:39: line-height: initial; On 2015/11/09 at 23:05:19, Dan Beam wrote: > what is this doing? http://imgur.com/a/pSIqv https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:47: text-align: right; On 2015/11/09 at 23:05:18, Dan Beam wrote: > RTL I prefer to address this in a separate CL. 1) The "send settings" UI is a port of the options one, until we get finalized mocks. 2) Old Options UI does not seem to handle RTL here, (or it is handled in someway I am not aware of), so I would need to investigate further. https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:55: } On 2015/11/09 at 23:05:18, Dan Beam wrote: > i would do one of 2 things: > a) don't port over the "settings" (i.e. things sent when you reset) until we have mocks > b) don't attempt to make it look user-facing at all if you are dead set on porting that part Thanks for the advice, but now both a and b are done. Undoing either of them it is more work, while at the same time having a decently looking "send settings" UI is not a bad state to be in. https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.js (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.js:70: onCheckboxChange_: function() { On 2015/11/09 at 23:05:19, Dan Beam wrote: > arguable nit: alphabetize these private methods Indeed this is arguable. I am failing to see the benefit of alphabetizing the order of methods. Let's have the discussion in person if you think it is worth it.
https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.css (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:47: text-align: right; On 2015/11/09 23:37:49, dpapad wrote: > On 2015/11/09 at 23:05:18, Dan Beam wrote: > > RTL > > I prefer to address this in a separate CL. > 1) The "send settings" UI is a port of the options one, until we get finalized > mocks. this CL is supposed to port functionality, mainly in C++ handlers, not the UI > 2) Old Options UI does not seem to handle RTL here, (or it is handled in > someway I am not aware of), so I would need to investigate further. that's a bug in the old settings UI that probably isn't worth fixing at this point. any code that goes into new settings probably would be. but again: I don't think this code should exist yet. https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:55: } On 2015/11/09 23:37:48, dpapad wrote: > On 2015/11/09 at 23:05:18, Dan Beam wrote: > > i would do one of 2 things: > > a) don't port over the "settings" (i.e. things sent when you reset) until we > have mocks > > b) don't attempt to make it look user-facing at all if you are dead set on > porting that part > > Thanks for the advice, but now both a and b are done. Undoing either of them it > is more work, while > at the same time having a decently looking "send settings" UI is not a bad state > to be in. yes it is because people will think designers actually wanted it that way
https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.css (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:55: } On 2015/11/09 23:42:03, Dan Beam wrote: > On 2015/11/09 23:37:48, dpapad wrote: > > On 2015/11/09 at 23:05:18, Dan Beam wrote: > > > i would do one of 2 things: > > > a) don't port over the "settings" (i.e. things sent when you reset) until we > > have mocks > > > b) don't attempt to make it look user-facing at all if you are dead set on > > porting that part > > > > Thanks for the advice, but now both a and b are done. Undoing either of them > it > > is more work, while > > at the same time having a decently looking "send settings" UI is not a bad > state > > to be in. > > yes it is because people will think designers actually wanted it that way people = our team
https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.css (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.css:55: } On 2015/11/10 01:41:30, Dan Beam wrote: > On 2015/11/09 23:42:03, Dan Beam wrote: > > On 2015/11/09 23:37:48, dpapad wrote: > > > On 2015/11/09 at 23:05:18, Dan Beam wrote: > > > > i would do one of 2 things: > > > > a) don't port over the "settings" (i.e. things sent when you reset) until > we > > > have mocks > > > > b) don't attempt to make it look user-facing at all if you are dead set on > > > porting that part > > > > > > Thanks for the advice, but now both a and b are done. Undoing either of them > > it > > > is more work, while > > > at the same time having a decently looking "send settings" UI is not a bad > > state > > > to be in. > > > > yes it is because people will think designers actually wanted it that way > > people = our team putting either a user-facing "OMG REMOVE THIS BEFORE WE LAUNCH" or stripping out the CSS for this unapproved version sound good to me
On 2015/11/10 at 01:57:45, dbeam wrote: > https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... > File chrome/browser/resources/settings/reset_page/reset_page.css (right): > > https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... > chrome/browser/resources/settings/reset_page/reset_page.css:55: } > On 2015/11/10 01:41:30, Dan Beam wrote: > > On 2015/11/09 23:42:03, Dan Beam wrote: > > > On 2015/11/09 23:37:48, dpapad wrote: > > > > On 2015/11/09 at 23:05:18, Dan Beam wrote: > > > > > i would do one of 2 things: > > > > > a) don't port over the "settings" (i.e. things sent when you reset) until > > we > > > > have mocks > > > > > b) don't attempt to make it look user-facing at all if you are dead set on > > > > porting that part > > > > > > > > Thanks for the advice, but now both a and b are done. Undoing either of them > > > it > > > > is more work, while > > > > at the same time having a decently looking "send settings" UI is not a bad > > > state > > > > to be in. > > > > > > yes it is because people will think designers actually wanted it that way > > > > people = our team > > putting either a user-facing "OMG REMOVE THIS BEFORE WE LAUNCH" or stripping out the CSS for this unapproved version sound good to me Removed CSS.
lgtm https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/reset_page/reset_page.js (right): https://codereview.chromium.org/1418803004/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/reset_page/reset_page.js:70: onCheckboxChange_: function() { On 2015/11/09 23:37:49, dpapad wrote: > On 2015/11/09 at 23:05:19, Dan Beam wrote: > > arguable nit: alphabetize these private methods > > Indeed this is arguable. I am failing to see the benefit of alphabetizing the > order of methods. Let's have the > discussion in person if you think it is worth it. I thought it was part of the style guide (JS or C++) but it's not. fwiw: if there's no other useful way to order them, alphabetically ordering makes them easier to scan as they're sorted. that's why I do this.
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418803004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418803004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Renamed ResetProfileSettingsHandler to ResetSettingsHandler, to work around crbug.com/554157. Still LG?
lgtm
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418803004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418803004/360001
Message was sent while issue was closed.
Committed patchset #13 (id:360001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/30956f2dbc14e5c41d27636f1092699721c13869 Cr-Commit-Position: refs/heads/master@{#358966} |
