[cros] Layout for OOBE WebUI.

[cros] Layout for OOBE WebUI.
* Based on the layout done by UX designers
* Dropped div elements for EULA content since it should be a separate file (might be reconsidered later), using iframes instead.
* Extracted select CSS to common resources.

BUG=chromium-os:15632
TEST=Manual, navigated to chrome://oobe.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88657

[Nikita (slow), 2011-06-03 17:39:04]

altimofeev for C++/strings changes,
jhawkins for HTML/CSS/JS changes.

[altimofeev, 2011-06-06 08:19:22]

LGTM

[Nikita (slow), 2011-06-06 20:40:54]

Ping?

On Fri, Jun 3, 2011 at 9:39 PM, <nkostylev@chromium.org> wrote:

> Reviewers: altimofeev, James Hawkins,
>
> Message:
> altimofeev for C++/strings changes,
> jhawkins for HTML/CSS/JS changes.
>
> Description:
> [cros] Layout for OOBE WebUI.
> * Based on the layout done by UX designers
> * Dropped div elements for EULA content since it should be a separate file
> (might be reconsidered later), using iframes instead.
> * Extracted select CSS to common resources.
>
> BUG=chromium-os:15632
> TEST=Manual, navigated to chrome://oobe.
>
>
> Please review this at
http://codereview.chromium.**org/7058048/<http://codereview.chromium.org/7058...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/app/generated_**resources.grd
>  M chrome/app/theme/theme_**resources.grd
>  M chrome/browser/resources/bug_**report.html
>  M chrome/browser/resources/**chromeos/oobe.css
>  M chrome/browser/resources/**chromeos/oobe.html
>  A chrome/browser/resources/**chromeos/oobe.js
>  M chrome/browser/resources/**options/options.html
>  M chrome/browser/resources/**options/options_page.css
>  D chrome/browser/resources/**options/select.png
>  A + chrome/browser/resources/**shared/images/select.png
>  M chrome/browser/resources/**shared_resources.grd
>  M chrome/browser/ui/webui/**chromeos/login/eula_screen_**handler.cc
>  M chrome/browser/ui/webui/**chromeos/login/oobe_ui.cc
>
>
>


-- 
Nikita

[Nikita (slow), 2011-06-06 21:08:24]

+ Evan

[Evan Stade, 2011-06-07 03:25:46]

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.css:165: #tpm {
don't abbreviate variable names

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:20: <span id="hconnect"
class="header-section" i18n-content="welcomeScreenTitle"></span>
don't abbreviate for variable names (hconnect->headerConnect or something)

actually, do you even need to give this an ID? where is the ID used?

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:22: <span id="hupdate"
class="header-section right" i18n-content="updateScreenTitle"></span>
80 still applies even in html and css files

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:28: <table>
I'm skeptical you really want a table, this doesn't look like tabular data.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:41: <tr height='40px'>
do not use height attribute

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:4: 
file level comment. I honestly have no idea what OOBE stands for.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:9: function Oobe() {
this seems wrong.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:18: current_step_: 0,
these are c style var names, instead they should be camelCase.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:20: toggleStep_: function(next_step) {
comment your functions

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:83: 
don't need this line return

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
File chrome/browser/resources/shared/css/select.css (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/select.css:1: select {
need license header

have you made sure that generalizing this doesn't cause any weird breakages for
other WebUI pages like print preview?

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/select.css:4: -webkit-box-shadow: 0px 1px
3px rgba(0, 0, 0, 0.1);
no px after 0

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/select.css:8: background-image:
url("../images/select.png"), -webkit-linear-gradient(#fafafa, #f4f4f4 40%,
#e5e5e5);
80

[Nikita (slow), 2011-06-07 17:03:35]

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.css:165: #tpm {
On 2011/06/07 03:25:46, Evan Stade wrote:
> don't abbreviate variable names

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:20: <span id="hconnect"
class="header-section" i18n-content="welcomeScreenTitle"></span>
On 2011/06/07 03:25:46, Evan Stade wrote:
> actually, do you even need to give this an ID? where is the ID used?

It's used in JS:
var oldheader = $("h" + steps[this.current_step_]);
var newheader = $("h" + steps[this.current_step_ + offset]);

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:20: <span id="hconnect"
class="header-section" i18n-content="welcomeScreenTitle"></span>
On 2011/06/07 03:25:46, Evan Stade wrote:
> don't abbreviate for variable names (hconnect->headerConnect or something)

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:22: <span id="hupdate"
class="header-section right" i18n-content="updateScreenTitle"></span>
On 2011/06/07 03:25:46, Evan Stade wrote:
> 80 still applies even in html and css files

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:28: <table>
On 2011/06/07 03:25:46, Evan Stade wrote:
> I'm skeptical you really want a table, this doesn't look like tabular data.

I've using this layout mostly as-is done by the designers.
I understand that using table is not the best approach but could it be left for
now to speed up development of related backend changes?

Initially I've had these 3 controls defined with divs, see
http://codereview.chromium.org/7076014/diff/6001/chrome/browser/resources/chr...
and corresponding CSS but that had issues with vertical alignment of the text in
case of longer i18n string translations.

I would like to switch to divs in future CLs, when I'll create a proper flexible
layout.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:41: <tr height='40px'>
On 2011/06/07 03:25:46, Evan Stade wrote:
> do not use height attribute

At all or move it to css style?

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:4: 
On 2011/06/07 03:25:46, Evan Stade wrote:
> file level comment. I honestly have no idea what OOBE stands for.

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:9: function Oobe() {
On 2011/06/07 03:25:46, Evan Stade wrote:
> this seems wrong.

You mean when there're no parameters I may as well remove it?

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:18: current_step_: 0,
On 2011/06/07 03:25:46, Evan Stade wrote:
> these are c style var names, instead they should be camelCase.

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:20: toggleStep_: function(next_step) {
On 2011/06/07 03:25:46, Evan Stade wrote:
> comment your functions

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:83: 
On 2011/06/07 03:25:46, Evan Stade wrote:
> don't need this line return

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
File chrome/browser/resources/shared/css/select.css (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/select.css:1: select {
On 2011/06/07 03:25:46, Evan Stade wrote:
> need license header

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/select.css:1: select {
On 2011/06/07 03:25:46, Evan Stade wrote:
> have you made sure that generalizing this doesn't cause any weird breakages
for
> other WebUI pages like print preview?

I've extracted select style from options_page.css and updated them all to
include new shared select.css.

How should I look for a possible regression on pages like print preview? Pages
that don't include options_page.css should be unaffected.

Actually I've added proxy_settings.html that I've missed first time.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/select.css:4: -webkit-box-shadow: 0px 1px
3px rgba(0, 0, 0, 0.1);
On 2011/06/07 03:25:46, Evan Stade wrote:
> no px after 0

Done.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/sha...
chrome/browser/resources/shared/css/select.css:8: background-image:
url("../images/select.png"), -webkit-linear-gradient(#fafafa, #f4f4f4 40%,
#e5e5e5);
On 2011/06/07 03:25:46, Evan Stade wrote:
> 80

Done.

[James Hawkins, 2011-06-07 17:17:53]

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:10: from(rgb(254, 254, 254)),
to(rgb(239, 239, 239)));
nit: Indentation. I can't remember if we indent 4 spaces from previous line, or
if we match the property value indentation. Look at various CSS files to be
sure.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:16: width: 640px;
nit: Alphabetize properties, here and elsewhere.  -webkit properties should be
at the top of the list (to highlight that they're non-standard).

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:25: padding: 20px 20px;
padding: 20px;

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:37: color: rgb(0, 102, 204);
nit: Use hex, here and elsewhere. rgba() is fine as is.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:49: background:
-webkit-gradient(linear, left top, left bottom, from(rgba(0,0,0,0.3)),
to(rgba(0,0,0,0.0)));
nit: 80 cols.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:81: content: "\00A0\203A\00A0\00A0";
Can you add a comment to describe what this is?

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:21:
i18n-content="welcomeScreenTitle"></span>
nit: 4 space indentation from previous line.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:31: <table>
This section is not displaying tabular data; please don't use tables.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:73: </div> <!-- Inner Container -->
nit: Remove this and similar comments.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:10: var steps = ["Connect", "Eula",
"Update"];
const?

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:25: initialized_: false,
You (seemingly) don't ever use this variable except to set it to true; remove
it.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:37: if (nextStep >= 0 && nextStep <
steps.length) {
In what situations would |nextStep| be < 0 or >= steps.length?

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:62:
setTimeout(function(){oldstep.classList.add('hidden');}, 500);
Is this animated via CSS?

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/select.css (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/select.css:8: select {
Cool, thanks for refactoring this.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/select.css:34: html[dir='rtl'] select {
nit: Can you move this to right below the 'select' selector?

[Nikita (slow), 2011-06-07 18:02:04]

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:10: from(rgb(254, 254, 254)),
to(rgb(239, 239, 239)));
On 2011/06/07 17:17:54, James Hawkins wrote:
> nit: Indentation. I can't remember if we indent 4 spaces from previous line,
or
> if we match the property value indentation. Look at various CSS files to be
> sure.

Done. I've seen, that we match the property value if it fits on a line.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:16: width: 640px;
On 2011/06/07 17:17:54, James Hawkins wrote:
> nit: Alphabetize properties, here and elsewhere.  -webkit properties should be
> at the top of the list (to highlight that they're non-standard).

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:25: padding: 20px 20px;
On 2011/06/07 17:17:54, James Hawkins wrote:
> padding: 20px;

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:37: color: rgb(0, 102, 204);
On 2011/06/07 17:17:54, James Hawkins wrote:
> nit: Use hex, here and elsewhere. rgba() is fine as is.

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:49: background:
-webkit-gradient(linear, left top, left bottom, from(rgba(0,0,0,0.3)),
to(rgba(0,0,0,0.0)));
On 2011/06/07 17:17:54, James Hawkins wrote:
> nit: 80 cols.

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:81: content: "\00A0\203A\00A0\00A0";
On 2011/06/07 17:17:54, James Hawkins wrote:
> Can you add a comment to describe what this is?

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:21:
i18n-content="welcomeScreenTitle"></span>
On 2011/06/07 17:17:54, James Hawkins wrote:
> nit: 4 space indentation from previous line.

Done. But I've seen such formatting in other WebUI pages.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:31: <table>
On 2011/06/07 17:17:54, James Hawkins wrote:
> This section is not displaying tabular data; please don't use tables.

Is it ok to convert to divs in follow up CL?

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:73: </div> <!-- Inner Container -->
On 2011/06/07 17:17:54, James Hawkins wrote:
> nit: Remove this and similar comments.

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:10: var steps = ["Connect", "Eula",
"Update"];
On 2011/06/07 17:17:54, James Hawkins wrote:
> const?

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:25: initialized_: false,
On 2011/06/07 17:17:54, James Hawkins wrote:
> You (seemingly) don't ever use this variable except to set it to true; remove
> it.

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:37: if (nextStep >= 0 && nextStep <
steps.length) {
On 2011/06/07 17:17:54, James Hawkins wrote:
> In what situations would |nextStep| be < 0 or >= steps.length?

It shouldn't be, just a sanity check.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:62:
setTimeout(function(){oldstep.classList.add('hidden');}, 500);
On 2011/06/07 17:17:54, James Hawkins wrote:
> Is this animated via CSS?

Yes:
.header-section {
  -webkit-transition: all .2s ease-in-out;
}
etc.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/sh...
File chrome/browser/resources/shared/css/select.css (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/sh...
chrome/browser/resources/shared/css/select.css:34: html[dir='rtl'] select {
On 2011/06/07 17:17:54, James Hawkins wrote:
> nit: Can you move this to right below the 'select' selector?

Done.

[James Hawkins, 2011-06-07 18:13:06]

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:21:
i18n-content="welcomeScreenTitle"></span>
On 2011/06/07 18:02:04, Nikita Kostylev wrote:
> On 2011/06/07 17:17:54, James Hawkins wrote:
> > nit: 4 space indentation from previous line.
> 
> Done. But I've seen such formatting in other WebUI pages.

Right; they need to be fixed to match the Chromium style as well.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:31: <table>
On 2011/06/07 18:02:04, Nikita Kostylev wrote:
> On 2011/06/07 17:17:54, James Hawkins wrote:
> > This section is not displaying tabular data; please don't use tables.
> 
> Is it ok to convert to divs in follow up CL?

I'd rather it be done in this CL, so we don't lose it.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:37: if (nextStep >= 0 && nextStep <
steps.length) {
On 2011/06/07 18:02:04, Nikita Kostylev wrote:
> On 2011/06/07 17:17:54, James Hawkins wrote:
> > In what situations would |nextStep| be < 0 or >= steps.length?
> 
> It shouldn't be, just a sanity check.

If the backend ever gets into a buggy state where it's sending invalid steps,
you won't know about it.  I'd remove this check and let the JS 'crash' so you
have data for that case.

http://codereview.chromium.org/7058048/diff/15001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/15001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:17: margin-top: -270px;
margin-top after margin-left.

http://codereview.chromium.org/7058048/diff/15001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:80: height: 1.5em;
Why are you using height instead of proper padding?

[Nikita (slow), 2011-06-08 14:30:54]

Converted from table to div elements.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:31: <table>
On 2011/06/07 18:13:06, James Hawkins wrote:
> On 2011/06/07 18:02:04, Nikita Kostylev wrote:
> > On 2011/06/07 17:17:54, James Hawkins wrote:
> > > This section is not displaying tabular data; please don't use tables.
> > 
> > Is it ok to convert to divs in follow up CL?
> 
> I'd rather it be done in this CL, so we don't lose it.

Done.

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/10015/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:37: if (nextStep >= 0 && nextStep <
steps.length) {
On 2011/06/07 18:13:06, James Hawkins wrote:
> On 2011/06/07 18:02:04, Nikita Kostylev wrote:
> > On 2011/06/07 17:17:54, James Hawkins wrote:
> > > In what situations would |nextStep| be < 0 or >= steps.length?
> > 
> > It shouldn't be, just a sanity check.
> 
> If the backend ever gets into a buggy state where it's sending invalid steps,
> you won't know about it.  I'd remove this check and let the JS 'crash' so you
> have data for that case.

Done.

http://codereview.chromium.org/7058048/diff/15001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/15001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:17: margin-top: -270px;
On 2011/06/07 18:13:06, James Hawkins wrote:
> margin-top after margin-left.

Done.

http://codereview.chromium.org/7058048/diff/15001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:80: height: 1.5em;
On 2011/06/07 18:13:06, James Hawkins wrote:
> Why are you using height instead of proper padding?

Done.

[Evan Stade, 2011-06-08 19:56:07]

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:41: <tr height='40px'>
On 2011/06/07 17:03:36, Nikita Kostylev wrote:
> On 2011/06/07 03:25:46, Evan Stade wrote:
> > do not use height attribute
> 
> At all or move it to css style?

move to css

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:9: function Oobe() {
On 2011/06/07 17:03:36, Nikita Kostylev wrote:
> On 2011/06/07 03:25:46, Evan Stade wrote:
> > this seems wrong.
> 
> You mean when there're no parameters I may as well remove it?

yes

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:32: table {
still needed?

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:42: -webkit-mask:
-webkit-gradient(linear, left top, right top,
-webkit-linear-gradient is simpler

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:27:
i18n-content="updateScreenTitle"></span>
indentation

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:93: <a id="security-link" href="#"
i18n-content="tpmSetting"></a>
I still dont' know what tpm is

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:23: * Current OOBE step, index in the
steps array.
@type {number}

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:50: }
fails silently for |offset| != 1. I would feel better if it were more robust.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:68: $("inner-container").style.height
= $(steps[0]).offsetHeight;
use single quotes in js

[Nikita (slow), 2011-06-08 22:28:27]

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.html:41: <tr height='40px'>
On 2011/06/08 19:56:07, Evan Stade wrote:
> On 2011/06/07 17:03:36, Nikita Kostylev wrote:
> > On 2011/06/07 03:25:46, Evan Stade wrote:
> > > do not use height attribute
> > 
> > At all or move it to css style?
> 
> move to css

Not needed any more.

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:9: function Oobe() {
On 2011/06/08 19:56:07, Evan Stade wrote:
> On 2011/06/07 17:03:36, Nikita Kostylev wrote:
> > On 2011/06/07 03:25:46, Evan Stade wrote:
> > > this seems wrong.
> > 
> > You mean when there're no parameters I may as well remove it?
> 
> yes

Done.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:32: table {
On 2011/06/08 19:56:07, Evan Stade wrote:
> still needed?

No, removed.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:42: -webkit-mask:
-webkit-gradient(linear, left top, right top,
On 2011/06/08 19:56:07, Evan Stade wrote:
> -webkit-linear-gradient is simpler

Converted 4 -webkit-gradient in this file.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:27:
i18n-content="updateScreenTitle"></span>
On 2011/06/08 19:56:07, Evan Stade wrote:
> indentation

James asked me to do 4 spaces instead of aligning to properties, which I had
initially.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:93: <a id="security-link" href="#"
i18n-content="tpmSetting"></a>
On 2011/06/08 19:56:07, Evan Stade wrote:
> I still dont' know what tpm is

It's a Trusted Platform Module. Do you think naming should be different? I might
think something up but it's a nit comment I guess.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:23: * Current OOBE step, index in the
steps array.
On 2011/06/08 19:56:07, Evan Stade wrote:
> @type {number}

Done.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:50: }
On 2011/06/08 19:56:07, Evan Stade wrote:
> fails silently for |offset| != 1. I would feel better if it were more robust.

Actually we'll need to make it work with offset like 2, I've put a TODO.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:68: $("inner-container").style.height
= $(steps[0]).offsetHeight;
On 2011/06/08 19:56:07, Evan Stade wrote:
> use single quotes in js

Done.

[Evan Stade, 2011-06-08 22:46:56]

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:207: #oobe.eula + #tpm {
I don't really think the + is necessary, isn't it the adjacent operator? But IDs
are unique so you could just do #tpm

although, that said, I don't see where it's used.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:27:
i18n-content="updateScreenTitle"></span>
On 2011/06/08 22:28:27, Nikita Kostylev wrote:
> On 2011/06/08 19:56:07, Evan Stade wrote:
> > indentation
> 
> James asked me to do 4 spaces instead of aligning to properties, which I had
> initially.

oh, ok. Then there are some cases down below that need fixing.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:61:
class="eula-frame"></iframe></div>
like here for example

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:93: <a id="security-link" href="#"
i18n-content="tpmSetting"></a>
actually I think the name should be more similar to the grd resource name, which
is apparently very different (IDS_EULA_SYSTEM_SECURITY_SETTING)

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:83: -webkit-transition: all .2s
ease-in-out;
don't use all. specify the property/s

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:54:
setTimeout(function(){oldstep.classList.add('hidden');}, 500);
this is not correct. I don't even think it's necessary, is it?

If you do have a need for it, then please connect to the webkit transition end
event instead of setting a timeout.

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:55: this.currentStep_ += offset;
I guess this is a pretty insignificant nit, but it seems kinda roundabout to
apply the offset instead of just setting = nextStep. The non-intuitiveness made
me go back and check if there was some other magic happening with the value of
offset.

[Nikita (slow), 2011-06-09 15:52:13]

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:9: function Oobe() {
On 2011/06/08 19:56:07, Evan Stade wrote:
> On 2011/06/07 17:03:36, Nikita Kostylev wrote:
> > On 2011/06/07 03:25:46, Evan Stade wrote:
> > > this seems wrong.
> > 
> > You mean when there're no parameters I may as well remove it?
> 
> yes

I've got cr.js:315, Uncaught TypeError: Cannot set property 'getInstance' of
undefined
in function addSingletonGetter(ctor) {
    ctor.getInstance = function() {
      return ctor.instance_ || (ctor.instance_ = new ctor());
    };
  }

Returned back.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:207: #oobe.eula + #tpm {
On 2011/06/08 22:46:56, Evan Stade wrote:
> I don't really think the + is necessary, isn't it the adjacent operator? But
IDs
> are unique so you could just do #tpm
> 
> although, that said, I don't see where it's used.

Updated ID.
security-info div is only visible on EULA step of the flow.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.html (right):

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:27:
i18n-content="updateScreenTitle"></span>
On 2011/06/08 22:46:56, Evan Stade wrote:
> On 2011/06/08 22:28:27, Nikita Kostylev wrote:
> > On 2011/06/08 19:56:07, Evan Stade wrote:
> > > indentation
> > 
> > James asked me to do 4 spaces instead of aligning to properties, which I had
> > initially.
> 
> oh, ok. Then there are some cases down below that need fixing.

Done.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:61:
class="eula-frame"></iframe></div>
On 2011/06/08 22:46:56, Evan Stade wrote:
> like here for example

Done.

http://codereview.chromium.org/7058048/diff/19001/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.html:93: <a id="security-link" href="#"
i18n-content="tpmSetting"></a>
On 2011/06/08 22:46:56, Evan Stade wrote:
> actually I think the name should be more similar to the grd resource name,
which
> is apparently very different (IDS_EULA_SYSTEM_SECURITY_SETTING)

Done.

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:83: -webkit-transition: all .2s
ease-in-out;
On 2011/06/08 22:46:56, Evan Stade wrote:
> don't use all. specify the property/s

Done.

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:54:
setTimeout(function(){oldstep.classList.add('hidden');}, 500);
On 2011/06/08 22:46:56, Evan Stade wrote:
> this is not correct. I don't even think it's necessary, is it?
> 
> If you do have a need for it, then please connect to the webkit transition end
> event instead of setting a timeout.

Done.

http://codereview.chromium.org/7058048/diff/21002/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:55: this.currentStep_ += offset;
On 2011/06/08 22:46:56, Evan Stade wrote:
> I guess this is a pretty insignificant nit, but it seems kinda roundabout to
> apply the offset instead of just setting = nextStep. The non-intuitiveness
made
> me go back and check if there was some other magic happening with the value of
> offset.

You're right, I'll change this. Initially offset was passed as a parameter, but
I forgot to update it here, Done.

[Nikita (slow), 2011-06-09 22:06:43]

Ping?
If you have any comments left, I'd like to iterate on them while being online so
cut 24h feedback loop.

[James Hawkins, 2011-06-09 22:09:59]

LGTM with nit.

http://codereview.chromium.org/7058048/diff/18018/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/18018/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:50: black 50%, rgba(0,0,0,0));
Alignment looks off here.

[Nikita (slow), 2011-06-09 22:30:36]

running trybots now

http://codereview.chromium.org/7058048/diff/18018/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/oobe.css (right):

http://codereview.chromium.org/7058048/diff/18018/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.css:50: black 50%, rgba(0,0,0,0));
On 2011/06/09 22:09:59, James Hawkins wrote:
> Alignment looks off here.

Done.

[Evan Stade, 2011-06-09 22:38:46]

LGTM with this last set of comments addressed

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:9: function Oobe() {
uh, ok then. I guess add a function comment then with @constructor

http://codereview.chromium.org/7058048/diff/22004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:35: } else if (offset == -1) {
again you are using offset here, and twice more below here where you don't need
to. Maybe you should just get rid of offset altogether, and change the checks in
the if and else if to nextStep > this.currentStep_

http://codereview.chromium.org/7058048/diff/22004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:55: 
isn't this newstep?

http://codereview.chromium.org/7058048/diff/22004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:76:
Oobe.getInstance().toggleStep_(next_step);
don't you need to getInstance?

[Nikita (slow), 2011-06-10 09:21:38]

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/oobe.js (right):

http://codereview.chromium.org/7058048/diff/1016/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/oobe.js:9: function Oobe() {
On 2011/06/09 22:38:46, Evan Stade wrote:
> uh, ok then. I guess add a function comment then with @constructor

Done.

http://codereview.chromium.org/7058048/diff/22004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:35: } else if (offset == -1) {
On 2011/06/09 22:38:46, Evan Stade wrote:
> again you are using offset here, and twice more below here where you don't
need
> to. Maybe you should just get rid of offset altogether, and change the checks
in
> the if and else if to nextStep > this.currentStep_

Done.

http://codereview.chromium.org/7058048/diff/22004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:55: 
On 2011/06/09 22:38:46, Evan Stade wrote:
> isn't this newstep?

Done.

http://codereview.chromium.org/7058048/diff/22004/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/oobe.js:76:
Oobe.getInstance().toggleStep_(next_step);
On 2011/06/09 22:38:46, Evan Stade wrote:
> don't you need to getInstance?

No, Oobe.toggleStep() is defined separately and it will call toggleStep_()