Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2236213002:
Add quick unlock Settings in options page (Closed)
|
Created:
4 years, 4 months ago by xiaoyinh(OOO Sep 11-29) Modified:
4 years, 4 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, extensions-reviews_chromium.org, michaelpg+watch-options_chromium.org, vabr+watchlistpasswordmanager_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, jlklein+watch-closure_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, dbeam+watch-closure_chromium.org, stevenjb+watch-md-settings_chromium.org, gcasto+watchlist_chromium.org, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd quick unlock Settings in options page
BUG=629662
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/36a1cc2dac63adf7cf223a158ce9826c3f2fa98b
Cr-Commit-Position: refs/heads/master@{#413268}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 35
Patch Set 3 : incorporate comments from jdufault@ #
Total comments: 2
Patch Set 4 : try to fix browser tests #
Total comments: 16
Patch Set 5 : incorporate comments from jdufault@ #
Total comments: 27
Patch Set 6 : pre-process i18n in options and trim route_stub.js #
Total comments: 12
Patch Set 7 : incorporate comments and fix browsertest failure #Patch Set 8 : fix browser test2 #
Total comments: 4
Patch Set 9 : nits #Messages
Total messages: 76 (37 generated)
Description was changed from ========== Add quick unlock Settings in options page BUG= ========== to ========== Add quick unlock Settings in options page BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by xiaoyinh@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by xiaoyinh@google.com 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...
Description was changed from ========== Add quick unlock Settings in options page BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add quick unlock Settings in options page BUG=629662 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Devlin@, please take a look at chrome/common/extensions/api/ xiyuan@,stevenjb@, jdufault@ please take a look at chrome/browser/resources/ and chrome/browser/ui/webui Sorry about the large CL. This includes: 1. small changes to make i18n work at options page 2. Add a overlay in options 3. import polymer elements to options Thanks!
Overall looks good, just some small comments. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:175: <inner-text>[[i18n('pinKeyboard1')]]</inner-text> Can you add a TODO to revert this change once we don't need to support options? https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:440: settings.navigateTo(settings.Route.LOCK_SCREEN); nit: indentation https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/compiled_resources.gyp (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/compiled_resources.gyp:46: '../settings/compiled_resources2.gyp:route', nit: indentation https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/sync_section.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/sync_section.html:47: <a is="action-link" id="manage-screenlock" i18n-content="manageScreenlock" hidden> nit: newline instead of wrapping https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/compiled_resources2.gyp (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/compiled_resources2.gyp:73: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:i18n_behavior', nit: indentation https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:28: [[i18n('lockScreenPasswordOnly')]] Please add a TODO to revert these changes once we don't need to support options. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:42: <div class="settings-box single-column"> Should the hidden$ be on this div instead? https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:44: label="$i18n{enableScreenlock}" hidden$="[[hideEnableSettings_()]]"> nit: newline instead of wrapping https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:44: label="$i18n{enableScreenlock}" hidden$="[[hideEnableSettings_()]]"> You can use the property directly instead of creating a helper function. hidden$="[[hideEnableScreenlock]]" https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:43: */ Add a TODO to remove this property after options is gone. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:106: document.dispatchEvent(new Event('close-overlay')); Can you make it work by firing an event on the element? this.fire('close-overlay') If it's going to be significant effort don't bother. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:106: document.dispatchEvent(new Event('close-overlay')); Add a TODO to remove this event after options is gone. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.html:1: <link rel="import" href="chrome://resources/html/i18n_behavior.html"> Please order this alphabetically. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.html:22: <div class="title">[[i18n('passwordPromptTitle')]]</div> Please add a TODO to revert these changes when we can drop options support. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:50: behaviors: [ Add TODO to remove https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:55: [[i18n('cancel')]] Add TODO to revert https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:219: response_bytes = Ouch! :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
chrome/common/extensions/api/_api_features.json lgtm
jdufault@, thank you for the review comments. Please find my reply below. I'm investigating the root case of browsertest failures, think that might be related to changes in option_ui.cc Once that's resolved, I will consider to refactor the long if statement there... Thanks! https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:175: <inner-text>[[i18n('pinKeyboard1')]]</inner-text> On 2016/08/11 18:34:10, jdufault wrote: > Can you add a TODO to revert this change once we don't need to support options? Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:440: settings.navigateTo(settings.Route.LOCK_SCREEN); On 2016/08/11 18:34:10, jdufault wrote: > nit: indentation Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/compiled_resources.gyp (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/compiled_resources.gyp:46: '../settings/compiled_resources2.gyp:route', On 2016/08/11 18:34:10, jdufault wrote: > nit: indentation Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/sync_section.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/options/sync_section.html:47: <a is="action-link" id="manage-screenlock" i18n-content="manageScreenlock" hidden> On 2016/08/11 18:34:10, jdufault wrote: > nit: newline instead of wrapping Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/compiled_resources2.gyp (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/compiled_resources2.gyp:73: '<(DEPTH)/ui/webui/resources/js/compiled_resources2.gyp:i18n_behavior', On 2016/08/11 18:34:10, jdufault wrote: > nit: indentation Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:28: [[i18n('lockScreenPasswordOnly')]] On 2016/08/11 18:34:10, jdufault wrote: > Please add a TODO to revert these changes once we don't need to support options. Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:42: <div class="settings-box single-column"> On 2016/08/11 18:34:10, jdufault wrote: > Should the hidden$ be on this div instead? Thanks Jacob. I could move it here. One thing is it will also hide the spacing that comes with settings-checkbox which we might not care about. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:44: label="$i18n{enableScreenlock}" hidden$="[[hideEnableSettings_()]]"> On 2016/08/11 18:34:10, jdufault wrote: > You can use the property directly instead of creating a helper function. > > hidden$="[[hideEnableScreenlock]]" Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:44: label="$i18n{enableScreenlock}" hidden$="[[hideEnableSettings_()]]"> On 2016/08/11 18:34:10, jdufault wrote: > You can use the property directly instead of creating a helper function. > > hidden$="[[hideEnableScreenlock]]" Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:43: */ On 2016/08/11 18:34:10, jdufault wrote: > Add a TODO to remove this property after options is gone. Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:106: document.dispatchEvent(new Event('close-overlay')); On 2016/08/11 18:34:11, jdufault wrote: > Add a TODO to remove this event after options is gone. Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:106: document.dispatchEvent(new Event('close-overlay')); On 2016/08/11 18:34:10, jdufault wrote: > Can you make it work by firing an event on the element? > > this.fire('close-overlay') > > If it's going to be significant effort don't bother. Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.html:1: <link rel="import" href="chrome://resources/html/i18n_behavior.html"> On 2016/08/11 18:34:11, jdufault wrote: > Please order this alphabetically. Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.html:22: <div class="title">[[i18n('passwordPromptTitle')]]</div> On 2016/08/11 18:34:11, jdufault wrote: > Please add a TODO to revert these changes when we can drop options support. Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.js (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.js:50: behaviors: [ On 2016/08/11 18:34:11, jdufault wrote: > Add TODO to remove Done. https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/setup_pin_dialog.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/setup_pin_dialog.html:55: [[i18n('cancel')]] On 2016/08/11 18:34:11, jdufault wrote: > Add TODO to revert Done.
Looks like browsertest failed because some overlays cannot be opened using URLs. for example: chrome://settings-frame/resetProfileSettings cannot be opened. I'm quite confused why this is happening, what should I do to resolve this issue.
lgtm https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:42: <div class="settings-box single-column"> On 2016/08/11 23:59:51, xiaoyinh wrote: > On 2016/08/11 18:34:10, jdufault wrote: > > Should the hidden$ be on this div instead? > > Thanks Jacob. I could move it here. One thing is it will also hide the spacing > that comes with settings-checkbox which we might not care about. Okay, feel free to leave it wherever it visually looks best. https://codereview.chromium.org/2236213002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2236213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: <!-- TODO(xiaoyinh@): revert changes to i18n text What about a comment like this? Use $i18n{id} format once we don't need to support options In a few months when reading this it might be unclear what revert means.
On 2016/08/12 21:44:38, xiaoyinh wrote: > Looks like browsertest failed because some overlays cannot be opened using URLs. > for example: > chrome://settings-frame/resetProfileSettings cannot be opened. > > I'm quite confused why this is happening, what should I do to resolve this > issue. Is this happening locally, or only on the trybots? It might be a flake. If it's not happening locally I'd submit another dry run.
On 2016/08/12 21:59:04, jdufault wrote: > On 2016/08/12 21:44:38, xiaoyinh wrote: > > Looks like browsertest failed because some overlays cannot be opened using > URLs. > > for example: > > chrome://settings-frame/resetProfileSettings cannot be opened. > > > > I'm quite confused why this is happening, what should I do to resolve this > > issue. > > Is this happening locally, or only on the trybots? It might be a flake. If it's > not happening locally I'd submit another dry run. This is happening locally as well. Note that not all overlays has this issue. I can open chrome://settings-frame/languages with no problem.
The CQ bit was checked by xiaoyinh@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
stevenjb@chromium.org changed reviewers: + dbeam@chromium.org, tommycli@chromium.org
+dbeam@, +tommycli@ This seems like a lot of change. I know we need this for 54, but I'm a little concerend about the complexity here. I am wondering if we couldn't just open an md-settings page with just the quick unlock settings instead?
On 2016/08/15 18:50:19, stevenjb wrote: > +dbeam@, +tommycli@ > > This seems like a lot of change. I know we need this for 54, but I'm a little > concerend about the complexity here. I am wondering if we couldn't just open an > md-settings page with just the quick unlock settings instead? Even though the +line count is high, I think most of the code here isn't too risky. It'd be great if the settings.Route logic was stubbed out instead of being duplicated. The other big risk I see is a change to how the options assets are loaded in options_ui.cc - maybe it's worthwhile to go back to the implementation in PS2 just so that there is less overall risk even if the code isn't as clean. What are the major areas of concern that you have?
https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/route.js (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/route.js:9: // I add a copy in options instead of import it directly from settings Can we avoid loading the polymer elements until after we need to show PIN keyboard? What about just creating a simple stub for this class? https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/sync_section.html (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/sync_section.html:47: <a is="action-link" id="manage-screenlock" ? https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:19: .radio-indent[hidden] { Are these needed? Add TODO to revert if so. https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:388: { "accessibilityAutoclick", Please keep style consistent; I'd revert the existing style changes. https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:147: (const base::DictionaryValue& localized_strings) { This formatting looks strange. Did you run git cl format? https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:151: source->AddLocalizedStrings(localized_strings); What changed so that this approach now works? Is the previous data source implementation being used at the same time, or does this one replace it entirely? https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:153: source->SetDefaultResource(IDR_OPTIONS_HTML); Do you need this? https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:448: // Set uo the chrome://settings-frame/source uo -> up
The CQ bit was checked by xiaoyinh@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...
In PS5, I have moved the sources loading logic back to URLDataSource, to keep consistent with how options_ui.cc was doing previously. Instead of duplicate the route.js in options, I also create a stub class for the routing logic in route_stub.js, this only keeps minimum functionality to integrate with lock_screen.js Thanks! https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:42: <div class="settings-box single-column"> On 2016/08/12 21:45:35, jdufault wrote: > On 2016/08/11 23:59:51, xiaoyinh wrote: > > On 2016/08/11 18:34:10, jdufault wrote: > > > Should the hidden$ be on this div instead? > > > > Thanks Jacob. I could move it here. One thing is it will also hide the spacing > > that comes with settings-checkbox which we might not care about. > > Okay, feel free to leave it wherever it visually looks best. Done. https://codereview.chromium.org/2236213002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2236213002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: <!-- TODO(xiaoyinh@): revert changes to i18n text On 2016/08/12 21:45:35, jdufault wrote: > What about a comment like this? > > Use $i18n{id} format once we don't need to support options > > In a few months when reading this it might be unclear what revert means. Done. https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/route.js (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/route.js:9: // I add a copy in options instead of import it directly from settings On 2016/08/15 19:21:18, jdufault wrote: > Can we avoid loading the polymer elements until after we need to show PIN > keyboard? > > What about just creating a simple stub for this class? Thanks Jacob! Could you take a look at route_stub.js? https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/sync_section.html (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/sync_section.html:47: <a is="action-link" id="manage-screenlock" On 2016/08/15 19:21:18, jdufault wrote: > ? This is incorporating your previous comment to use newline instead of wrapping https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:19: .radio-indent[hidden] { On 2016/08/15 19:21:18, jdufault wrote: > Are these needed? > > Add TODO to revert if so. Yes. It is very weird that in the options the hidden attribute is not working unless I set it to "display: none" specifically. I have added TODO. Thanks! https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:388: { "accessibilityAutoclick", On 2016/08/15 19:21:18, jdufault wrote: > Please keep style consistent; I'd revert the existing style changes. I have run the git cl format, hope this time it's ok. https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:147: (const base::DictionaryValue& localized_strings) { On 2016/08/15 19:21:18, jdufault wrote: > This formatting looks strange. Did you run git cl format? This part has been removed. Thanks! https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:151: source->AddLocalizedStrings(localized_strings); On 2016/08/15 19:21:18, jdufault wrote: > What changed so that this approach now works? Is the previous data source > implementation being used at the same time, or does this one replace it > entirely? I just found out that WebUIDataSource is a wrapper of URLDataSource. For the same host, I shouldn't use both WebUIDataSource and URLDataSource, one will be overwritten by another. Since in option_ui.cc we originally use URLDataSource, I will keep the new resources loading in URLDataSource as well. Thanks! https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:153: source->SetDefaultResource(IDR_OPTIONS_HTML); On 2016/08/15 19:21:18, jdufault wrote: > Do you need this? This part has been removed. Thanks! https://codereview.chromium.org/2236213002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:448: // Set uo the chrome://settings-frame/source On 2016/08/15 19:21:18, jdufault wrote: > uo -> up This part has been removed. Thanks!
On 2016/08/15 18:50:19, stevenjb wrote: > +dbeam@, +tommycli@ > > This seems like a lot of change. I know we need this for 54, but I'm a little > concerend about the complexity here. I am wondering if we couldn't just open an > md-settings page with just the quick unlock settings instead? stevenjb@, could you take a look at the new patch (PS5) and let me know your thoughts? Hopefully this patch looks clean and less changes to existing logic.
lgtm https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.html:3: <h1 i18n-content="lockScreenTitle"> </h1> nit: removing middle space <h1 ...></h1> https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/route_stub.js (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/route_stub.js:27: <if expr="chromeos"> This file should only be included if we're on cros, can we drop the <if>? https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.html:21: <!--TODO(xiaoyinh@): Use this format $i18n{passwordPromptPasswordLabel} once Is there trailing whitespace at the end of this line?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options_resources.grd:20: <structure name="IDR_OPTIONS_PIN_KEYBOARD_HTML" Can we only include them for chromeos build? https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:120: const char kPinKeyboardHTMLPath[] = "people_page/pin_keyboard.html"; nit: constexpr char... here and other places https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:237: path_to_idr_map_[kPinKeyboardHTMLPath] = IDR_OPTIONS_PIN_KEYBOARD_HTML; If we include these resources only for chormeos, this function needs update too.
tommycli@chromium.org changed reviewers: + dschuyler@chromium.org
Two major suggestions below can make this patch smaller: i18n-preprocess Options, and axe most of the route stubbing. Thanks! https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: <!-- TODO(xiaoyinh@): Use this format: $i18n{pinKeyboardClear} +dschuyler: Can we make Options just i18n pre-process Options instead of making these changes in this file? https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:440: settings.navigateTo(settings.Route.LOCK_SCREEN); Instead of stubbing out all of route.js, does it work if you just do this here: ? settings.getCurrentRoute = function() { return settings.Route.LOCK_SCREEN; };
I added a couple of my specific concerns. I'm mostly not excited about the many small changes to settings/people_page to support options. That said, if tommycli@ and/or dbeam@ are OK with this, then I am OK with it. https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:177: <inner-text>[[i18n('pinKeyboard1')]]</inner-text> I hate that we need to do all of these changes just to have to undo them later. If we opened this as an iframe or a separate browser window instead we wouldn't need to. https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/options.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/options.html:17: <link rel="import" href="chrome://settings-frame/people_page/lock_screen.html"> Could we move all of these into a separate options_polymer.html file? https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/route_stub.js (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/route_stub.js:8: // It should be removed once we don't need to support options. This is one of the pieces that strikes me as somewhat fragile. It would be nice if we could avoid the need for it somehow.
On 2016/08/16 20:59:52, stevenjb wrote: > I added a couple of my specific concerns. I'm mostly not excited about the many > small changes to settings/people_page to support options. That said, if > tommycli@ and/or dbeam@ are OK with this, then I am OK with it. > > https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): > > https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:177: > <inner-text>[[i18n('pinKeyboard1')]]</inner-text> > I hate that we need to do all of these changes just to have to undo them later. > If we opened this as an iframe or a separate browser window instead we wouldn't > need to. > > https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/options/options.html (right): > > https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... > chrome/browser/resources/options/options.html:17: <link rel="import" > href="chrome://settings-frame/people_page/lock_screen.html"> > Could we move all of these into a separate options_polymer.html file? > > https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/options/route_stub.js (right): > > https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... > chrome/browser/resources/options/route_stub.js:8: // It should be removed once > we don't need to support options. > This is one of the pieces that strikes me as somewhat fragile. It would be nice > if we could avoid the need for it somehow. If it's possible to open this Settings page in an IFRAME or popup-window, that would be much preferable to embedding the whole element within Options. That being said, I have suggested two changes that would make this patch more palatable (smaller). 1. Remove all the i18n changes and have Options go through the same i18n pre-processor as Settings. 2. Remove most all of the Route stubbing. Just override the settings.getCurrentRoute method, and settings.RouteObserverBehavior = {}; If those two things are done, I'll be okay with how this is done. But I'm not an Options owner - dbeam needs to weigh in.
One more question: https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:19: don't need to support options --> Why are these needed for Options? Doesn't hidden mean the same thing as display: none?
On 2016/08/16 21:10:24, tommycli wrote: > One more question: > > https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/settings/people_page/lock_screen.html (right): > > https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... > chrome/browser/resources/settings/people_page/lock_screen.html:19: don't need to > support options --> > Why are these needed for Options? Doesn't hidden mean the same thing as display: > none? When I tested it in options, a hidden elements still shows up unless it is specifically set to display:none in here. I'm quite confused with this behavior as well.
Hi, thank you so much for the comments. In this patch I have: 1. Incorporate comments from jdufault@ and xiyuan@ 2. Add the logic to pre-process i18n in options (dschuyler@, could you take a look at this part?)and removed all the i18n changes in settings. 3. removed most of route_stub.js tommycli@, stevenjb@ , could you take another look and let me know? dbeam@, could you chime in as well? Thanks! https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:174: <!-- TODO(xiaoyinh@): Use this format: $i18n{pinKeyboardClear} On 2016/08/16 20:08:35, tommycli wrote: > +dschuyler: Can we make Options just i18n pre-process Options instead of making > these changes in this file? Done. https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin_keyboard.html:177: <inner-text>[[i18n('pinKeyboard1')]]</inner-text> On 2016/08/16 20:59:52, stevenjb wrote: > I hate that we need to do all of these changes just to have to undo them later. > If we opened this as an iframe or a separate browser window instead we wouldn't > need to. After adding i18n pre-process in options, this part has been reverted. Thanks! https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:440: settings.navigateTo(settings.Route.LOCK_SCREEN); On 2016/08/16 20:08:35, tommycli wrote: > Instead of stubbing out all of route.js, does it work if you just do this here: > ? > > settings.getCurrentRoute = function() { return settings.Route.LOCK_SCREEN; }; I have removed most of it, please take a look. Thanks! https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.html:3: <h1 i18n-content="lockScreenTitle"> </h1> On 2016/08/16 17:43:14, jdufault wrote: > nit: removing middle space <h1 ...></h1> Done. https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/options.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/options.html:17: <link rel="import" href="chrome://settings-frame/people_page/lock_screen.html"> On 2016/08/16 20:59:52, stevenjb wrote: > Could we move all of these into a separate options_polymer.html file? Done. https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/route_stub.js (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/route_stub.js:8: // It should be removed once we don't need to support options. On 2016/08/16 20:59:52, stevenjb wrote: > This is one of the pieces that strikes me as somewhat fragile. It would be nice > if we could avoid the need for it somehow. I have removed most of it, please take a look. Thanks! https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/route_stub.js:27: <if expr="chromeos"> On 2016/08/16 17:43:14, jdufault wrote: > This file should only be included if we're on cros, can we drop the <if>? Removed, thanks! https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options_resources.grd:20: <structure name="IDR_OPTIONS_PIN_KEYBOARD_HTML" On 2016/08/16 18:19:47, xiyuan wrote: > Can we only include them for chromeos build? Done. https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/password_prompt_dialog.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/password_prompt_dialog.html:21: <!--TODO(xiaoyinh@): Use this format $i18n{passwordPromptPasswordLabel} once On 2016/08/16 17:43:14, jdufault wrote: > Is there trailing whitespace at the end of this line? Removed. Thanks! https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:120: const char kPinKeyboardHTMLPath[] = "people_page/pin_keyboard.html"; On 2016/08/16 18:19:47, xiyuan wrote: > nit: constexpr char... here and other places Done. https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:237: path_to_idr_map_[kPinKeyboardHTMLPath] = IDR_OPTIONS_PIN_KEYBOARD_HTML; On 2016/08/16 18:19:47, xiyuan wrote: > If we include these resources only for chormeos, this function needs update too. Done.
https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:53: label="$i18n{enableScreenlock}" hidden$="[[hideEnableScreenlock]]"> Newline instead of wrapping https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:477: IDS_SETTINGS_PEOPLE_CONFIGURE_PIN_CHOOSE_PIN_TITLE}, I think it'd be helpful to revert the styling changes here so it's easy to see what strings are being added. You might not need to add all of these strings since they are now being added using the string replacement logic.
some further comments. https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/route_stub.js (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/route_stub.js:107: return { I think in this whole file, all you need is: var Route = { LOCK_SCREEN: 0, }, return { Route: Route, getCurrentRoute: function() { return Route.LOCK_SCREEN; }, RouteObserverBehavior: {}, } https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:19: don't need to support options --> On 2016/08/16 21:10:24, tommycli wrote: > Why are these needed for Options? Doesn't hidden mean the same thing as display: > none? We should find out why hidden doesn't work in Options before proceeding. https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:45: hideEnableScreenlock: { Instead of adding this Polymer property here for hiding, can the embedding overlay programmatically set the element to hidden? https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:108: this.fire('close-overlay'); Likewise, can the embedding code find the inner settings-password-prompt-dialog element and attach an event listener independently?
https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:240: if (path == kLocalizedStringsFile || path == kOptionsBundleJsFile) nit: We can get rid of this since the added "if" below should cover it as well. https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:290: it.value().DeepCopy()->GetAsString(&str_value); A couple of problems: 1. DeepCopy() is wrong and it leaks. We should just call GetAsString. 2. |localized_strings| name is misleading. It is actually load time data and contains boolean as well. We should probably do if (!it.value().GetAsString(&str_value)) continue;
The CQ bit was checked by xiaoyinh@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...
PS7 has removed route_stub.html/js files and all the changes in settings. Also changed quick_unlock_private_api.cc to fix some browser test failure. devlin@, jdufault@ could you take a look? https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/options/route_stub.js (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/options/route_stub.js:107: return { This file has been removed. Thanks! https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:19: don't need to support options --> On 2016/08/17 21:19:58, tommycli wrote: > On 2016/08/16 21:10:24, tommycli wrote: > > Why are these needed for Options? Doesn't hidden mean the same thing as > display: > > none? > > We should find out why hidden doesn't work in Options before proceeding. In settings we have imported chrome://resources/polymer/v1_0/iron-flex-layout/classes/iron-flex-layout.html which has the following style that set hidden to none. [hidden] { display: none !important; } I have imported that file in options as well, now it works fine. https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:53: label="$i18n{enableScreenlock}" hidden$="[[hideEnableScreenlock]]"> On 2016/08/17 21:07:29, jdufault wrote: > Newline instead of wrapping This has been removed. Please take another look, thanks! https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:45: hideEnableScreenlock: { On 2016/08/17 21:19:59, tommycli wrote: > Instead of adding this Polymer property here for hiding, can the embedding > overlay programmatically set the element to hidden? Done. https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:108: this.fire('close-overlay'); On 2016/08/17 21:19:59, tommycli wrote: > Likewise, can the embedding code find the inner settings-password-prompt-dialog > element and attach an event listener independently? Done. https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:477: IDS_SETTINGS_PEOPLE_CONFIGURE_PIN_CHOOSE_PIN_TITLE}, On 2016/08/17 21:07:29, jdufault wrote: > I think it'd be helpful to revert the styling changes here so it's easy to see > what strings are being added. You might not need to add all of these strings > since they are now being added using the string replacement logic. Don't know why it looks like this, maybe it was formatted by git cl format. I have reverted the style change. If I understand it correctly, the replacement logic will make a copy from localized_strings and use the mappings to replace i18n{id} to the actual string. So these may still be needed to create mappings in options's localized_string https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:240: if (path == kLocalizedStringsFile || path == kOptionsBundleJsFile) On 2016/08/17 21:30:35, xiyuan wrote: > nit: We can get rid of this since the added "if" below should cover it as well. Done. https://codereview.chromium.org/2236213002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:290: it.value().DeepCopy()->GetAsString(&str_value); On 2016/08/17 21:30:35, xiyuan wrote: > A couple of problems: > 1. DeepCopy() is wrong and it leaks. We should just call GetAsString. > 2. |localized_strings| name is misleading. It is actually load time data and > contains boolean as well. We should probably do > > if (!it.value().GetAsString(&str_value)) > continue; Oops :(, thanks for catching that!
Since there is no longer any changes to anything in Settings or any route stubbing, I have no objections. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 xiaoyinh@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.
dudddddddde, this is a lot of code why are we using polymer in the old options UI? this bums me out
On 2016/08/19 06:39:20, Dan Beam wrote: > dudddddddde, this is a lot of code > > why are we using polymer in the old options UI? > > this bums me out md-settings isn't shipping in time :(
On 2016/08/19 06:39:20, Dan Beam wrote: > dudddddddde, this is a lot of code > > why are we using polymer in the old options UI? > > this bums me out This is unfortunate, but for +350 SLOC, it could have been worse (check out prior patchsets), and it's probably worth. The IFRAME approach would have been cool, but I suspect it would have been about +350 SLOC anyways to make it work right. Out of curiosity, any screenshots of how it looks in Options? Tommy
On 2016/08/19 17:46:45, tommycli wrote: > On 2016/08/19 06:39:20, Dan Beam wrote: > > dudddddddde, this is a lot of code > > > > why are we using polymer in the old options UI? > > > > this bums me out > > This is unfortunate, but for +350 SLOC, it could have been worse (check out > prior patchsets), and it's probably worth. The IFRAME approach would have been > cool, but I suspect it would have been about +350 SLOC anyways to make it work > right. > > Out of curiosity, any screenshots of how it looks in Options? > > Tommy Screenshot added in crbug.com/629662 Thanks!
On 2016/08/19 16:46:25, jdufault wrote: > On 2016/08/19 06:39:20, Dan Beam wrote: > > dudddddddde, this is a lot of code > > > > why are we using polymer in the old options UI? > > > > this bums me out > > md-settings isn't shipping in time :( but options also isn't md-settings. i don't think mixing them in the same page is acceptable. if you want to try to more clearly differentiate somehow, that's fine. but this will likely never pass UI review
not lgtm until I see some proof that UI people are OK with this
A couple nits. The ReplaceTemplates part of the CL LGTM. https://codereview.chromium.org/2236213002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.html (right): https://codereview.chromium.org/2236213002/diff/140001/chrome/browser/resourc... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.html:5: prefs="{{prefs}}"> nit: unwrap line. https://codereview.chromium.org/2236213002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2236213002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:757: for (int j = 0; j <= 9; j++) { nit: Please use pre-increment unless post-increment is specifically needed. "++j"
On 2016/08/19 18:19:17, Dan Beam wrote: > not lgtm until I see some proof that UI people are OK with this enough people have vouched that we really want to do mix polymer and kennedy styles. ok, lgtm in a very ironic sense.
lgtm
The CQ bit was checked by xiaoyinh@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/2236213002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.html (right): https://codereview.chromium.org/2236213002/diff/140001/chrome/browser/resourc... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.html:5: prefs="{{prefs}}"> On 2016/08/19 18:27:38, dschuyler wrote: > nit: unwrap line. Done. https://codereview.chromium.org/2236213002/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2236213002/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:757: for (int j = 0; j <= 9; j++) { On 2016/08/19 18:27:38, dschuyler wrote: > nit: Please use pre-increment unless > post-increment is specifically needed. > "++j" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaoyinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, jdufault@chromium.org, tommycli@chromium.org, dbeam@chromium.org, xiyuan@chromium.org, dschuyler@chromium.org Link to the patchset: https://codereview.chromium.org/2236213002/#ps160001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add quick unlock Settings in options page BUG=629662 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add quick unlock Settings in options page BUG=629662 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Add quick unlock Settings in options page BUG=629662 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add quick unlock Settings in options page BUG=629662 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/36a1cc2dac63adf7cf223a158ce9826c3f2fa98b Cr-Commit-Position: refs/heads/master@{#413268} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/36a1cc2dac63adf7cf223a158ce9826c3f2fa98b Cr-Commit-Position: refs/heads/master@{#413268} |
