Polished UI for the host side of pairing flow.

Polished UI for the host side of pairing flow.

* Polymer framework was used in the implementation
* Created custom element <oobe-screen>.
* HostPairingScreen was converted to <host-paring-screen> which extends <oobe-screen>.
* Several <core-*> Polymer elements were used.
* Roboto font used.

Polymer elements and fonts are imported to the page in a runtime, in contrast with a current implementation, where almost all resources are unlined. Can't say how runtime imports affect a performance.

BUG=375191
TEST=manually

Committed: https://crrev.com/71ce1027469ab6214158a7eca515afffccdb8017
Cr-Commit-Position: refs/heads/master@{#297244}

[dzhioev (left Google), 2014-09-25 19:21:19]

dzhioev@chromium.org changed reviewers:
+ nkostylev@chromium.org

[dzhioev (left Google), 2014-09-25 19:28:22]

https://codereview.chromium.org/599273004/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe-screen.js (right):

https://codereview.chromium.org/599273004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe-screen.js:4: 
This file is mostly a copy of ui/login/screen.js. Code duplication will be
removed when controller part is converted to Polymer as well.

https://codereview.chromium.org/599273004/diff/1/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html
(right):

https://codereview.chromium.org/599273004/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html:33: 
i18n and C are defined in 'oobe-screen.js'

[Nikita (slow), 2014-09-25 19:58:16]

Can you please make screenshots available via Drive link?

[Nikita (slow), 2014-09-25 19:59:57]

Please expand description and mention major things that are now used like custom
embedded fonts, and so on.

If you had to make any shortcuts in this CL please also mention them with a plan
to address them.

[Nikita (slow), 2014-09-25 20:20:00]

first round of comments

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/login_common.js (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/login_common.js:378:
window.addEventListener('polymer-ready', function() {
Who generates this event?

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe-screen.html (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.html:2: 
nit: drop empty line?

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe-screen.js (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.js:215: 
nit: Drop empty line.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe.html (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe.html:7: <script
src="chrome://oobe/polymer/platform/platform.js"></script>
I suggest having all Polymer specific include/script/link elements grouped
together. May as well add comment.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe.html:11: <link rel="stylesheet"
href="roboto_font.css">
Do you think whether this will have perfomance effect on boot time?
Specifically for cases when font is not needed?

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe.js (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe.js:10: var WAIT_FOR_POLYMER = true;
nit: comment is missing.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe.js:13: <include
src="oobe-screen.js">
nit: Same regarding grouping Polymer specific elements.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.css
(right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.css:16: left:
114px;
nit: rtl

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.css:27: right:
95px;
nit: rtl

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.css:35: left:
130px;
nit: rtl

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html
(right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html:54: <!--
'enrollmentTitle' contains <strong> tag. We need to wrap it in
nit: Provide generalized version of this comment in html-echo element
definition.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:86: }
nit: drop {}

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:124:
source->AddResourcePath("Roboto-Thin.ttf", IDR_FONT_ROBOTO_THIN);
nit: I think it makes sense to file a tracking bug and figure out whether we
want to promote this font resource to something more common and whether other
Chrome WebUI pages should use it.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/polymer_resources_map.h (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/polymer_resources_map.h:14:
std::vector<std::pair<std::string, int>> GetPolymerResourcesMap();
nit: Same here, file a tracking bug to figure out a right place for this file
and registration of these elements with WebUIDataSource.

[Nikita (slow), 2014-09-26 09:10:08]

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe-screen.js (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.js:24: C: null,
nit: Add comment. Maybe use more descriptive name instead?

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.js:118: sendImpl_:
function() {
Maybe drop sendImpl_ method?

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.js:123: * Does the following
things:
Is this comment describes what initializeImpl_ method will eventually contain?

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html
(right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html:46: <!--
Not yet implemented on backend side. -->
nit: File a tracking bug for this.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.js
(right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.js:11:
polymerScreen_: null,
nit: Add comment explaining why such wrapper is needed.

[Nikita (slow), 2014-09-26 12:22:02]

overall lgtm with these comments addressed.

[dzhioev (left Google), 2014-09-26 22:56:11]

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/login_common.js (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/login_common.js:378:
window.addEventListener('polymer-ready', function() {
On 2014/09/25 20:19:59, Nikita Kostylev wrote:
> Who generates this event?

Polymer does, when it is loaded and initialized.
https://www.polymer-project.org/docs/polymer/polymer.html#polymer-ready

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe-screen.html (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.html:2: 
On 2014/09/25 20:19:59, Nikita Kostylev wrote:
> nit: drop empty line?
I prefer to leave this empty line. <link> elements declare and import
dependencies of the file. Something like list of includes in *.cc files.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe-screen.js (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.js:24: C: null,
On 2014/09/26 09:10:08, Nikita Kostylev wrote:
> nit: Add comment. Maybe use more descriptive name instead?

Done with comment.
I think such short name is better, because it is used very often in HTML data
bindings.
Android has 'R' object with a similar purpose.
http://developer.android.com/reference/android/R.html

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.js:118: sendImpl_:
function() {
On 2014/09/26 09:10:08, Nikita Kostylev wrote:
> Maybe drop sendImpl_ method?
It will be used for controller screen implementation.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.js:123: * Does the following
things:
On 2014/09/26 09:10:07, Nikita Kostylev wrote:
> Is this comment describes what initializeImpl_ method will eventually contain?

It is not needed. Removed.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe-screen.js:215: 
On 2014/09/25 20:19:59, Nikita Kostylev wrote:
> nit: Drop empty line.

Done.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe.html (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe.html:7: <script
src="chrome://oobe/polymer/platform/platform.js"></script>
On 2014/09/25 20:19:59, Nikita Kostylev wrote:
> I suggest having all Polymer specific include/script/link elements grouped
> together. May as well add comment.
It is not possible now:
* custom element's CSS should be defined inside element definition.
* We should all <script src="..."> inclusions of local JS files to oobe.js.
Otherwise Grit will flatten them and they will not work.
* platform.js should be included first.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe.html:11: <link rel="stylesheet"
href="roboto_font.css">
On 2014/09/25 20:19:59, Nikita Kostylev wrote:
> Do you think whether this will have perfomance effect on boot time?
> Specifically for cases when font is not needed?
I doubt it.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe.js (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe.js:10: var WAIT_FOR_POLYMER = true;
On 2014/09/25 20:19:59, Nikita Kostylev wrote:
> nit: comment is missing.

Done.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe.js:13: <include
src="oobe-screen.js">
On 2014/09/25 20:19:59, Nikita Kostylev wrote:
> nit: Same regarding grouping Polymer specific elements.

See my comment above.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.css
(right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.css:16: left:
114px;
On 2014/09/25 20:20:00, Nikita Kostylev wrote:
> nit: rtl

RTL will not be supported in this version. Added TODO.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.css:27: right:
95px;
On 2014/09/25 20:20:00, Nikita Kostylev wrote:
> nit: rtl

ditto

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.css:35: left:
130px;
On 2014/09/25 20:20:00, Nikita Kostylev wrote:
> nit: rtl

ditto

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html
(right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html:46: <!--
Not yet implemented on backend side. -->
On 2014/09/26 09:10:08, Nikita Kostylev wrote:
> nit: File a tracking bug for this.
OK, will file on monday.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.html:54: <!--
'enrollmentTitle' contains <strong> tag. We need to wrap it in
On 2014/09/25 20:20:00, Nikita Kostylev wrote:
> nit: Provide generalized version of this comment in html-echo element
> definition.

Done.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
File chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.js
(right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/resources...
chrome/browser/resources/chromeos/login/oobe_screen_host_pairing.js:11:
polymerScreen_: null,
On 2014/09/26 09:10:08, Nikita Kostylev wrote:
> nit: Add comment explaining why such wrapper is needed.

Done.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:86: }
On 2014/09/25 20:20:00, Nikita Kostylev wrote:
> nit: drop {}

Done.

https://codereview.chromium.org/599273004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:124:
source->AddResourcePath("Roboto-Thin.ttf", IDR_FONT_ROBOTO_THIN);
On 2014/09/25 20:20:00, Nikita Kostylev wrote:
> nit: I think it makes sense to file a tracking bug and figure out whether we
> want to promote this font resource to something more common and whether other
> Chrome WebUI pages should use it.

Agree, will file on Monday.

[dzhioev (left Google), 2014-09-26 23:03:15]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-09-26 23:03:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599273004/60001

[commit-bot: I haz the power, 2014-09-27 00:51:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-27 00:51:56]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dzhioev (left Google), 2014-09-27 03:20:33]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-09-27 03:21:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599273004/80001

[commit-bot: I haz the power, 2014-09-27 04:46:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-27 04:46:24]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dzhioev (left Google), 2014-09-29 14:19:45]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-09-29 14:20:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/599273004/100001

[commit-bot: I haz the power, 2014-09-29 20:27:29]

Committed patchset #6 (id:100001) as f592f541804869c1d75275d07680e37134e0b6b5

[commit-bot: I haz the power, 2014-09-29 20:28:11]

Patchset 6 (id:??) landed as
https://crrev.com/71ce1027469ab6214158a7eca515afffccdb8017
Cr-Commit-Position: refs/heads/master@{#297244}