Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691943005:
fix quick unlock config in options (Closed)
|
Created:
3 years, 10 months ago by xiaoyinh(OOO Sep 11-29) Modified:
3 years, 9 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis CL is trying to fix 2 issues:
1. Quick unlock configuration in options are importing polymer elements(lock_screen.html) created in settings, that includes importing all the elements that lock_screen.html depends on. So if there's new polymer elements imported in lock_screen.html, options wouldn't know where to find them, thus not able to load the lock_screen element. This CL makes options page know these newly added elements.
2. Trying to pre-load polymer elements in options page for better user experience.
BUG=679795
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2691943005
Cr-Commit-Position: refs/heads/master@{#453733}
Committed: https://chromium.googlesource.com/chromium/src/+/a7b35b6e3091d62218d6093633589997036e3b98
Patch Set 1 #Patch Set 2 : set timeout #
Total comments: 6
Patch Set 3 : Incorporate comments #
Total comments: 18
Patch Set 4 : incorporate comments #Patch Set 5 : update options_resources.grd #Patch Set 6 : rebase #
Total comments: 2
Patch Set 7 : incorporate comments #
Total comments: 4
Patch Set 8 : incorporate comments #
Messages
Total messages: 62 (42 generated)
Description was changed from ========== fix quickunlock options BUG=679795 ========== to ========== fix quickunlock options BUG=679795 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: 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.
xiaoyinh@chromium.org changed reviewers: + dbeam@chromium.org, jdufault@chromium.org, sammiequon@chromium.org
lgtm https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js (right): https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:46: // md-settings are lanuching soon. nit: are lanuching -> is launching https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/i18n_setup.html (right): https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... chrome/browser/resources/options/i18n_setup.html:1: <!-- Intentionally leave it empty nit: change first line to "This file is intentionally empty." https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... chrome/browser/resources/options/i18n_setup.html:2: settings-lock-screen element will import i18n_setup.html file in settings. The scripts included by nit: cleanup wrapping/line length
https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js (right): https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:46: // md-settings are lanuching soon. On 2017/02/15 23:53:22, jdufault wrote: > nit: are lanuching -> is launching Done. https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... File chrome/browser/resources/options/i18n_setup.html (right): https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... chrome/browser/resources/options/i18n_setup.html:1: <!-- Intentionally leave it empty On 2017/02/15 23:53:22, jdufault wrote: > nit: change first line to "This file is intentionally empty." Done. https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... chrome/browser/resources/options/i18n_setup.html:2: settings-lock-screen element will import i18n_setup.html file in settings. The scripts included by On 2017/02/15 23:53:22, jdufault wrote: > nit: cleanup wrapping/line length Done.
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...
On 2017/02/17 01:31:44, xiaoyinh wrote: > https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... > File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js > (right): > > https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... > chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:46: > // md-settings are lanuching soon. > On 2017/02/15 23:53:22, jdufault wrote: > > nit: are lanuching -> is launching > > Done. > > https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... > File chrome/browser/resources/options/i18n_setup.html (right): > > https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... > chrome/browser/resources/options/i18n_setup.html:1: <!-- Intentionally leave it > empty > On 2017/02/15 23:53:22, jdufault wrote: > > nit: change first line to "This file is intentionally empty." > > Done. > > https://codereview.chromium.org/2691943005/diff/20001/chrome/browser/resource... > chrome/browser/resources/options/i18n_setup.html:2: settings-lock-screen element > will import i18n_setup.html file in settings. The scripts included by > On 2017/02/15 23:53:22, jdufault wrote: > > nit: cleanup wrapping/line length > > Done. I would love it if the CL description was a little bit more detailed - what exactly is broken/is getting fixed? (Alas, the bug doesn't really have much detail either)
https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:11: * we start to load polymer elements. please wrap comments at 80 cols https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:42: document.onreadystatechange = function () { this is not properly formatted, please run git cl format --js https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:44: // Setting a timeout here to avoid racing with some browsertests, i don't think this guarantees anything about racing with browser tests https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:50: }.bind(this); can you just do window.addEventListener('load', function() { }.bind(this)); instead? https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:88: 'div.settings-box'); please reformat this with git cl format --js https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/i18n_setup.html (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/i18n_setup.html:6: --> this is crazy sauce https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options_resources.grd:167: allowexternalscript="true" /> why are all these flattenhtml="true" and allowexternalscript="true" needed? https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:96: settings.getCurrentRoute()); say whattttt?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== fix quickunlock options BUG=679795 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== This CL is trying to fix 2 issues: 1. Quick unlock configuration in options are importing polymer elements(lock_screen.html) created in settings, that includes importing all the elements that lock_screen.html depends on. So if there's new polymer elements imported in lock_screen.html, options wouldn't know where to find them, thus not able to load the lock_screen element. This CL makes options page know these newly added elements. 2. We are trying to pre-load polymer elements in options page. Using requestIdleCallback seems to race with some browsertests and I'm trying to avoid that by using setTimeout. BUG=679795 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== This CL is trying to fix 2 issues: 1. Quick unlock configuration in options are importing polymer elements(lock_screen.html) created in settings, that includes importing all the elements that lock_screen.html depends on. So if there's new polymer elements imported in lock_screen.html, options wouldn't know where to find them, thus not able to load the lock_screen element. This CL makes options page know these newly added elements. 2. We are trying to pre-load polymer elements in options page. Using requestIdleCallback seems to race with some browsertests and I'm trying to avoid that by using setTimeout. BUG=679795 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== This CL is trying to fix 2 issues: 1. Quick unlock configuration in options are importing polymer elements(lock_screen.html) created in settings, that includes importing all the elements that lock_screen.html depends on. So if there's new polymer elements imported in lock_screen.html, options wouldn't know where to find them, thus not able to load the lock_screen element. This CL makes options page know these newly added elements. 2. Trying to pre-load polymer elements in options page for better user experience. BUG=679795 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
groby@, I update the description of the CL and hope it make things more clear. https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:11: * we start to load polymer elements. On 2017/02/17 02:45:17, Dan Beam wrote: > please wrap comments at 80 cols Done. https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:42: document.onreadystatechange = function () { On 2017/02/17 02:45:17, Dan Beam wrote: > this is not properly formatted, please run git cl format --js Done. https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:44: // Setting a timeout here to avoid racing with some browsertests, On 2017/02/17 02:45:17, Dan Beam wrote: > i don't think this guarantees anything about racing with browser tests Thanks, I think the failed browsertests are running after the options page is loaded. Setting a timeout here so that the browser tests could complete before we start to load the polymer elements. I understand this is not ideal and guaranteed, but I'm not sure if there's another way to do it? Is there a way to know if this is running as part of the browsertests vs regular routes? https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:50: }.bind(this); On 2017/02/17 02:45:17, Dan Beam wrote: > can you just do > > window.addEventListener('load', function() { > > }.bind(this)); > > instead? Done. https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:88: 'div.settings-box'); On 2017/02/17 02:45:17, Dan Beam wrote: > please reformat this with git cl format --js Done. https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/i18n_setup.html (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options/i18n_setup.html:6: --> On 2017/02/17 02:45:17, Dan Beam wrote: > this is crazy sauce Sorry about these. It's particularly difficult to use something that is not designed for options. But these things won't be here for long. https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options_resources.grd:167: allowexternalscript="true" /> On 2017/02/17 02:45:17, Dan Beam wrote: > why are all these flattenhtml="true" and allowexternalscript="true" needed? Removed. https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:96: settings.getCurrentRoute()); On 2017/02/17 02:45:17, Dan Beam wrote: > say whattttt? I'm not sure why we want to explicitly set the route "LOCK_SCREEN" when the element is attached. In other places, it seems to use navigateTo to change route. This change should not impact settings functionality though, if I understand it correctly, there're 2 ways to navigate to lock screen page: 1. Navigate from people page: It will setCurrentRoute to LOCK_SCREEN in people_page.js by using settings.navigateTo(settings.Route.LOCK_SCREEN); 2. Navigate to lockscreen directly by using chrome://md-settings/lockScreen. route.js will call initializeRouteFromUrl and set the current route to LOCK_SCREEN. Either way, the currentRoute is set to LOCK_SCREEN before calling attached(). The change is mainly for options page, because we want to pre-load the lock screen element now, the attached function will get called even though the quick unlock overlay is still hidden, setting the route to LOCK_SCREEN will open a (hidden)password dialog before we want to configure the pin in options.
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/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options_resources.grd:167: allowexternalscript="true" /> On 2017/02/17 19:33:46, xiaoyinh wrote: > On 2017/02/17 02:45:17, Dan Beam wrote: > > why are all these flattenhtml="true" and allowexternalscript="true" needed? > > Removed. so preprocess="true" is required for <if> and <include> flattenhtml="true" inlines things magically like <script src="..."> and url() in CSS are you sure you're not breaking some of these things? they all have a specific purpose, but people just copy them without understand them often :(
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... chrome/browser/resources/options_resources.grd:167: allowexternalscript="true" /> On 2017/02/17 20:21:15, Dan Beam wrote: > On 2017/02/17 19:33:46, xiaoyinh wrote: > > On 2017/02/17 02:45:17, Dan Beam wrote: > > > why are all these flattenhtml="true" and allowexternalscript="true" needed? > > > > Removed. > > so preprocess="true" is required for <if> and <include> > > flattenhtml="true" inlines things magically like <script src="..."> and url() in > CSS > > are you sure you're not breaking some of these things? > > they all have a specific purpose, but people just copy them without understand > them often :( There's no docs for what the various parameters mean, and the grit implementation doesn't make it super clear either :(
On 2017/02/17 20:21:16, Dan Beam wrote: > https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... > File chrome/browser/resources/options_resources.grd (right): > > https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... > chrome/browser/resources/options_resources.grd:167: allowexternalscript="true" > /> > On 2017/02/17 19:33:46, xiaoyinh wrote: > > On 2017/02/17 02:45:17, Dan Beam wrote: > > > why are all these flattenhtml="true" and allowexternalscript="true" needed? > > > > Removed. > > so preprocess="true" is required for <if> and <include> > > flattenhtml="true" inlines things magically like <script src="..."> and url() in > CSS > > are you sure you're not breaking some of these things? > > they all have a specific purpose, but people just copy them without understand > them often :( Thanks for the information. I will double check every single one. When should we use allowexternalscript="true"?
On 2017/02/17 21:54:01, xiaoyinh wrote: > On 2017/02/17 20:21:16, Dan Beam wrote: > > > https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/options_resources.grd (right): > > > > > https://codereview.chromium.org/2691943005/diff/40001/chrome/browser/resource... > > chrome/browser/resources/options_resources.grd:167: allowexternalscript="true" > > /> > > On 2017/02/17 19:33:46, xiaoyinh wrote: > > > On 2017/02/17 02:45:17, Dan Beam wrote: > > > > why are all these flattenhtml="true" and allowexternalscript="true" > needed? > > > > > > Removed. > > > > so preprocess="true" is required for <if> and <include> > > > > flattenhtml="true" inlines things magically like <script src="..."> and url() > in > > CSS > > > > are you sure you're not breaking some of these things? > > > > they all have a specific purpose, but people just copy them without understand > > them often :( > Thanks for the information. I will double check every single one. > When should we use allowexternalscript="true"? I'm fairly sure that allowexternalscripts only matters when flattenhtml="true". if you omit allowexternalscripts or say allowexternalscripts="false", <script src="file.js"> is replaced with the contents of files.js allowexternalscripts="true" means that <script src="file.js"> stays as a <script> that loads a URL (assuming i'm reading the code correctly) https://cs.chromium.org/chromium/src/tools/grit/grit/format/html_inline.py?ty...
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 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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.
Hi, I have incorporated all the above comments, would you please take another look? Thanks!
https://codereview.chromium.org/2691943005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2691943005/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:96: settings.getCurrentRoute(), settings.getCurrentRoute()); rather than just calling currentRouteChanged(), can you make a separate method like showLockScreen_() and call it from here if getCurrentRoute() == LOCK_SCREEN in attached()?
The CQ bit was checked by xiaoyinh@chromium.org to run a CQ dry run
https://codereview.chromium.org/2691943005/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2691943005/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:96: settings.getCurrentRoute(), settings.getCurrentRoute()); On 2017/02/23 06:33:51, Dan Beam wrote: > rather than just calling currentRouteChanged(), can you make a separate method > like showLockScreen_() and call it from here if getCurrentRoute() == LOCK_SCREEN > in attached()? Done.
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.
https://codereview.chromium.org/2691943005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2691943005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:93: this.showLockScreen_(); this probably ought to be like maybeShowLockScreen_() or showLockScreenIfNecessary_() or maybe /** * @param {!settings.Route} route * @return {boolean} Whether the lock screen should be shown. */ shouldShowLockScreen_: function(route) { return route == settings.Route.LOCK_SCREEN && !this.setModes_; }, and then: attached: function() { if (this.shouldShowLockScreen_(settings.getCurrentRoute())) this.$.passwordPrompt.open(); }, as well as something similar in currentRouteChanged() https://codereview.chromium.org/2691943005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:114: this.$.passwordPrompt.open(); right, but you didn't update this part of the code to use showLockScreen_
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/2691943005/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2691943005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:93: this.showLockScreen_(); On 2017/02/28 02:49:16, Dan Beam wrote: > this probably ought to be like maybeShowLockScreen_() or > showLockScreenIfNecessary_() or maybe > > /** > * @param {!settings.Route} route > * @return {boolean} Whether the lock screen should be shown. > */ > shouldShowLockScreen_: function(route) { > return route == settings.Route.LOCK_SCREEN && !this.setModes_; > }, > > and then: > > attached: function() { > if (this.shouldShowLockScreen_(settings.getCurrentRoute())) > this.$.passwordPrompt.open(); > }, > > as well as something similar in currentRouteChanged() Thanks for the suggestion. I changed name to be "shouldAskForPassword_" since we will open a password dialog if returns true. LockScreen here probably includes more things like setup pin, edit fingerprint etc. Please let me know if you think otherwise. https://codereview.chromium.org/2691943005/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.js:114: this.$.passwordPrompt.open(); On 2017/02/28 02:49:16, Dan Beam wrote: > right, but you didn't update this part of the code to use showLockScreen_ Oh, thanks! I've updated it.
lgtm
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 jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2691943005/#ps140001 (title: "incorporate comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488322543998280,
"parent_rev": "bd25aca5753c52eb1c0e348c0146589df14802bc", "commit_rev":
"a7b35b6e3091d62218d6093633589997036e3b98"}
Message was sent while issue was closed.
Description was changed from ========== This CL is trying to fix 2 issues: 1. Quick unlock configuration in options are importing polymer elements(lock_screen.html) created in settings, that includes importing all the elements that lock_screen.html depends on. So if there's new polymer elements imported in lock_screen.html, options wouldn't know where to find them, thus not able to load the lock_screen element. This CL makes options page know these newly added elements. 2. Trying to pre-load polymer elements in options page for better user experience. BUG=679795 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== This CL is trying to fix 2 issues: 1. Quick unlock configuration in options are importing polymer elements(lock_screen.html) created in settings, that includes importing all the elements that lock_screen.html depends on. So if there's new polymer elements imported in lock_screen.html, options wouldn't know where to find them, thus not able to load the lock_screen element. This CL makes options page know these newly added elements. 2. Trying to pre-load polymer elements in options page for better user experience. BUG=679795 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2691943005 Cr-Commit-Position: refs/heads/master@{#453733} Committed: https://chromium.googlesource.com/chromium/src/+/a7b35b6e3091d62218d609363358... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/a7b35b6e3091d62218d609363358...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2728563003/ by yutak@chromium.org. The reason for reverting is: This caused the following test to fail on ChromiumOS debug bots. I've confirmed reverting this patch would fix the errors on my local environment. PolicyPrefIndicatorTestInstance/PolicyPrefIndicatorTest.CheckPolicyIndicators/26 https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%....
Message was sent while issue was closed.
On 2017/03/01 09:11:32, Yuta Kitamura wrote: > A revert of this CL (patchset #8 id:140001) has been created in > https://codereview.chromium.org/2728563003/ by mailto:yutak@chromium.org. > > The reason for reverting is: This caused the following test to fail on > ChromiumOS > debug bots. I've confirmed reverting this patch would > fix the errors on my local environment. > > PolicyPrefIndicatorTestInstance/PolicyPrefIndicatorTest.CheckPolicyIndicators/26 > > https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... This change might also be responsible for the redness in the following bot: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
