Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1933913002:
Add a very basic PIN UI implementation that is shared between lock and settings. (Closed)
|
Created:
4 years, 7 months ago by jdufault Modified:
4 years, 7 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dzhioev+watch_chromium.org, achuith+watch_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 Base URL:
https://chromium.googlesource.com/chromium/src.git@lkgr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a very basic PIN UI implementation that is shared between lock and settings.
Most things are still in flux, but this will help provide some structure for the
rest of the code.
BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/22dfa22df0ddbcab9f47d189207a11bdea820d75
Cr-Commit-Position: refs/heads/master@{#393081}
Patch Set 1 #
Total comments: 65
Patch Set 2 : Address comments, add TODOs #
Total comments: 10
Patch Set 3 : Address comments #
Total comments: 6
Patch Set 4 : Use ResourceLoader, rename pin.* to pin_keyboard.* #
Total comments: 11
Patch Set 5 : Nits and fix tests #Patch Set 6 : Fix tests by removing a previously unused import that started getting used b/c of resource loader c… #Messages
Total messages: 30 (8 generated)
jdufault@chromium.org changed reviewers: + achuith@chromium.org, stevenjb@chromium.org, tommycli@chromium.org, xiyuan@chromium.org
+tommycli@ or stevenjb@ for c/b/r/settings and polymer in c/b/r/cros/quick_unlock +achuith@ or xiyuan@ for c/b/r/login, c/b/r/cros/quick_unlock, and ui/login Thanks!
stevenjb: I'll start looking at this on Monday.
General theme: Maybe break up this CL if that helps. There are some portions that might not be ready to go in yet. I did not review the C++ portions of this CL. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/custom_elements_lock.html (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/custom_elements_lock.html:1: <script src="chrome://oobe/custom_elements.js"></script> Does this direct the script to custom_elements_lock.js? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/custom_elements_lock.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/custom_elements_lock.js:30: window.addEventListener('DOMContentLoaded', function() { Can we commit the "naive" and obvious approach for the first pass and then address performance concerns after? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin.css (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:14: box-shadow: 0 4px 23px 5px rgba(0, 0, 0, 0.2), It appears that this shares CSS from user_pod_row.css. Can we de-duplicate? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:30: .btn { Can we use a more descriptive class name such as "digit-button"? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:51: color: #969696; Can we use --paper-grey-500 here? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:64: background: rgb(63,132,249); spaces between commas https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:96: #pin-input[type=number]::-webkit-inner-spin-button, missing \n above https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:102: [hidden] { Why is this necessary? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:103: display: none !important; Why is this necessary? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin.html (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:16: <input id="pin-input" type="number" placeholder="Enter PIN"></input> Typically we use i18n strings instead of string literals. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:20: <paper-button class="btn custom-appearance" id="btn1"> custom-appearance is no longer needed right? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:24: <inner-text>2<subhead>ABC</subhead></inner-text> What is the internationalization of the numeric pinpad? Is it truly 1 -> ABC, 2 -> DEF, internationally? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:5: (function() { Since there's no variables to hide, there's no need to do the (function() { thing, and we can unindent everything. Also I'm not sure if 'use strict' is necessary. I don't see it in the other Polymer element definitions https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:10: properties: { missing \n above this line https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:20: onUsernameChanged: function(username) { These are leftovers right? In any case, We can use these property change observers: https://www.polymer-project.org/1.0/docs/devguide/properties.html#change-call... https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:30: let input = this.$$('#pin-input'); Does Chromium support 'let'? i haven't seen much usage. This is an ES6 feature right? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:35: buttonElement.onclick = function() { Instead of doing this, you can declaratively choose an event handler for every paper-button within the HTML using on-tap attribute. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:41: let handleInputSubmitted = function(pin) { Likewise, use the declarative syntax for submitting the pin also. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:42: console.log('Submitting pin ' + pin); Remove stray console.logs https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:44: // TODO(jdufault): Use real values. Hmm... if we aren't using real values, I'm not sure this CL is ready to be committed to codebase. Are there smaller portions of this patch that are more ready? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:47: chrome.send('authenticateUserWithPin', [username, password]); Instead of doing chrome.send directly, try using the FooBrowserProxy model as shown here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... This will allow you to write browser tests for your Polymer element. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:50: input.onkeydown = function(event) { Likewise here, you should use the declarative event handler syntax. Here is the reference: https://www.polymer-project.org/1.0/docs/devguide/events.html#annotated-liste... https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:104: <if expr="chromeos"> Since below is also <if expr="chromeos"> you can combine both blocks. To confirm - have you checked with UX to determine if this should go above or below the Easy Unlock settings box? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:107: <p>Quick Unlock PIN</p> i18n https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.js:72: /* TODO(jdufault): Return a real value. */ We should probably return the real value before this is ready for committing. It's adding an item to the loadTimeData right? Look at how easy_unlock_settings_hnadler.cc is implemented: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Maybe this portion should wait for a separate CL? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_resources.grd:493: <if expr="chromeos"> There's an existing chromeos only section below at line 751.
https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin.html (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:57: <div id="submitContainer"> nit: We don't use camel case for element id. submitContainer -> submit-container https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:59: id="submitButton"></paper-icon-button> nit: submitButton -> submit-button https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:47: chrome.send('authenticateUserWithPin', [username, password]); nit: On C++ side, the handler expect a serialized AccountId [1]. https://code.google.com/p/chromium/codesearch#chromium/src/components/login/b... https://codereview.chromium.org/1933913002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1017: LOG(ERROR) << "Implement SigninScreenHandler::HandleAuthenticateUserWithPin"; nit: We have NOTIMPLEMENTED() for this purpose.
https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/custom_elements_lock.html (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/custom_elements_lock.html:1: <script src="chrome://oobe/custom_elements.js"></script> On 2016/05/02 16:11:26, tommycli wrote: > Does this direct the script to custom_elements_lock.js? Yes, custom_elements.js is a magic redirect to custom_elements_lock.js (and various other things) that gets setup in oobe_ui.cc. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/custom_elements_lock.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/custom_elements_lock.js:30: window.addEventListener('DOMContentLoaded', function() { On 2016/05/02 16:11:26, tommycli wrote: > Can we commit the "naive" and obvious approach for the first pass and then > address performance concerns after? We can't load this directly from the HTML file, because then it will be loaded when we are not using PIN unlock. We try to keep lock screen initialization time at a bare minimum. So it ends up that the there isn't an obvious approach (that I know of - any ideas?); I think the simplest we can do is to remove the requestIdleCallback invocation. I don't think having that adds much complexity, so I'd prefer to leave this as-is for now. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin.css (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:14: box-shadow: 0 4px 23px 5px rgba(0, 0, 0, 0.2), On 2016/05/02 16:11:26, tommycli wrote: > It appears that this shares CSS from user_pod_row.css. Can we de-duplicate? The only way I can think of is to split user_pod_row.css into another file, user_pod_row_shared.css, then <include> the css into each file. The shared code is ~8 lines. I'm not sure it is worth it. It's also possible the UI mocks will not include this common-theming in the settings. If that's the case, then this CSS will be removed and put somewhere else. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:30: .btn { On 2016/05/02 16:11:26, tommycli wrote: > Can we use a more descriptive class name such as "digit-button"? Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:51: color: #969696; On 2016/05/02 16:11:26, tommycli wrote: > Can we use --paper-grey-500 here? Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:64: background: rgb(63,132,249); On 2016/05/02 16:11:27, tommycli wrote: > spaces between commas Replaced with var(...). https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:96: #pin-input[type=number]::-webkit-inner-spin-button, On 2016/05/02 16:11:26, tommycli wrote: > missing \n above Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:102: [hidden] { On 2016/05/02 16:11:27, tommycli wrote: > Why is this necessary? Removed, stale/unused code. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.css:103: display: none !important; On 2016/05/02 16:11:26, tommycli wrote: > Why is this necessary? Removed, stale/unused code. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin.html (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:16: <input id="pin-input" type="number" placeholder="Enter PIN"></input> On 2016/05/02 16:11:27, tommycli wrote: > Typically we use i18n strings instead of string literals. Added TODO. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:20: <paper-button class="btn custom-appearance" id="btn1"> On 2016/05/02 16:11:27, tommycli wrote: > custom-appearance is no longer needed right? Done. When I wrote the element I initially used <button> - some global styles applied to it (even through the shady/shadow DOM), but those global styles do not apply to paper-button. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:24: <inner-text>2<subhead>ABC</subhead></inner-text> On 2016/05/02 16:11:27, tommycli wrote: > What is the internationalization of the numeric pinpad? Is it truly 1 -> ABC, 2 > -> DEF, internationally? Added TODO https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:57: <div id="submitContainer"> On 2016/05/02 20:03:33, xiyuan wrote: > nit: We don't use camel case for element id. submitContainer -> submit-container Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.html:59: id="submitButton"></paper-icon-button> On 2016/05/02 20:03:33, xiyuan wrote: > nit: submitButton -> submit-button Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/quick_unlock/pin.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:5: (function() { On 2016/05/02 16:11:27, tommycli wrote: > Since there's no variables to hide, there's no need to do the (function() { > thing, and we can unindent everything. > > Also I'm not sure if 'use strict' is necessary. I don't see it in the other > Polymer element definitions Done. iirc, the "use strict;" enables let. I've removed the usage of let so we don't need strict mode anymore. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:10: properties: { On 2016/05/02 16:11:27, tommycli wrote: > missing \n above this line Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:20: onUsernameChanged: function(username) { On 2016/05/02 16:11:27, tommycli wrote: > These are leftovers right? In any case, We can use these property change > observers: > https://www.polymer-project.org/1.0/docs/devguide/properties.html#change-call... I've removed these observers for now. I'll add them back later when they are needed. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:30: let input = this.$$('#pin-input'); On 2016/05/02 16:11:27, tommycli wrote: > Does Chromium support 'let'? i haven't seen much usage. This is an ES6 feature > right? Yes, Chromium is fine, but I'm not sure if we need to support iOS here so I've gone back to var. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:35: buttonElement.onclick = function() { On 2016/05/02 16:11:27, tommycli wrote: > Instead of doing this, you can declaratively choose an event handler for every > paper-button within the HTML using on-tap attribute. Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:41: let handleInputSubmitted = function(pin) { On 2016/05/02 16:11:27, tommycli wrote: > Likewise, use the declarative syntax for submitting the pin also. Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:42: console.log('Submitting pin ' + pin); On 2016/05/02 16:11:27, tommycli wrote: > Remove stray console.logs Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:44: // TODO(jdufault): Use real values. On 2016/05/02 16:11:27, tommycli wrote: > Hmm... if we aren't using real values, I'm not sure this CL is ready to be > committed to codebase. > > Are there smaller portions of this patch that are more ready? Discussed in chat. I'd like to land this CL to create some initial structure for collaboration and to resolve dependencies on other work that can be done in parallel. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:47: chrome.send('authenticateUserWithPin', [username, password]); On 2016/05/02 20:03:33, xiyuan wrote: > nit: On C++ side, the handler expect a serialized AccountId [1]. > > https://code.google.com/p/chromium/codesearch#chromium/src/components/login/b... Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:47: chrome.send('authenticateUserWithPin', [username, password]); On 2016/05/02 16:11:27, tommycli wrote: > Instead of doing chrome.send directly, try using the FooBrowserProxy model as > shown here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > This will allow you to write browser tests for your Polymer element. I've opted instead to just fire an event, which the embedder can listen to. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/quick_unlock/pin.js:50: input.onkeydown = function(event) { On 2016/05/02 16:11:27, tommycli wrote: > Likewise here, you should use the declarative event handler syntax. Here is the > reference: > https://www.polymer-project.org/1.0/docs/devguide/events.html#annotated-liste... Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:104: <if expr="chromeos"> On 2016/05/02 16:11:27, tommycli wrote: > Since below is also <if expr="chromeos"> you can combine both blocks. > > To confirm - have you checked with UX to determine if this should go above or > below the Easy Unlock settings box? I combined the blocks and added a TODO. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.html:107: <p>Quick Unlock PIN</p> On 2016/05/02 16:11:27, tommycli wrote: > i18n Added a TODO. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.js:72: /* TODO(jdufault): Return a real value. */ On 2016/05/02 16:11:27, tommycli wrote: > We should probably return the real value before this is ready for committing. > It's adding an item to the loadTimeData right? > > Look at how easy_unlock_settings_hnadler.cc is implemented: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > Maybe this portion should wait for a separate CL? I think it makes sense to define this using a private API method call instead, because then we won't have to write a separate handler for the options page as well. So with the private API, pinUnlockAllowed will be defined something like: QuickUnlockMode[] modes = chrome.quickUnlockPrivate.getAllowedModes() return modes.includes(QuickUnlockMode.PIN); or maybe return chrome.quickUnlockPrivate.isAllowed() depending on if we treat all quick unlock modes the same w.r.t. policy. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/settings_resources.grd:493: <if expr="chromeos"> On 2016/05/02 16:11:27, tommycli wrote: > There's an existing chromeos only section below at line 751. Done. https://codereview.chromium.org/1933913002/diff/1/chrome/browser/ui/webui/chr... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/ui/webui/chr... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1017: LOG(ERROR) << "Implement SigninScreenHandler::HandleAuthenticateUserWithPin"; On 2016/05/02 20:03:33, xiyuan wrote: > nit: We have NOTIMPLEMENTED() for this purpose. Done.
https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/custom_elements_lock.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/custom_elements_lock.js:30: window.addEventListener('DOMContentLoaded', function() { On 2016/05/03 19:21:57, jdufault wrote: > On 2016/05/02 16:11:26, tommycli wrote: > > Can we commit the "naive" and obvious approach for the first pass and then > > address performance concerns after? > > We can't load this directly from the HTML file, because then it will be loaded > when we are not using PIN unlock. We try to keep lock screen initialization time > at a bare minimum. > > So it ends up that the there isn't an obvious approach (that I know of - any > ideas?); I think the simplest we can do is to remove the requestIdleCallback > invocation. I don't think having that adds much complexity, so I'd prefer to > leave this as-is for now. Hmm, this still seems pretty non-obvious. Can we load it directly in the HTML file and use Polymer's lazy registration to reduce the performance cost? https://www.polymer-project.org/1.0/docs/release-notes.html#v1-4-0 Or alternatively, if you really don't want to pay the cost, maybe load it in an IFRAME dynamically? https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/people_page/people_page.js:72: /* TODO(jdufault): Return a real value. */ On 2016/05/03 19:21:58, jdufault wrote: > On 2016/05/02 16:11:27, tommycli wrote: > > We should probably return the real value before this is ready for committing. > > It's adding an item to the loadTimeData right? > > > > Look at how easy_unlock_settings_hnadler.cc is implemented: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > Maybe this portion should wait for a separate CL? > > I think it makes sense to define this using a private API method call instead, > because then we won't have to write a separate handler for the options page as > well. > > So with the private API, pinUnlockAllowed will be defined something like: > > QuickUnlockMode[] modes = chrome.quickUnlockPrivate.getAllowedModes() > return modes.includes(QuickUnlockMode.PIN); > > or maybe > > return chrome.quickUnlockPrivate.isAllowed() > > depending on if we treat all quick unlock modes the same w.r.t. policy. Ah I forgot you were doing a private extension API. This query will be fast and synchronous right? (So it won't introduce a UI flicker?) https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:35: if (true || showPin) { leftover true || https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin.html (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin.html:1: <!-- TODO(crbug.com/603217): Use i18n instead of string literals. Figure out I have some reservations - but this is okay with me if the c/b/r/chromeos OWNER is okay with this. https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin.js (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin.js:30: // enter pressed Enter pressed. https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:107: <template is="dom-if" if=[[pinUnlockAllowed_]]> Hmm... it appears that this just embeds the pin-keyboard within the People section? Do you have a link to the mocks for PIN unlock within Settings? If this isn't ready perhaps we can split off the Settings portion of this review into a separate CL
https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/custom_elements_lock.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/custom_elements_lock.js:30: window.addEventListener('DOMContentLoaded', function() { On 2016/05/04 19:29:02, tommycli wrote: > On 2016/05/03 19:21:57, jdufault wrote: > > On 2016/05/02 16:11:26, tommycli wrote: > > > Can we commit the "naive" and obvious approach for the first pass and then > > > address performance concerns after? > > > > We can't load this directly from the HTML file, because then it will be loaded > > when we are not using PIN unlock. We try to keep lock screen initialization > time > > at a bare minimum. > > > > So it ends up that the there isn't an obvious approach (that I know of - any > > ideas?); I think the simplest we can do is to remove the requestIdleCallback > > invocation. I don't think having that adds much complexity, so I'd prefer to > > leave this as-is for now. > > Hmm, this still seems pretty non-obvious. Can we load it directly in the HTML > file and use Polymer's lazy registration to reduce the performance cost? > https://www.polymer-project.org/1.0/docs/release-notes.html#v1-4-0 > > Or alternatively, if you really don't want to pay the cost, maybe load it in an > IFRAME dynamically? I did some investigation here. Using polymer lazy registration is still too-heavy performance wise - loading times goes from 1.3s to 2s, which is a big regression. Even with lazy registration, the PIN element will still have to be inserted into the DOM dynamically otherwise the lazy loading will not work. An iframe approach is promising, but it introduces a fairly large amount of spread-out complexity: - It requires adding some new files, pin_host.html/css/js. - The iframe needs to be conditionally loaded otherwise the PIN will always show up, which means we still need some (simpler) JS in this file - we might be able to work around this in oobe_ui.cc by just not exposing the pin_host.html file. - It requires a CSP whitelist in oobe_ui.cc. - It is styled separately from everything else. The primary problem here is width/height of the PIN element. - pin_host.js has to install a 'submit' event listener on the pin element and forward the event to the parent document. I prefer the current approach simply because it has so much less complexity - for example, I have a feeling that tabbing will require workarounds if we go with the iframe approach. I reworked the current approach a little; hopefully it reads better. Depending on timing and how everything feels at the end of this project, I may do further investigation on using an iframe for performance reasons, as we can start loading the PIN keypad immediately which may make it show up a bit sooner, but it increases the lock-load time from 1.3s to 1.5s. https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:35: if (true || showPin) { On 2016/05/04 19:29:02, tommycli wrote: > leftover true || Done. https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin.html (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin.html:1: <!-- TODO(crbug.com/603217): Use i18n instead of string literals. Figure out On 2016/05/04 19:29:02, tommycli wrote: > I have some reservations - but this is okay with me if the c/b/r/chromeos OWNER > is okay with this. Makes sense. I haven't done this yet because it requires some research and the actual change may get delegated. https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin.js (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin.js:30: // enter pressed On 2016/05/04 19:29:03, tommycli wrote: > Enter pressed. Done. https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:107: <template is="dom-if" if=[[pinUnlockAllowed_]]> On 2016/05/04 19:29:03, tommycli wrote: > Hmm... it appears that this just embeds the pin-keyboard within the People > section? Do you have a link to the mocks for PIN unlock within Settings? > > If this isn't ready perhaps we can split off the Settings portion of this review > into a separate CL UI review mocks are at go/rwftg. My primary intent with this CL is to setup the code-sharing structure, since there are a fair number of changes/new-files needed to make that happen. If you prefer, I can pull the settings changes out of this CL and upload them later once they are more fleshed out. If that ends up happening, I'd like to first confirm that the overall code-layout/sharing structure looks good.
https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/custom_elements_lock.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/custom_elements_lock.js:30: window.addEventListener('DOMContentLoaded', function() { On 2016/05/06 00:05:46, jdufault wrote: > On 2016/05/04 19:29:02, tommycli wrote: > > On 2016/05/03 19:21:57, jdufault wrote: > > > On 2016/05/02 16:11:26, tommycli wrote: > > > > Can we commit the "naive" and obvious approach for the first pass and then > > > > address performance concerns after? > > > > > > We can't load this directly from the HTML file, because then it will be > loaded > > > when we are not using PIN unlock. We try to keep lock screen initialization > > time > > > at a bare minimum. > > > > > > So it ends up that the there isn't an obvious approach (that I know of - any > > > ideas?); I think the simplest we can do is to remove the requestIdleCallback > > > invocation. I don't think having that adds much complexity, so I'd prefer to > > > leave this as-is for now. > > > > Hmm, this still seems pretty non-obvious. Can we load it directly in the HTML > > file and use Polymer's lazy registration to reduce the performance cost? > > https://www.polymer-project.org/1.0/docs/release-notes.html#v1-4-0 > > > > Or alternatively, if you really don't want to pay the cost, maybe load it in > an > > IFRAME dynamically? > > I did some investigation here. > > Using polymer lazy registration is still too-heavy performance wise - loading > times goes from 1.3s to 2s, which is a big regression. Even with lazy > registration, the PIN element will still have to be inserted into the DOM > dynamically otherwise the lazy loading will not work. > > An iframe approach is promising, but it introduces a fairly large amount of > spread-out complexity: > - It requires adding some new files, pin_host.html/css/js. > - The iframe needs to be conditionally loaded otherwise the PIN will always show > up, which means we still need some (simpler) JS in this file - we might be able > to work around this in oobe_ui.cc by just not exposing the pin_host.html file. > - It requires a CSP whitelist in oobe_ui.cc. > - It is styled separately from everything else. The primary problem here is > width/height of the PIN element. > - pin_host.js has to install a 'submit' event listener on the pin element and > forward the event to the parent document. > > I prefer the current approach simply because it has so much less complexity - > for example, I have a feeling that tabbing will require workarounds if we go > with the iframe approach. > > I reworked the current approach a little; hopefully it reads better. > > Depending on timing and how everything feels at the end of this project, I may > do further investigation on using an iframe for performance reasons, as we can > start loading the PIN keypad immediately which may make it show up a bit sooner, > but it increases the lock-load time from 1.3s to 1.5s. I wonder whether we could use existing cr.ui.login.ResourceLoader (xhr based) for the async loading. Currently, the enrollment screen is loaded via that [1]. This might add more work as we might need to change how pin html/js/css a bit and extend ResourceLoader. [1] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
I have some additional comments, but I think we are getting close. I think the only sticking point for me now is the dynamic loading code. Since achuith and xiyuan are the ChromeOS OWNERs, I can defer to them on that topic. As an outside reader though, I still think an IFRAME or some other dynamic loading mechanism would be nicer. https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:107: <template is="dom-if" if=[[pinUnlockAllowed_]]> On 2016/05/06 00:05:46, jdufault wrote: > On 2016/05/04 19:29:03, tommycli wrote: > > Hmm... it appears that this just embeds the pin-keyboard within the People > > section? Do you have a link to the mocks for PIN unlock within Settings? > > > > If this isn't ready perhaps we can split off the Settings portion of this > review > > into a separate CL > > UI review mocks are at go/rwftg. > > My primary intent with this CL is to setup the code-sharing structure, since > there are a fair number of changes/new-files needed to make that happen. > > If you prefer, I can pull the settings changes out of this CL and upload them > later once they are more fleshed out. If that ends up happening, I'd like to > first confirm that the overall code-layout/sharing structure looks good. Okay cool, thanks for the mocks. That helps me a lot. Assuming the code sharing works, since this is hidden (given pinUnlockAllowed_ is false), I'm fine with it. https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin.html (right): https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin.html:10: <dom-module id="pin-keyboard"> Since this is actually the pin-keyboard specifically, I think it would be helpful if the HTML/JS files were named pin-keyboard.html/js https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin.html:11: <link rel="import" type="css" href="pin.css"> External stylesheets have been deprecated in Polymer. See https://www.polymer-project.org/1.0/docs/devguide/styling.html https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/pin.js (right): https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/pin.js:5: <include src="../../chromeos/quick_unlock/pin.js"> This code sharing is just fine with me (assuming it works and you've tested it locally?) Maybe these files should be called pin-keyboard.js in both places.
Description was changed from ========== Add a very basic PIN UI implementation that is shared between lock and settings. Most things are still in flux, but this will help provide some structure for the rest of the code. BUG=603217 ========== to ========== Add a very basic PIN UI implementation that is shared between lock and settings. Most things are still in flux, but this will help provide some structure for the rest of the code. BUG=603217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/custom_elements_lock.js (right): https://codereview.chromium.org/1933913002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/custom_elements_lock.js:30: window.addEventListener('DOMContentLoaded', function() { On 2016/05/06 16:52:34, xiyuan wrote: > On 2016/05/06 00:05:46, jdufault wrote: > > On 2016/05/04 19:29:02, tommycli wrote: > > > On 2016/05/03 19:21:57, jdufault wrote: > > > > On 2016/05/02 16:11:26, tommycli wrote: > > > > > Can we commit the "naive" and obvious approach for the first pass and > then > > > > > address performance concerns after? > > > > > > > > We can't load this directly from the HTML file, because then it will be > > loaded > > > > when we are not using PIN unlock. We try to keep lock screen > initialization > > > time > > > > at a bare minimum. > > > > > > > > So it ends up that the there isn't an obvious approach (that I know of - > any > > > > ideas?); I think the simplest we can do is to remove the > requestIdleCallback > > > > invocation. I don't think having that adds much complexity, so I'd prefer > to > > > > leave this as-is for now. > > > > > > Hmm, this still seems pretty non-obvious. Can we load it directly in the > HTML > > > file and use Polymer's lazy registration to reduce the performance cost? > > > https://www.polymer-project.org/1.0/docs/release-notes.html#v1-4-0 > > > > > > Or alternatively, if you really don't want to pay the cost, maybe load it in > > an > > > IFRAME dynamically? > > > > I did some investigation here. > > > > Using polymer lazy registration is still too-heavy performance wise - loading > > times goes from 1.3s to 2s, which is a big regression. Even with lazy > > registration, the PIN element will still have to be inserted into the DOM > > dynamically otherwise the lazy loading will not work. > > > > An iframe approach is promising, but it introduces a fairly large amount of > > spread-out complexity: > > - It requires adding some new files, pin_host.html/css/js. > > - The iframe needs to be conditionally loaded otherwise the PIN will always > show > > up, which means we still need some (simpler) JS in this file - we might be > able > > to work around this in oobe_ui.cc by just not exposing the pin_host.html file. > > - It requires a CSP whitelist in oobe_ui.cc. > > - It is styled separately from everything else. The primary problem here is > > width/height of the PIN element. > > - pin_host.js has to install a 'submit' event listener on the pin element and > > forward the event to the parent document. > > > > I prefer the current approach simply because it has so much less complexity - > > for example, I have a feeling that tabbing will require workarounds if we go > > with the iframe approach. > > > > I reworked the current approach a little; hopefully it reads better. > > > > Depending on timing and how everything feels at the end of this project, I may > > do further investigation on using an iframe for performance reasons, as we can > > start loading the PIN keypad immediately which may make it show up a bit > sooner, > > but it increases the lock-load time from 1.3s to 1.5s. > > I wonder whether we could use existing cr.ui.login.ResourceLoader (xhr based) > for the async loading. Currently, the enrollment screen is loaded via that [1]. > This might add more work as we might need to change how pin html/js/css a bit > and extend ResourceLoader. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... Done - moving the resource loading code to ResourceLoader makes this code much easier to read. https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/1933913002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:107: <template is="dom-if" if=[[pinUnlockAllowed_]]> On 2016/05/06 18:15:44, tommycli wrote: > On 2016/05/06 00:05:46, jdufault wrote: > > On 2016/05/04 19:29:03, tommycli wrote: > > > Hmm... it appears that this just embeds the pin-keyboard within the People > > > section? Do you have a link to the mocks for PIN unlock within Settings? > > > > > > If this isn't ready perhaps we can split off the Settings portion of this > > review > > > into a separate CL > > > > UI review mocks are at go/rwftg. > > > > My primary intent with this CL is to setup the code-sharing structure, since > > there are a fair number of changes/new-files needed to make that happen. > > > > If you prefer, I can pull the settings changes out of this CL and upload them > > later once they are more fleshed out. If that ends up happening, I'd like to > > first confirm that the overall code-layout/sharing structure looks good. > > Okay cool, thanks for the mocks. That helps me a lot. Assuming the code sharing > works, since this is hidden (given pinUnlockAllowed_ is false), I'm fine with > it. Thanks - the code sharing does indeed work. https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/quick_unlock/pin.html (right): https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin.html:10: <dom-module id="pin-keyboard"> On 2016/05/06 18:15:44, tommycli wrote: > Since this is actually the pin-keyboard specifically, I think it would be > helpful if the HTML/JS files were named pin-keyboard.html/js Done. https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/quick_unlock/pin.html:11: <link rel="import" type="css" href="pin.css"> On 2016/05/06 18:15:44, tommycli wrote: > External stylesheets have been deprecated in Polymer. See > https://www.polymer-project.org/1.0/docs/devguide/styling.html Done. https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/pin.js (right): https://codereview.chromium.org/1933913002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/people_page/pin.js:5: <include src="../../chromeos/quick_unlock/pin.js"> On 2016/05/06 18:15:44, tommycli wrote: > This code sharing is just fine with me (assuming it works and you've tested it > locally?) Maybe these files should be called pin-keyboard.js in both places. Done.
this is lgtm except for the below from my perspective. You will need someone else to look at the ui/ directory changes, and I didn't look too closely. https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:34: if (showPin) { nit: just inline the getBoolean into here https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:120: <div id="googleg-logo-container"> Can you use a plain img tag and position using flex item css rules? https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:200: this.syncBrowserProxy_.openActivityControlsUrl(); This will break the closure compiler since this method doesn't exist on the sync browser proxy, i think.
On 2016/05/06 23:54:18, tommycli wrote: > this is lgtm except for the below from my perspective. You will need someone > else to look at the ui/ directory changes, and I didn't look too closely. > > https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... > File chrome/browser/resources/chromeos/login/lock.js (right): > > https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... > chrome/browser/resources/chromeos/login/lock.js:34: if (showPin) { > nit: just inline the getBoolean into here > > https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... > File chrome/browser/resources/settings/people_page/people_page.html (right): > > https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... > chrome/browser/resources/settings/people_page/people_page.html:120: <div > id="googleg-logo-container"> > Can you use a plain img tag and position using flex item css rules? > > https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... > File chrome/browser/resources/settings/people_page/people_page.js (right): > > https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... > chrome/browser/resources/settings/people_page/people_page.js:200: > this.syncBrowserProxy_.openActivityControlsUrl(); > This will break the closure compiler since this method doesn't exist on the sync > browser proxy, i think. Thanks for sticking with it :)
https://codereview.chromium.org/1933913002/diff/60001/ui/login/resource_loade... File ui/login/resource_loader.js (right): https://codereview.chromium.org/1933913002/diff/60001/ui/login/resource_loade... ui/login/resource_loader.js:195: function loadAssetsOnIdle(id, callback, idleTimeoutMs = 250) { idleTimeoutMs -> opt_IdleTimeoutMs since it is an optional argument Try to refrain from ES6 features since we have not decided on whether to use them in chromium code. https://codereview.chromium.org/1933913002/diff/60001/ui/login/resource_loade... ui/login/resource_loader.js:205: else { nit: move this to line 204, i.e. } else {
We really ought to have compiled_resources2 files for all of this, but specifically the new pin_keyboard code. https://codereview.chromium.org/1933913002/diff/60001/ui/login/resource_loade... File ui/login/resource_loader.js (right): https://codereview.chromium.org/1933913002/diff/60001/ui/login/resource_loade... ui/login/resource_loader.js:195: function loadAssetsOnIdle(id, callback, idleTimeoutMs = 250) { On 2016/05/09 18:27:38, xiyuan wrote: > idleTimeoutMs -> opt_IdleTimeoutMs since it is an optional argument > > Try to refrain from ES6 features since we have not decided on whether to use > them in chromium code. We have been using ES6 is settings UI since it will only ever run in Chrome and Chrome does officially support it now. +1 on opt_idleTimeoutMs
https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/lock.js (right): https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/lock.js:34: if (showPin) { On 2016/05/06 23:54:18, tommycli wrote: > nit: just inline the getBoolean into here Done. https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.html (right): https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.html:120: <div id="googleg-logo-container"> On 2016/05/06 23:54:18, tommycli wrote: > Can you use a plain img tag and position using flex item css rules? Sorry, this is a rebase change. I accidentally forgot to separate the rebase patch. https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/1933913002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:200: this.syncBrowserProxy_.openActivityControlsUrl(); On 2016/05/06 23:54:18, tommycli wrote: > This will break the closure compiler since this method doesn't exist on the sync > browser proxy, i think. Rebase change. https://codereview.chromium.org/1933913002/diff/60001/ui/login/resource_loade... File ui/login/resource_loader.js (right): https://codereview.chromium.org/1933913002/diff/60001/ui/login/resource_loade... ui/login/resource_loader.js:195: function loadAssetsOnIdle(id, callback, idleTimeoutMs = 250) { On 2016/05/09 19:10:26, stevenjb wrote: > On 2016/05/09 18:27:38, xiyuan wrote: > > idleTimeoutMs -> opt_IdleTimeoutMs since it is an optional argument > > > > Try to refrain from ES6 features since we have not decided on whether to use > > them in chromium code. > > We have been using ES6 is settings UI since it will only ever run in Chrome and > Chrome does officially support it now. > +1 on opt_idleTimeoutMs Done. https://codereview.chromium.org/1933913002/diff/60001/ui/login/resource_loade... ui/login/resource_loader.js:205: else { On 2016/05/09 18:27:38, xiyuan wrote: > nit: move this to line 204, i.e. } else { Done.
Thanks for updating compiled_resources.gyp. LGTM.
lgtm
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org Link to the patchset: https://codereview.chromium.org/1933913002/#ps80001 (title: "Nits and fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1933913002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1933913002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, stevenjb@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/1933913002/#ps100001 (title: "Fix tests by removing a previously unused import that started getting used b/c of resource loader c…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1933913002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1933913002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add a very basic PIN UI implementation that is shared between lock and settings. Most things are still in flux, but this will help provide some structure for the rest of the code. BUG=603217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Add a very basic PIN UI implementation that is shared between lock and settings. Most things are still in flux, but this will help provide some structure for the rest of the code. BUG=603217 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/22dfa22df0ddbcab9f47d189207a11bdea820d75 Cr-Commit-Position: refs/heads/master@{#393081} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/22dfa22df0ddbcab9f47d189207a11bdea820d75 Cr-Commit-Position: refs/heads/master@{#393081} |
