Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2189733006:
ChromeOS: Implement Language Selection screen of material design OOBE. (Closed)
|
Created:
4 years, 4 months ago by Alexander Alekseev Modified:
4 years, 4 months ago CC:
chromium-reviews, dcheng, dzhioev+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeOS: Implement Language Selection screen of material design OOBE.
This CL adds Language Selection screen to material design OOBE.
BUG=604119
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/f5174e6456f308a7af62106850315c59c255c8e5
Cr-Commit-Position: refs/heads/master@{#410255}
Patch Set 1 #Patch Set 2 : Use translatable string for OK button text #
Total comments: 56
Patch Set 3 : Update after review. #
Total comments: 2
Patch Set 4 : Update after review #
Total comments: 2
Patch Set 5 : Update after review. #
Total comments: 40
Patch Set 6 : Update after review. #
Total comments: 7
Patch Set 7 : Update after review. Fix tests. #Patch Set 8 : Update after review. #
Total comments: 6
Messages
Total messages: 47 (24 generated)
Description was changed from ========== ChromeOS: Implement Language Selection screen of material design OOBE. This CL adds Language Selection screen to material design OOBE. BUG=604119 TEST=none ========== to ========== ChromeOS: Implement Language Selection screen of material design OOBE. This CL adds Language Selection screen to material design OOBE. BUG=604119 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
alemate@chromium.org changed reviewers: + stevenjb@chromium.org
Please review.
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
+michaelpg@ as Steven may be busy. Please, review.
https://codereview.chromium.org/2189733006/diff/20001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:1629: <message name="IDS_LANGUAGE_DROPDOWN_TITLE" desc="Tile of language selection dropdown menu" meaning="Small title near language dropdown menu suggesting that this is a language selector."> Title (x4) https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_dialog.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_dialog.html:44: hidden="[[!hasButtons]]"> if you'd prefer, you can make hasButtons just a normal attribute and use css: :host(:not([has-buttons])) #oobe-bottom { display: none; } https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:28: <template is="dom-repeat" items$="[[items]]" on-dom-change="onDomChange_"> wrap line https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:28: <template is="dom-repeat" items$="[[items]]" on-dom-change="onDomChange_"> items should be property-bound not attribute-bound, ie items="[[items]]" https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:23: 'iron-select': 'onSelect_', prefer declarative listeners: <paper-listbox on-iron-select="onSelect_" https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:29: observers_disabled_: false, camelCase: observersDisabled_ https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:50: // DOM update, so we need to replace items completely. > modifying element itself doesn't lead to DOM update have you tried using this.set? See example in https://www.polymer-project.org/1.0/docs/devguide/data-binding#array-binding here's an example that uses on-iron-select and on-iron-deselect to show/hide the icons, without replacing the items array: https://jsfiddle.net/y4hettan/3/ When |items| changes from C++, I think you still need to do the trick of setting selected to undefined, and return early in onSelect_ if selected is undefined, but you don't need observers_disabled_ or to loop through the entire items array. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:110: this.$.listboxDropdown.selectIndex(null); you should be able to this in onItemsChanged_, and not need onDomChange_ https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:35: <div id="langugeButton" class="buttonbox layout vertical" languageButton https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:55: </h1> remove https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:18: value: null, a nullable string is weird... what about '' or undefined? https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:24: oobeLanguages: { nit: why not just "languages"? https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:32: oobeKeyboards: { same: "keyboards"? https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:241: onLanguageSelected_: function(event) { @param {!Event} event, @private https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:250: onKeyboardSelected_: function(event) { @param {!Event} event, @private
https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe.js:312: welcomeScreen.oobeKeyboards = data.inputMethodsList; nit: shouldn't we only do this inside the if below, i.e. if newOobeUI == 'on'? https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_dialog.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_dialog.html:44: hidden="[[!hasButtons]]"> On 2016/07/29 06:26:49, michaelpg wrote: > if you'd prefer, you can make hasButtons just a normal attribute and use css: > > :host(:not([has-buttons])) #oobe-bottom { > display: none; > } Or use <template is="dom-if" if="[[hasButtons]]"> https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:11: <g id="check"> Attribute where this came from, e.g. see comment in cr_elements/icons.html: https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/icons.htm... (It probably also makes sense for login to include a shared icons.html, but that can be cleaned up later) https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:16: type: Array, Array of what? Ideally this should be typed in the comment, e.g. @type {!Array<!I18nMenuItem>|undefined}. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:36: selectIndex_: null, The use of observersDisabled_ and selectIndex_ seems very C++ like; it would be nice if we could avoid them. Try Michael's suggestion below. You may need to add a Polymer.dom.flush() before calling this.$.listboxDropdown.selectIndex() in onItemsChanged_. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:38: onSelect_: function(event) { Document. e.g.: /** * @param {!{detail: !{item: { item: {!I18nMenuItem}}}} event * @private */ https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:35: <div id="langugeButton" class="buttonbox layout vertical" On 2016/07/29 06:26:50, michaelpg wrote: > languageButton Is this id needed? https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:50: <oobe-dialog id="languageSection" class="fit" id needed? https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:53: <div class="header flex vertical layout end-justified start"> nit: 'layout vertical' https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:57: <div class="footer vertical layout"> nit: 'layout vertical' (and below; use consistent order throughout, 'layout horizontal' or 'layout vertical' is the more common ordering). https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:18: value: null, On 2016/07/29 06:26:50, michaelpg wrote: > a nullable string is weird... what about '' or undefined? Generally undefined would be preferred if used in data binding so that the dom is only updated when the property is set (vs. twice, once with the initial value and once when set). Note: value: can be omitted if initially undefined. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:25: type: Array, @type these arrays. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:241: onLanguageSelected_: function(event) { On 2016/07/29 06:26:50, michaelpg wrote: > @param {!Event} event, @private Even better, '{!{detail: {!ItemType}}} event' (here and below) where ItemType is the correct type for Item, which currently isn't clear. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:277: */ We should either convert login/oobe to use the i18n{} translation model we are using in Settings, or use a shared behavior like we used to do for Settings. I wouldn't say that this is an absolute blocker for this CL, but it makes me sad to see this copied here. Please at least replace (stevenjb) with someone on the oobe/login team. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:167: builder->Add("OobeOKButtonText", IDS_OOBE_OK_BUTTON_TEXT); oobeOKButtonText
I am working on JS types checker, but all the rest should be fine. https://codereview.chromium.org/2189733006/diff/20001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:1629: <message name="IDS_LANGUAGE_DROPDOWN_TITLE" desc="Tile of language selection dropdown menu" meaning="Small title near language dropdown menu suggesting that this is a language selector."> On 2016/07/29 06:26:49, michaelpg wrote: > Title (x4) Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe.js:312: welcomeScreen.oobeKeyboards = data.inputMethodsList; On 2016/07/29 16:11:52, stevenjb wrote: > nit: shouldn't we only do this inside the if below, i.e. if newOobeUI == 'on'? Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_dialog.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_dialog.html:44: hidden="[[!hasButtons]]"> On 2016/07/29 16:11:52, stevenjb wrote: > On 2016/07/29 06:26:49, michaelpg wrote: > > if you'd prefer, you can make hasButtons just a normal attribute and use css: > > > > :host(:not([has-buttons])) #oobe-bottom { > > display: none; > > } > > Or use <template is="dom-if" if="[[hasButtons]]"> Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:11: <g id="check"> On 2016/07/29 16:11:52, stevenjb wrote: > Attribute where this came from, e.g. see comment in cr_elements/icons.html: > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/icons.htm... > > (It probably also makes sense for login to include a shared icons.html, but that > can be cleaned up later) Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:28: <template is="dom-repeat" items$="[[items]]" on-dom-change="onDomChange_"> On 2016/07/29 06:26:50, michaelpg wrote: > wrap line Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:28: <template is="dom-repeat" items$="[[items]]" on-dom-change="onDomChange_"> On 2016/07/29 06:26:49, michaelpg wrote: > items should be property-bound not attribute-bound, ie items="[[items]]" Done. The problem (was) that it doesn't work when items are replaced inside onSelect(). this.set() doesn't have this problem, so I removed $. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:23: 'iron-select': 'onSelect_', On 2016/07/29 06:26:50, michaelpg wrote: > prefer declarative listeners: > <paper-listbox on-iron-select="onSelect_" Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:29: observers_disabled_: false, On 2016/07/29 06:26:50, michaelpg wrote: > camelCase: observersDisabled_ Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:36: selectIndex_: null, On 2016/07/29 16:11:52, stevenjb wrote: > The use of observersDisabled_ and selectIndex_ seems very C++ like; it would be > nice if we could avoid them. Try Michael's suggestion below. You may need to add > a Polymer.dom.flush() before calling this.$.listboxDropdown.selectIndex() in > onItemsChanged_. Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:50: // DOM update, so we need to replace items completely. On 2016/07/29 06:26:50, michaelpg wrote: > > modifying element itself doesn't lead to DOM update > > have you tried using this.set? See example in > https://www.polymer-project.org/1.0/docs/devguide/data-binding#array-binding > > here's an example that uses on-iron-select and on-iron-deselect to show/hide the > icons, without replacing the items array: https://jsfiddle.net/y4hettan/3/ > > When |items| changes from C++, I think you still need to do the trick of setting > selected to undefined, and return early in onSelect_ if selected is undefined, > but you don't need observers_disabled_ or to loop through the entire items > array. Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:110: this.$.listboxDropdown.selectIndex(null); On 2016/07/29 06:26:50, michaelpg wrote: > you should be able to this in onItemsChanged_, and not need onDomChange_ Done.
This looks better. Still waiting on updates to oobe_welcome.*. https://codereview.chromium.org/2189733006/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:59: continue; nit: var index = items.findIndex(function(item) { return item.selected; }); if (index != -1) ... More info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
I added Closure Compiler descriptions. > This looks better. Still waiting on updates to oobe_welcome.*. Sorry, I missed these files. Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:16: type: Array, On 2016/07/29 16:11:52, stevenjb wrote: > Array of what? Ideally this should be typed in the comment, e.g. @type > {!Array<!I18nMenuItem>|undefined}. Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:38: onSelect_: function(event) { On 2016/07/29 16:11:52, stevenjb wrote: > Document. e.g.: > /** > * @param {!{detail: !{item: { item: {!I18nMenuItem}}}} event > * @private > */ Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.html (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:35: <div id="langugeButton" class="buttonbox layout vertical" On 2016/07/29 16:11:52, stevenjb wrote: > On 2016/07/29 06:26:50, michaelpg wrote: > > languageButton > > Is this id needed? Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:50: <oobe-dialog id="languageSection" class="fit" On 2016/07/29 16:11:52, stevenjb wrote: > id needed? Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:53: <div class="header flex vertical layout end-justified start"> On 2016/07/29 16:11:52, stevenjb wrote: > nit: 'layout vertical' Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:55: </h1> On 2016/07/29 06:26:50, michaelpg wrote: > remove Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:57: <div class="footer vertical layout"> On 2016/07/29 16:11:52, stevenjb wrote: > nit: 'layout vertical' (and below; use consistent order throughout, 'layout > horizontal' or 'layout vertical' is the more common ordering). > Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.js (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:18: value: null, On 2016/07/29 16:11:52, stevenjb wrote: > On 2016/07/29 06:26:50, michaelpg wrote: > > a nullable string is weird... what about '' or undefined? > > Generally undefined would be preferred if used in data binding so that the dom > is only updated when the property is set (vs. twice, once with the initial value > and once when set). Note: value: can be omitted if initially undefined. > Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:24: oobeLanguages: { On 2016/07/29 06:26:50, michaelpg wrote: > nit: why not just "languages"? Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:25: type: Array, On 2016/07/29 16:11:52, stevenjb wrote: > @type these arrays. Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:32: oobeKeyboards: { On 2016/07/29 06:26:50, michaelpg wrote: > same: "keyboards"? Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:241: onLanguageSelected_: function(event) { On 2016/07/29 16:11:52, stevenjb wrote: > On 2016/07/29 06:26:50, michaelpg wrote: > > @param {!Event} event, @private > > Even better, '{!{detail: {!ItemType}}} event' (here and below) where ItemType is > the correct type for Item, which currently isn't clear. > > Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:250: onKeyboardSelected_: function(event) { On 2016/07/29 06:26:50, michaelpg wrote: > @param {!Event} event, @private Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:277: */ On 2016/07/29 16:11:52, stevenjb wrote: > We should either convert login/oobe to use the i18n{} translation model we are > using in Settings, or use a shared behavior like we used to do for Settings. I > wouldn't say that this is an absolute blocker for this CL, but it makes me sad > to see this copied here. Please at least replace (stevenjb) with someone on the > oobe/login team. > Done. https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right): https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:167: builder->Add("OobeOKButtonText", IDS_OOBE_OK_BUTTON_TEXT); On 2016/07/29 16:11:53, stevenjb wrote: > oobeOKButtonText Done. https://codereview.chromium.org/2189733006/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:59: continue; On 2016/08/01 17:34:57, stevenjb wrote: > nit: > var index = items.findIndex(function(item) { return item.selected; }); > if (index != -1) > ... > > More info: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... 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...
lgtm https://codereview.chromium.org/2189733006/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:14: */ Fix alignment
https://codereview.chromium.org/2189733006/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:14: */ On 2016/08/01 22:38:39, stevenjb wrote: > Fix alignment 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/2189733006/diff/80001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:1638: <message name="IDS_LANGUAGE_SECTION_TITE" desc="Title of language selection screen" meaning="A title of the dialog where user should select ChromeOS UI language and default keyboard layout during initial device setup."> TITLE here & below https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/custom_elements_oobe.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/custom_elements_oobe.js:7: // This inclusion should go first, as <{controller,host}-paring-screen> depend update comment? https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:5: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html"> alphabetize imports https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:27: color: var(--google-blue-500); import whatever polymer file defines this color https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:35: <paper-item item="[[item]]" disabled="[[item.optionGroupName]]"> paper-item doesn't have an |item| property... https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:37: <div class="item-name" hidden="[[item.optionGroupName]]"> what does this class do? https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:44: <hr hidden="[[!item.optionGroupName]]"> should this be done in CSS or is the <hr> styled properly? https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:27: value: null, why not just omit value (undefined)? https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:38: * @param {!{detail: !{item: { item: {!I18nMenuItem}}}}} event dom-repeat has a |mode| property on the event, check out the docs. i think that's what you want instead of the item.item thing https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:74: // this.items (for example, translated into other language) . nit: remove space before . https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:78: this.$.listboxDropdown.selectIndex(null); can this be wrapped in something like if (this.$.listboxDropdown.selected == index) https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_network.js:34: function(languageId) { optional: this.onLanguageSelected_.bind(this) https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_network.js:40: self.onKeyboardSelected_(inputMethodId); same https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_network.js:44: function(timezoneId) { same https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_text_button.css (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_text_button.css:19: revert newline https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_types.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_types.js:16: * code: (!String|undefined), ! is implied for primitive types like String and Boolean https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.html (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:36: on-click="onWelcomeSelectLanguageButtonClicked_"> on-tap? https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:27: value: null, same for these, can the value just be left out? see https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2189733006/diff/80001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:1638: <message name="IDS_LANGUAGE_SECTION_TITE" desc="Title of language selection screen" meaning="A title of the dialog where user should select ChromeOS UI language and default keyboard layout during initial device setup."> On 2016/08/02 00:06:38, michaelpg wrote: > TITLE here & below Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/custom_elements_oobe.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/custom_elements_oobe.js:7: // This inclusion should go first, as <{controller,host}-paring-screen> depend On 2016/08/02 00:06:38, michaelpg wrote: > update comment? Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:5: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html"> On 2016/08/02 00:06:38, michaelpg wrote: > alphabetize imports Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:27: color: var(--google-blue-500); On 2016/08/02 00:06:38, michaelpg wrote: > import whatever polymer file defines this color Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:35: <paper-item item="[[item]]" disabled="[[item.optionGroupName]]"> On 2016/08/02 00:06:38, michaelpg wrote: > paper-item doesn't have an |item| property... I just need this property in 'iron-select' event detail. Instead of creating another custom element around paper-item, I added property to polymer element. This seems to work. Do you think I should use item$=... ? https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:37: <div class="item-name" hidden="[[item.optionGroupName]]"> On 2016/08/02 00:06:38, michaelpg wrote: > what does this class do? Removed. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:44: <hr hidden="[[!item.optionGroupName]]"> On 2016/08/02 00:06:38, michaelpg wrote: > should this be done in CSS or is the <hr> styled properly? What do you mean? I need "horizontal line in the middle of the [empty] entry". This one works. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:27: value: null, On 2016/08/02 00:06:38, michaelpg wrote: > why not just omit value (undefined)? Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:38: * @param {!{detail: !{item: { item: {!I18nMenuItem}}}}} event On 2016/08/02 00:06:38, michaelpg wrote: > dom-repeat has a |mode| property on the event, check out the docs. i think > that's what you want instead of the item.item thing Sorry, I didn't find anything like that. https://www.polymer-project.org/1.0/docs/api/dom-repeat says there is |model| property, but it is an instance of Polymer.Base. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:74: // this.items (for example, translated into other language) . On 2016/08/02 00:06:38, michaelpg wrote: > nit: remove space before . Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:78: this.$.listboxDropdown.selectIndex(null); On 2016/08/02 00:06:38, michaelpg wrote: > can this be wrapped in something like > if (this.$.listboxDropdown.selected == index) Sure. Done. I just don't understand why adding two lines would make is better here. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_network.js:34: function(languageId) { On 2016/08/02 00:06:38, michaelpg wrote: > optional: this.onLanguageSelected_.bind(this) Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_network.js:40: self.onKeyboardSelected_(inputMethodId); On 2016/08/02 00:06:38, michaelpg wrote: > same Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_screen_network.js:44: function(timezoneId) { On 2016/08/02 00:06:38, michaelpg wrote: > same Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_text_button.css (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_text_button.css:19: On 2016/08/02 00:06:38, michaelpg wrote: > revert newline Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_types.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_types.js:16: * code: (!String|undefined), On 2016/08/02 00:06:38, michaelpg wrote: > ! is implied for primitive types like String and Boolean Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.html (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.html:36: on-click="onWelcomeSelectLanguageButtonClicked_"> On 2016/08/02 00:06:38, michaelpg wrote: > on-tap? Done. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_welcome.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_welcome.js:27: value: null, On 2016/08/02 00:06:38, michaelpg wrote: > same for these, can the value just be left out? > see > https://codereview.chromium.org/2189733006/diff/20001/chrome/browser/resource... 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:35: <paper-item item="[[item]]" disabled="[[item.optionGroupName]]"> On 2016/08/02 04:25:56, Alexander Alekseev wrote: > On 2016/08/02 00:06:38, michaelpg wrote: > > paper-item doesn't have an |item| property... > > I just need this property in 'iron-select' event detail. use the dom-repeat's modelForElement function: https://www.polymer-project.org/1.0/docs/api/dom-repeat > Instead of creating another custom element around paper-item, I added property > to polymer element. > > This seems to work. > > Do you think I should use item$=... ? https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.html:44: <hr hidden="[[!item.optionGroupName]]"> On 2016/08/02 04:25:56, Alexander Alekseev wrote: > On 2016/08/02 00:06:38, michaelpg wrote: > > should this be done in CSS or is the <hr> styled properly? > > What do you mean? > > I need "horizontal line in the middle of the [empty] entry". This > one works. I mean will this be a default, tall, grey, inset <hr> from the 90's or just a simple black line? We almost never use <hr>. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:38: * @param {!{detail: !{item: { item: {!I18nMenuItem}}}}} event On 2016/08/02 04:25:57, Alexander Alekseev wrote: > On 2016/08/02 00:06:38, michaelpg wrote: > > dom-repeat has a |mode| property on the event, check out the docs. i think > > that's what you want instead of the item.item thing > > Sorry, I didn't find anything like that. I wasn't quite right: in this case, the event is firing on the <paper-listbox>, so that's what provides event.detail.model here. > https://www.polymer-project.org/1.0/docs/api/dom-repeat says there is |model| > property, but it is an instance of Polymer.Base. Yes, which is awesome! You can manipulate the model like any other Polymer element, so you can do: model.set('item.foo', 'Foo') and the item will update everywhere like normal. https://codereview.chromium.org/2189733006/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:78: this.$.listboxDropdown.selectIndex(null); On 2016/08/02 04:25:57, Alexander Alekseev wrote: > On 2016/08/02 00:06:38, michaelpg wrote: > > can this be wrapped in something like > > if (this.$.listboxDropdown.selected == index) > > Sure. Done. > > I just don't understand why adding two lines would make is better here. Hmm, it seemed like a rare edge case but I'm not quite sure what circumstances this happens in. If the <input> element is usually wrong, it's fine as-is. Just seems silly to toggle the listbox selection when we don't need to. https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:44: var selected = event.detail.item.item; var selectedModel = domRepeat.modelForElement(event.detail.item); https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:48: this.set('items.' + this.$$('paper-listbox').selected + '.selected', true); selectedModel.set('item.selected', true); https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:56: 'items.' + this.$$('paper-listbox').indexOf(e.detail.item) + same https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_welcome.js (right): https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_welcome.js:25: languages: { opt nit, throughout: use the shorthand for unconfigured properties languages = Array;
https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:44: var selected = event.detail.item.item; On 2016/08/02 20:31:53, michaelpg wrote: > var selectedModel = domRepeat.modelForElement(event.detail.item); Done. https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:48: this.set('items.' + this.$$('paper-listbox').selected + '.selected', true); On 2016/08/02 20:31:53, michaelpg wrote: > selectedModel.set('item.selected', true); Done. https://codereview.chromium.org/2189733006/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:56: 'items.' + this.$$('paper-listbox').indexOf(e.detail.item) + On 2016/08/02 20:31:53, michaelpg wrote: > same Done.
Please note, that I hid language selection from tests because of bug in polymer a11y.
On 2016/08/03 06:34:02, Alexander Alekseev wrote: > Please note, that I hid language selection from tests because of bug in polymer > a11y. It's not clear how it's hidden or that it's done for testing purposes. Did the options I showed you to disable the tests for the buggy elements not work? Also please provide a link to the bug(s). https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe.js (right): https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe.js:308: if (data.newOobeUI == 'on') { We know at startup whether or not the md-oobe flag is enabled, so why do we create the element when the --enable-md-oobe flag isn't enabled? Is this feature effectively "launched" because the code runs on every device regardless of flags? https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe.js:318: welcomeScreen.enabled = true; I guess I would say if we want to enable/disable the MD welcome screen, we should enable/disable the whole element, not just a Language section we want to hide from tests. https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js (right): https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.js:32: * @type {!string} type not needed when it's the same as below https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_welcome.js (right): https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_welcome.js:62: * Flag that enables MD-OOBE. Can you explain more what this does? If this is the flag you added to hide the language selection from tests, it needs a better name and comment.
lgtm to unblock other work https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right): https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_network.js:44: var languageList = loadTimeData.getValue('languageList'); this section looks like it's doing the same work that oobe.js does. is this a different instance of oobe-welcome-md?
Thank you! https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right): https://codereview.chromium.org/2189733006/diff/140001/chrome/browser/resourc... chrome/browser/resources/chromeos/login/oobe_screen_network.js:44: var languageList = loadTimeData.getValue('languageList'); On 2016/08/04 22:59:28, michaelpg wrote: > this section looks like it's doing the same work that oobe.js does. is this a > different instance of oobe-welcome-md? This is called during initialization. The code that looks the same in oobe.js is called when language is changed, so all strings are replaced. The problem is that the initialization data and updates are different in structure, that is why we have different implementations.
The CQ bit was checked by alemate@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2189733006/#ps140001 (title: "Update after review.")
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
Try jobs failed on following builders: linux_chromium_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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alemate@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== ChromeOS: Implement Language Selection screen of material design OOBE. This CL adds Language Selection screen to material design OOBE. BUG=604119 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== ChromeOS: Implement Language Selection screen of material design OOBE. This CL adds Language Selection screen to material design OOBE. BUG=604119 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/f5174e6456f308a7af62106850315c59c255c8e5 Cr-Commit-Position: refs/heads/master@{#410255} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f5174e6456f308a7af62106850315c59c255c8e5 Cr-Commit-Position: refs/heads/master@{#410255} |
