Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2600983003:
ChromeOS MD-OOBE: Add HID detection screen. (Closed)
|
Created:
3 years, 12 months ago by Alexander Alekseev Modified:
3 years, 11 months ago Reviewers:
xiyuan CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, arv+watch_chromium.org, stevenjb, The one and only Dr. Crash Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeOS MD-OOBE: Add HID detection screen.
This CL implements HID detection screen for material design ChromeOS OOBE.
BUG=604119
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2600983003
Cr-Commit-Position: refs/heads/master@{#443023}
Committed: https://chromium.googlesource.com/chromium/src/+/9e81b5fefe756225638bb71dfb097fa11d2a8f8c
Patch Set 1 #Patch Set 2 : Rebased. #Patch Set 3 : Fix build #
Total comments: 39
Patch Set 4 : Update after review. #Patch Set 5 : Hide two versions of the screen behind dom-if template. #
Total comments: 9
Patch Set 6 : Revert to #4 #
Total comments: 14
Patch Set 7 : Update after review #Patch Set 8 : Update after review. #Patch Set 9 : Rebased. #Messages
Total messages: 56 (35 generated)
Description was changed from ========== ChromeOS MD-OOBE: Add HID detection screen. This CL implements HID detection screen for material design ChromeOS OOBE. BUG=604119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== ChromeOS MD-OOBE: Add HID detection screen. This CL implements HID detection screen for material design ChromeOS OOBE. BUG=604119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== ChromeOS MD-OOBE: Add HID detection screen. This CL implements HID detection screen for material design ChromeOS OOBE. BUG=604119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== ChromeOS MD-OOBE: Add HID detection screen. This CL implements HID detection screen for material design ChromeOS OOBE. BUG=604119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by alemate@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...
alemate@chromium.org changed reviewers: + michaelpg@chromium.org
Please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 alemate@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 alemate@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...
alemate@chromium.org changed reviewers: + stevenjb@chromium.org - michaelpg@chromium.org
Michael is OOO, Steven, please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:611: // TODO(drcrash): Remove this after testing (http://crbug.com/647411). Remove TODO? https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_hid_detection.css (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.css:7: } nit: Group # selectors together https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.css:34: /*visibility: hidden;*/ remove https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_hid_detection.html (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:3: found in the LICENSE file. --> No Copyright in .html https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:25: <iron-icon icon="oobe-hid-detection:bluetooth" class="oobe-icon"></iron-icon> Use multiple lines instead of wrapping https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:32: <div class="layout horizontal center"> Can we combine these divs? https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:90: </div> Fix wrapping on all of these https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:99: <oobe-text-button id="hid-continue-button" inverse on-tap="onHIDContinue_" nit: By convention we would name the function 'onHIDContinueTap_' Also: Fix wrapping https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_hid_detection.js (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:11: var PINCODE_LENGTH = 6; I don't think the { does anything, i.e. I don't think that actually scopes PINCODE_LENGTH. In Settings we would do the following: (function() { /** @const {number} */ var PINCODE_LENGTH = 6; Polymer({ ... }); })(); https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:19: */ nit: All of these comments that fit can be a single line: /** "Continue" button is disabled until HID devices are paired. */ https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:26: * This is the displaed text for keyboard "Pairing" state. displayed https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:31: * This is the displaed text for keyboard "Paired" state. displayed https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:38: mouseState: String, This appears to be internal? If so, append _ to the name and add @private to the comment. Same with any other private properties (i.e. only used in this html/js, not set by other html or js files). https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:66: calculateState_: function(state, newState) { Add @param for state, newState. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:80: tickIsVisible_: function(state) { @param for all functions https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:115: ')'); Remove debugging logs. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:133: for (var i = 0; i < (PINCODE_LENGTH + 1); i++) { nit: in cros specific UI we can use 'let' which scopes the variable: for (let i = 0; ...) https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:4: </oobe-hid-detection-md> It would be much more efficient, and more readable, to use dom-if here: <template is="dom-if" if="[[useMDOobe_]]"> <oobe-hid-detection-md id="oobe-hid-detection-md"> </oobe-hid-detection-md> </template> <template is="dom-if" if="[[!useMDOobe_]]"> <div class="step-contents"> ... </template> https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:35: BLOCK: {MOUSE: 'hid-mouse-block', KEYBOARD: 'hid-keyboard-block'}, nit: If you put a ',' at the end of the last enum entry, clang will use multiple lines which is more readable imho.
https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/login/wizard_controller.cc (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/login/wizard_controller.cc:611: // TODO(drcrash): Remove this after testing (http://crbug.com/647411). On 2016/12/28 20:47:03, stevenjb wrote: > Remove TODO? Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_hid_detection.css (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.css:7: } On 2016/12/28 20:47:04, stevenjb wrote: > nit: Group # selectors together Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.css:34: /*visibility: hidden;*/ On 2016/12/28 20:47:03, stevenjb wrote: > remove Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_hid_detection.html (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:3: found in the LICENSE file. --> On 2016/12/28 20:47:04, stevenjb wrote: > No Copyright in .html Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:25: <iron-icon icon="oobe-hid-detection:bluetooth" class="oobe-icon"></iron-icon> On 2016/12/28 20:47:04, stevenjb wrote: > Use multiple lines instead of wrapping Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:32: <div class="layout horizontal center"> On 2016/12/28 20:47:04, stevenjb wrote: > Can we combine these divs? This inner div helps positioning of "#mouse-tick" and "#keyboard-tick" with the same style. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:90: </div> On 2016/12/28 20:47:04, stevenjb wrote: > Fix wrapping on all of these Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.html:99: <oobe-text-button id="hid-continue-button" inverse on-tap="onHIDContinue_" On 2016/12/28 20:47:04, stevenjb wrote: > nit: By convention we would name the function 'onHIDContinueTap_' > > Also: Fix wrapping Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_hid_detection.js (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:11: var PINCODE_LENGTH = 6; On 2016/12/28 20:47:04, stevenjb wrote: > I don't think the { does anything, i.e. I don't think that actually scopes > PINCODE_LENGTH. > > In Settings we would do the following: > > (function() { > /** @const {number} */ var PINCODE_LENGTH = 6; > > Polymer({ > ... > }); > })(); > > > > > Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:19: */ On 2016/12/28 20:47:04, stevenjb wrote: > nit: All of these comments that fit can be a single line: > > /** "Continue" button is disabled until HID devices are paired. */ Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:26: * This is the displaed text for keyboard "Pairing" state. On 2016/12/28 20:47:04, stevenjb wrote: > displayed Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:31: * This is the displaed text for keyboard "Paired" state. On 2016/12/28 20:47:04, stevenjb wrote: > displayed Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:38: mouseState: String, On 2016/12/28 20:47:04, stevenjb wrote: > This appears to be internal? If so, append _ to the name and add @private to the > comment. Same with any other private properties (i.e. only used in this html/js, > not set by other html or js files). > Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:66: calculateState_: function(state, newState) { On 2016/12/28 20:47:04, stevenjb wrote: > Add @param for state, newState. Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:80: tickIsVisible_: function(state) { On 2016/12/28 20:47:04, stevenjb wrote: > @param for all functions Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:115: ')'); On 2016/12/28 20:47:04, stevenjb wrote: > Remove debugging logs. Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:133: for (var i = 0; i < (PINCODE_LENGTH + 1); i++) { On 2016/12/28 20:47:04, stevenjb wrote: > nit: in cros specific UI we can use 'let' which scopes the variable: > for (let i = 0; ...) Done. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:4: </oobe-hid-detection-md> On 2016/12/28 20:47:04, stevenjb wrote: > It would be much more efficient, and more readable, to use dom-if here: > > <template is="dom-if" if="[[useMDOobe_]]"> > <oobe-hid-detection-md id="oobe-hid-detection-md"> > </oobe-hid-detection-md> > </template> > <template is="dom-if" if="[[!useMDOobe_]]"> > <div class="step-contents"> > ... > </template> dom-if conflicts with i18n-content="hidDetectionInvitation" attributes. https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:35: BLOCK: {MOUSE: 'hid-mouse-block', KEYBOARD: 'hid-keyboard-block'}, On 2016/12/28 20:47:04, stevenjb wrote: > nit: If you put a ',' at the end of the last enum entry, clang will use multiple > lines which is more readable imho. Done.
The CQ bit was checked by alemate@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/2600983003/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:4: </oobe-hid-detection-md> On 2016/12/28 22:56:23, Alexander Alekseev wrote: > On 2016/12/28 20:47:04, stevenjb wrote: > > It would be much more efficient, and more readable, to use dom-if here: > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > </oobe-hid-detection-md> > > </template> > > <template is="dom-if" if="[[!useMDOobe_]]"> > > <div class="step-contents"> > > ... > > </template> > > dom-if conflicts with i18n-content="hidDetectionInvitation" attributes. Ugh. We should really switch login to use the $18n{} macro we are using in Settings. In the meanwhile, can we move the body of this to oobe_hid_detection.html and include either the old body or the new? i.e. <template is="dom-if" if="[[useMDOobe_]]"> <oobe-hid-detection-md id="oobe-hid-detection-md"> </oobe-hid-detection-md> </template> <template is="dom-if" if="[[!useMDOobe_]]"> <oobe-hid-detection> </oobe-hid-detection> </template> The way this is currently set up, in addition to being twice as slow to load, is very confusing. I assume that we want to get rid of the non OOBE version "soon", but we both know how long "soon" can be.
On 2016/12/29 00:32:19, stevenjb wrote: > https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html > (right): > > https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... > chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:4: > </oobe-hid-detection-md> > On 2016/12/28 22:56:23, Alexander Alekseev wrote: > > On 2016/12/28 20:47:04, stevenjb wrote: > > > It would be much more efficient, and more readable, to use dom-if here: > > > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > > </oobe-hid-detection-md> > > > </template> > > > <template is="dom-if" if="[[!useMDOobe_]]"> > > > <div class="step-contents"> > > > ... > > > </template> > > > > dom-if conflicts with i18n-content="hidDetectionInvitation" attributes. > > Ugh. We should really switch login to use the $18n{} macro we are using in > Settings. > > In the meanwhile, can we move the body of this to oobe_hid_detection.html and > include either the old body or the new? i.e. > > <template is="dom-if" if="[[useMDOobe_]]"> > <oobe-hid-detection-md id="oobe-hid-detection-md"> > </oobe-hid-detection-md> > </template> > <template is="dom-if" if="[[!useMDOobe_]]"> > <oobe-hid-detection> > </oobe-hid-detection> > </template> > > > The way this is currently set up, in addition to being twice as slow to load, is > very confusing. I assume that we want to get rid of the non OOBE version "soon", > but we both know how long "soon" can be. Alternatively, create oobe_screen_hid_detection_md.html that is a drop-in replacement for oobe_screen_hid_detection.html and leave the non-md version as-is.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/29 00:36:13, stevenjb wrote: > On 2016/12/29 00:32:19, stevenjb wrote: > > > https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... > > File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html > > (right): > > > > > https://codereview.chromium.org/2600983003/diff/40001/chrome/browser/resource... > > chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:4: > > </oobe-hid-detection-md> > > On 2016/12/28 22:56:23, Alexander Alekseev wrote: > > > On 2016/12/28 20:47:04, stevenjb wrote: > > > > It would be much more efficient, and more readable, to use dom-if here: > > > > > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > > > </oobe-hid-detection-md> > > > > </template> > > > > <template is="dom-if" if="[[!useMDOobe_]]"> > > > > <div class="step-contents"> > > > > ... > > > > </template> > > > > > > dom-if conflicts with i18n-content="hidDetectionInvitation" attributes. > > > > Ugh. We should really switch login to use the $18n{} macro we are using in > > Settings. > > > > In the meanwhile, can we move the body of this to oobe_hid_detection.html and > > include either the old body or the new? i.e. > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > </oobe-hid-detection-md> > > </template> > > <template is="dom-if" if="[[!useMDOobe_]]"> > > <oobe-hid-detection> > > </oobe-hid-detection> > > </template> > > > > > > The way this is currently set up, in addition to being twice as slow to load, > is > > very confusing. I assume that we want to get rid of the non OOBE version > "soon", > > but we both know how long "soon" can be. > > Alternatively, create oobe_screen_hid_detection_md.html that is a drop-in > replacement for oobe_screen_hid_detection.html and leave the non-md version > as-is. But in the meanwhile we still want oobe_hid_detection.js (i.e. JS bindings for NON-MD version) work mostly unmodified. And this means that we still want selectors like ***this.$['hid-keyboard-pincode-sym-' + (i + 1)]; *** to continue returning non-empty results.
I'm not sure I followed that, but any solution that allows us to only load either the MD or the non-MD version of the page would be fine. In particular, having one version embedded and the other not is very confusing. On Tue, Jan 3, 2017 at 5:18 PM, <alemate@chromium.org> wrote: > On 2016/12/29 00:36:13, stevenjb wrote: > > On 2016/12/29 00:32:19, stevenjb wrote: > > > > > > https://codereview.chromium.org/2600983003/diff/40001/ > chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html > > > File chrome/browser/resources/chromeos/login/oobe_screen_ > hid_detection.html > > > (right): > > > > > > > > > https://codereview.chromium.org/2600983003/diff/40001/ > chrome/browser/resources/chromeos/login/oobe_screen_ > hid_detection.html#newcode4 > > > chrome/browser/resources/chromeos/login/oobe_screen_ > hid_detection.html:4: > > > </oobe-hid-detection-md> > > > On 2016/12/28 22:56:23, Alexander Alekseev wrote: > > > > On 2016/12/28 20:47:04, stevenjb wrote: > > > > > It would be much more efficient, and more readable, to use dom-if > here: > > > > > > > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > > > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > > > > </oobe-hid-detection-md> > > > > > </template> > > > > > <template is="dom-if" if="[[!useMDOobe_]]"> > > > > > <div class="step-contents"> > > > > > ... > > > > > </template> > > > > > > > > dom-if conflicts with i18n-content="hidDetectionInvitation" > attributes. > > > > > > Ugh. We should really switch login to use the $18n{} macro we are > using in > > > Settings. > > > > > > In the meanwhile, can we move the body of this to > oobe_hid_detection.html > and > > > include either the old body or the new? i.e. > > > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > > </oobe-hid-detection-md> > > > </template> > > > <template is="dom-if" if="[[!useMDOobe_]]"> > > > <oobe-hid-detection> > > > </oobe-hid-detection> > > > </template> > > > > > > > > > The way this is currently set up, in addition to being twice as slow to > load, > > is > > > very confusing. I assume that we want to get rid of the non OOBE > version > > "soon", > > > but we both know how long "soon" can be. > > > > Alternatively, create oobe_screen_hid_detection_md.html that is a > drop-in > > replacement for oobe_screen_hid_detection.html and leave the non-md > version > > as-is. > > But in the meanwhile we still want oobe_hid_detection.js (i.e. JS bindings > for > NON-MD version) work mostly unmodified. And this means that we still want > selectors like ***this.$['hid-keyboard-pincode-sym-' + (i + 1)]; *** to > continue > returning non-empty results. > > > https://codereview.chromium.org/2600983003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/01/04 01:50:42, stevenjb wrote: > I'm not sure I followed that, but any solution that allows us to only load > either the MD or the non-MD version of the page would be fine. In > particular, having one version embedded and the other not is very confusing. I implemented what you suggested. Due to many asynchronous events, it became way more complicate. But it still doesn't work, because updateScreenSize() from display_manager.js requires screen content to exist, otherwise screen size will be 0 and incorrectly positioned. Personally I prefer Patch Set 4, which is way less complex and "just works" ;) > > > On Tue, Jan 3, 2017 at 5:18 PM, <mailto:alemate@chromium.org> wrote: > > > On 2016/12/29 00:36:13, stevenjb wrote: > > > On 2016/12/29 00:32:19, stevenjb wrote: > > > > > > > > > https://codereview.chromium.org/2600983003/diff/40001/ > > chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html > > > > File chrome/browser/resources/chromeos/login/oobe_screen_ > > hid_detection.html > > > > (right): > > > > > > > > > > > > > https://codereview.chromium.org/2600983003/diff/40001/ > > chrome/browser/resources/chromeos/login/oobe_screen_ > > hid_detection.html#newcode4 > > > > chrome/browser/resources/chromeos/login/oobe_screen_ > > hid_detection.html:4: > > > > </oobe-hid-detection-md> > > > > On 2016/12/28 22:56:23, Alexander Alekseev wrote: > > > > > On 2016/12/28 20:47:04, stevenjb wrote: > > > > > > It would be much more efficient, and more readable, to use dom-if > > here: > > > > > > > > > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > > > > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > > > > > </oobe-hid-detection-md> > > > > > > </template> > > > > > > <template is="dom-if" if="[[!useMDOobe_]]"> > > > > > > <div class="step-contents"> > > > > > > ... > > > > > > </template> > > > > > > > > > > dom-if conflicts with i18n-content="hidDetectionInvitation" > > attributes. > > > > > > > > Ugh. We should really switch login to use the $18n{} macro we are > > using in > > > > Settings. > > > > > > > > In the meanwhile, can we move the body of this to > > oobe_hid_detection.html > > and > > > > include either the old body or the new? i.e. > > > > > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > > > </oobe-hid-detection-md> > > > > </template> > > > > <template is="dom-if" if="[[!useMDOobe_]]"> > > > > <oobe-hid-detection> > > > > </oobe-hid-detection> > > > > </template> > > > > > > > > > > > > The way this is currently set up, in addition to being twice as slow to > > load, > > > is > > > > very confusing. I assume that we want to get rid of the non OOBE > > version > > > "soon", > > > > but we both know how long "soon" can be. > > > > > > Alternatively, create oobe_screen_hid_detection_md.html that is a > > drop-in > > > replacement for oobe_screen_hid_detection.html and leave the non-md > > version > > > as-is. > > > > But in the meanwhile we still want oobe_hid_detection.js (i.e. JS bindings > > for > > NON-MD version) work mostly unmodified. And this means that we still want > > selectors like ***this.$['hid-keyboard-pincode-sym-' + (i + 1)]; *** to > > continue > > returning non-empty results. > > > > > > https://codereview.chromium.org/2600983003/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by alemate@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.
Maybe you can have Achuith look at it? We really need someone else working on login / oobe that can review more complex changes like this. I have too many other things going to look at this more closely right now. Maybe I can make time later this week. I don't understand what this has to do with display_manager.js (which I am not at all familiar with). I realize that there are challenges related to asynchronous loading, and that the structure of the login UI may not play nicely with mixing and matching the old UI and new UI. I don't know what the best solution for that is. Maybe we can't use dom-if, I just want to make sure we are aware of the cost and have tried and considered other approaches. If PS5 doesn't work correctly then there doesn't seem to be much point in reviewing it. On Tue, Jan 3, 2017 at 8:57 PM, <alemate@chromium.org> wrote: > On 2017/01/04 01:50:42, stevenjb wrote: > > I'm not sure I followed that, but any solution that allows us to only > load > > either the MD or the non-MD version of the page would be fine. In > > particular, having one version embedded and the other not is very > confusing. > > I implemented what you suggested. Due to many asynchronous events, it > became way > more complicate. > > But it still doesn't work, because updateScreenSize() from > display_manager.js > requires screen content to exist, otherwise screen size will be 0 and > incorrectly positioned. > > > Personally I prefer Patch Set 4, which is way less complex and "just > works" ;) > > > > > > > > > On Tue, Jan 3, 2017 at 5:18 PM, <mailto:alemate@chromium.org> wrote: > > > > > On 2016/12/29 00:36:13, stevenjb wrote: > > > > On 2016/12/29 00:32:19, stevenjb wrote: > > > > > > > > > > > > https://codereview.chromium.org/2600983003/diff/40001/ > > > chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html > > > > > File chrome/browser/resources/chromeos/login/oobe_screen_ > > > hid_detection.html > > > > > (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2600983003/diff/40001/ > > > chrome/browser/resources/chromeos/login/oobe_screen_ > > > hid_detection.html#newcode4 > > > > > chrome/browser/resources/chromeos/login/oobe_screen_ > > > hid_detection.html:4: > > > > > </oobe-hid-detection-md> > > > > > On 2016/12/28 22:56:23, Alexander Alekseev wrote: > > > > > > On 2016/12/28 20:47:04, stevenjb wrote: > > > > > > > It would be much more efficient, and more readable, to use > dom-if > > > here: > > > > > > > > > > > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > > > > > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > > > > > > </oobe-hid-detection-md> > > > > > > > </template> > > > > > > > <template is="dom-if" if="[[!useMDOobe_]]"> > > > > > > > <div class="step-contents"> > > > > > > > ... > > > > > > > </template> > > > > > > > > > > > > dom-if conflicts with i18n-content="hidDetectionInvitation" > > > attributes. > > > > > > > > > > Ugh. We should really switch login to use the $18n{} macro we are > > > using in > > > > > Settings. > > > > > > > > > > In the meanwhile, can we move the body of this to > > > oobe_hid_detection.html > > > and > > > > > include either the old body or the new? i.e. > > > > > > > > > > <template is="dom-if" if="[[useMDOobe_]]"> > > > > > <oobe-hid-detection-md id="oobe-hid-detection-md"> > > > > > </oobe-hid-detection-md> > > > > > </template> > > > > > <template is="dom-if" if="[[!useMDOobe_]]"> > > > > > <oobe-hid-detection> > > > > > </oobe-hid-detection> > > > > > </template> > > > > > > > > > > > > > > > The way this is currently set up, in addition to being twice as > slow to > > > load, > > > > is > > > > > very confusing. I assume that we want to get rid of the non OOBE > > > version > > > > "soon", > > > > > but we both know how long "soon" can be. > > > > > > > > Alternatively, create oobe_screen_hid_detection_md.html that is a > > > drop-in > > > > replacement for oobe_screen_hid_detection.html and leave the non-md > > > version > > > > as-is. > > > > > > But in the meanwhile we still want oobe_hid_detection.js (i.e. JS > bindings > > > for > > > NON-MD version) work mostly unmodified. And this means that we still > want > > > selectors like ***this.$['hid-keyboard-pincode-sym-' + (i + 1)]; *** > to > > > continue > > > returning non-empty results. > > > > > > > > > https://codereview.chromium.org/2600983003/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2600983003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
alemate@chromium.org changed reviewers: + xiyuan@chromium.org - stevenjb@chromium.org
-stevenjb +xiyuan Please review.
Using dom-if would complicate things since the existing code does not play well with it. display_manager.js calculates the current screen dimension and explicitly set it on the screen div. dom-if might affect this since it could show/hide the contained elements in the screen div. Steven has a valid point though. We should figure out a way to improve performance and have cleaner code. But that needs more thinking/design and is probably out of scope of this CL. If there is no easy way to make dom-if work, let's go back to PS4. https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_hid_detection.js (right): https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:62: * screen.CONNECTION). nit: 2 more space indent https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_hid_detection.js:85: return this.screen && ( nit: no need to test this.screen since the "if" above already did that. https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:88: oldPaired.textContent = label; nit: wrong indent https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:60: core_oobe_actor_->ReloadContent(localized_strings); Why do we need to do this again? Wouldn't 'newOobeUI' be set in OobeUI's ctor?
I reverted Cl to #4. https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:60: core_oobe_actor_->ReloadContent(localized_strings); On 2017/01/04 21:02:23, xiyuan wrote: > Why do we need to do this again? Wouldn't 'newOobeUI' be set in OobeUI's ctor? This was an attempt to create DOM objects. It actually doesn't work, so I added checks for existence of dom-objects everywhere.
The CQ bit was checked by alemate@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.
+drcrash fyi https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:60: core_oobe_actor_->ReloadContent(localized_strings); On 2017/01/04 21:14:19, Alexander Alekseev wrote: > On 2017/01/04 21:02:23, xiyuan wrote: > > Why do we need to do this again? Wouldn't 'newOobeUI' be set in OobeUI's ctor? > > This was an attempt to create DOM objects. It actually doesn't work, so I added > checks for existence of dom-objects everywhere. Can you clarify? What does not work? What happens if we remove this? MD hid screen would not show up? https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/wizard_controller.cc (left): https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/wizard_controller.cc:612: SetShowMdOobe(false); // Disable the MD OOBE from there on. We should check with drcrash@ to make sure this does not break his zero touch enrollment work. He had problem before with md turned on. https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.css (right): https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.css:11: #oobe[md-mode] #hid-detection { Why not get rid of this and update line 5 to something like: #oobe:not([md-mode]) #hid-detection { ... } https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:14: <img id="hid-mouse-tick" src="chrome://theme/IDR_BLUETOOTH_PAIRING_TICK" nit: wrap https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:34: <img id="hid-keyboard-icon" src="chrome://theme/IDR_BLUETOOTH_KEYBOARD" nit: wrap https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:51: <div id="hid-keyboard-pincode-sym-1" class="bluetooth-keyboard-button"> nit: wrap here and similar lines below https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:162: // whether the functionality of getting num of entered keys is available. nit: whether -> Whether https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:196: /* nit: use ** for js doc and align properly /** * */
https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:60: core_oobe_actor_->ReloadContent(localized_strings); On 2017/01/05 00:09:06, xiyuan wrote: > On 2017/01/04 21:14:19, Alexander Alekseev wrote: > > On 2017/01/04 21:02:23, xiyuan wrote: > > > Why do we need to do this again? Wouldn't 'newOobeUI' be set in OobeUI's > ctor? > > > > This was an attempt to create DOM objects. It actually doesn't work, so I > added > > checks for existence of dom-objects everywhere. > > Can you clarify? What does not work? What happens if we remove this? MD hid > screen would not show up? Sorry, my comment was about a different code snippet. This code is correct. useMdOobe flag is set in strings. Now it is false by default, but after OOBE screens are initialized, it might be set to true. When OOBE "network" screen is displayed, it has language selection code, so strings are reloaded when it is displayed. But "HID detection" screen can be shown before "network" screen, so all strings are in their "initial" values and "useMDOobe" flag is false. So this code updates strings after all screens were initialized. https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/wizard_controller.cc (left): https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/wizard_controller.cc:612: SetShowMdOobe(false); // Disable the MD OOBE from there on. On 2017/01/05 00:09:06, xiyuan wrote: > We should check with drcrash@ to make sure this does not break his zero touch > enrollment work. He had problem before with md turned on. http://crbug.com/647411 is saying about missing HID detection screen which is added in this CL. https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.css (right): https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.css:11: #oobe[md-mode] #hid-detection { On 2017/01/05 00:09:06, xiyuan wrote: > Why not get rid of this and update line 5 to something like: > > #oobe:not([md-mode]) #hid-detection { > ... > } Done. https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html (right): https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:14: <img id="hid-mouse-tick" src="chrome://theme/IDR_BLUETOOTH_PAIRING_TICK" On 2017/01/05 00:09:06, xiyuan wrote: > nit: wrap Done. https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:34: <img id="hid-keyboard-icon" src="chrome://theme/IDR_BLUETOOTH_KEYBOARD" On 2017/01/05 00:09:06, xiyuan wrote: > nit: wrap Done. https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.html:51: <div id="hid-keyboard-pincode-sym-1" class="bluetooth-keyboard-button"> On 2017/01/05 00:09:06, xiyuan wrote: > nit: wrap here and similar lines below Done. https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js (right): https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:162: // whether the functionality of getting num of entered keys is available. On 2017/01/05 00:09:06, xiyuan wrote: > nit: whether -> Whether Done. https://codereview.chromium.org/2600983003/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_hid_detection.js:196: /* On 2017/01/05 00:09:06, xiyuan wrote: > nit: use ** for js doc and align properly > > /** > * > */ Done.
https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:60: core_oobe_actor_->ReloadContent(localized_strings); On 2017/01/05 02:01:16, Alexander Alekseev wrote: > On 2017/01/05 00:09:06, xiyuan wrote: > > On 2017/01/04 21:14:19, Alexander Alekseev wrote: > > > On 2017/01/04 21:02:23, xiyuan wrote: > > > > Why do we need to do this again? Wouldn't 'newOobeUI' be set in OobeUI's > > ctor? > > > > > > This was an attempt to create DOM objects. It actually doesn't work, so I > > added > > > checks for existence of dom-objects everywhere. > > > > Can you clarify? What does not work? What happens if we remove this? MD hid > > screen would not show up? > > Sorry, my comment was about a different code snippet. > > This code is correct. useMdOobe flag is set in strings. > Now it is false by default, but after OOBE screens are initialized, it might be > set to true. > > When OOBE "network" screen is displayed, it has language selection code, so > strings are reloaded when it is displayed. > > But "HID detection" screen can be shown before "network" screen, so all strings > are in their "initial" values and "useMDOobe" flag is false. So this code > updates strings after all screens were initialized. emm, this feels hacky. Imaging we have another screen precedes network screen that needs to have a MD version (e.g. WrongHWIDScreen, or ), then we need to do the same hack there too. We should probably move the ReloadContent call into WizardController::SetShowMdOobe and call it when the pref value changes. Or have a better way to pass useMDOobe flag instead of using loadTimeData.
PTAL. https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc (right): https://codereview.chromium.org/2600983003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:60: core_oobe_actor_->ReloadContent(localized_strings); On 2017/01/05 17:25:07, xiyuan wrote: > On 2017/01/05 02:01:16, Alexander Alekseev wrote: > > On 2017/01/05 00:09:06, xiyuan wrote: > > > On 2017/01/04 21:14:19, Alexander Alekseev wrote: > > > > On 2017/01/04 21:02:23, xiyuan wrote: > > > > > Why do we need to do this again? Wouldn't 'newOobeUI' be set in OobeUI's > > > ctor? > > > > > > > > This was an attempt to create DOM objects. It actually doesn't work, so I > > > added > > > > checks for existence of dom-objects everywhere. > > > > > > Can you clarify? What does not work? What happens if we remove this? MD hid > > > screen would not show up? > > > > Sorry, my comment was about a different code snippet. > > > > This code is correct. useMdOobe flag is set in strings. > > Now it is false by default, but after OOBE screens are initialized, it might > be > > set to true. > > > > When OOBE "network" screen is displayed, it has language selection code, so > > strings are reloaded when it is displayed. > > > > But "HID detection" screen can be shown before "network" screen, so all > strings > > are in their "initial" values and "useMDOobe" flag is false. So this code > > updates strings after all screens were initialized. > > emm, this feels hacky. > > Imaging we have another screen precedes network screen that needs to have a MD > version (e.g. WrongHWIDScreen, or ), then we need to do the same hack there too. > > We should probably move the ReloadContent call into > WizardController::SetShowMdOobe and call it when the pref value changes. Or have > a better way to pass useMDOobe flag instead of using loadTimeData. I implemented UpdateLocalizedStringsIfNeeded().
The CQ bit was checked by alemate@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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
lgtm Thank you for making the changes.
Thank you!
The CQ bit was checked by alemate@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/2600983003/#ps160001 (title: "Rebased.")
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": 160001, "attempt_start_ts": 1484168226247720,
"parent_rev": "159b984703758efd4aac5f9ebd988ff20498e54b", "commit_rev":
"9e81b5fefe756225638bb71dfb097fa11d2a8f8c"}
Message was sent while issue was closed.
Description was changed from ========== ChromeOS MD-OOBE: Add HID detection screen. This CL implements HID detection screen for material design ChromeOS OOBE. BUG=604119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== ChromeOS MD-OOBE: Add HID detection screen. This CL implements HID detection screen for material design ChromeOS OOBE. BUG=604119 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2600983003 Cr-Commit-Position: refs/heads/master@{#443023} Committed: https://chromium.googlesource.com/chromium/src/+/9e81b5fefe756225638bb71dfb09... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/9e81b5fefe756225638bb71dfb09... |
