Adding client code for new desktop First Run Experience.

Adding client code for new desktop First Run Experience.

BUG=618454
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/e3f9c38e402ea28ea28cab54059ac52897ecc1b5
Cr-Commit-Position: refs/heads/master@{#418232}

[tmartino, 2016-08-17 14:47:17]

Description was changed from

==========
Adding client code for new desktop First Run Experience.

BUG=618454
==========

to

==========
Adding client code for new desktop First Run Experience.

BUG=618454
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tmartino, 2016-08-17 15:50:07]

tmartino@chromium.org changed reviewers:
+ mahmadi@chromium.org

[tmartino, 2016-08-17 15:50:07]

Sending to Moe for a local pass.

[Moe, 2016-08-19 17:10:53]

Hey tommy, it usually helps to include screenshots with your UI patches. I
personally use imgur. Feel free to send this back to me for further review.

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.css (right):

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:1: /* Copyright (c) 2016 The
Chromium Authors. All rights reserved.
you can probably inline this entire file in welcome.html inside <style
is="custom-style">

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:14: color: rgba(0,0,0,.87);
why color with opacity? is there no grey color close to this?

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:17: font: 14px/24px Roboto,
'Helvetica Neue', 'Lucida Grande', sans-serif;
use chrome://resources/css/text_defaults_md.css if possible.

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:140: background: rgba(0,0,0,.2);
space after commas

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:168: background: rgb(66, 133, 244);
is this a standard google color? you're expected to reuse colors in:
polymer/v1_0/components-chromium/paper-styles/color.html for the most part.

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:169: color: #fff;
white

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:170: font-size: 12px;
if your WebUI page is in the content area. it should be responsive to font size
changes. a fixed font size is probably a bad idea. look at c/b/r/settings for
ideas.

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:195: background: rgba(0,0,0,.24);
rgba(0, 0, 0, .24);

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:31: <p class="signin-description"
i18n-content="descriptionText"></p>
it's preferred to use $i18n{...} instead of i18n-*. see examples under
md_user_manager

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:40: <script
src="chrome://resources/js/i18n_template.js"></script>
you should be able to get rid of this after converting i18n-* stuff.

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
ui/webui/resources/webui_resources.grd:190: file="images/google_logo.svg"
type="BINDATA" />
This should probably be 'product_logo.svg'

[tmartino, 2016-08-19 20:11:29]

Hey Moe, thanks for the great feedback. I've made all the changes you suggested,
except in places where I replied.

Screenshot: http://i.imgur.com/qwz5fFS.png

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.css (right):

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:1: /* Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/08/19 at 17:10:52, moe wrote:
> you can probably inline this entire file in welcome.html inside <style
is="custom-style">

Is that the preferred way of doing it? It seemed like separating css/html/js
into their own files was more common,

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:168: background: rgb(66, 133, 244);
On 2016/08/19 at 17:10:52, moe wrote:
> is this a standard google color? you're expected to reuse colors in:
polymer/v1_0/components-chromium/paper-styles/color.html for the most part.

Yeah, it is, it's Google Blue 500. I actually tried including that file, but I
wasn't able to get it to respect the variables for some reason. Is there
something I need to do other than
<link rel="import" href=[that url]> in the .html and background-color:
var(--google-blue-500) in the CSS? A JS helper or the like?

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
ui/webui/resources/webui_resources.grd:190: file="images/google_logo.svg"
type="BINDATA" />
On 2016/08/19 at 17:10:52, moe wrote:
> This should probably be 'product_logo.svg'

Hmm? That's a different image. This is for a Google wordmark at the bottom of
the page.

[Moe, 2016-08-19 20:40:45]

On 2016/08/19 20:11:29, tmartino wrote:
> Hey Moe, thanks for the great feedback. I've made all the changes you
suggested,
> except in places where I replied.
> 
> Screenshot: http://i.imgur.com/qwz5fFS.png
> 
>
https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/welcome/welcome.css (right):
> 
>
https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/welcome/welcome.css:1: /* Copyright (c) 2016 The
> Chromium Authors. All rights reserved.
> On 2016/08/19 at 17:10:52, moe wrote:
> > you can probably inline this entire file in welcome.html inside <style
> is="custom-style">
> 
> Is that the preferred way of doing it? It seemed like separating css/html/js
> into their own files was more common,
> 
>
https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/welcome/welcome.css:168: background: rgb(66, 133,
244);
> On 2016/08/19 at 17:10:52, moe wrote:
> > is this a standard google color? you're expected to reuse colors in:
> polymer/v1_0/components-chromium/paper-styles/color.html for the most part.
> 
> Yeah, it is, it's Google Blue 500. I actually tried including that file, but I
> wasn't able to get it to respect the variables for some reason. Is there
> something I need to do other than
> <link rel="import" href=[that url]> in the .html and background-color:
> var(--google-blue-500) in the CSS? A JS helper or the like?
> 
>
https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
> File ui/webui/resources/webui_resources.grd (right):
> 
>
https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
> ui/webui/resources/webui_resources.grd:190: file="images/google_logo.svg"
> type="BINDATA" />
> On 2016/08/19 at 17:10:52, moe wrote:
> > This should probably be 'product_logo.svg'
> 
> Hmm? That's a different image. This is for a Google wordmark at the bottom of
> the page.

Thanks tommy. here's some more feedback. maybe also include a screenshot for
Chromium FRE?

[Moe, 2016-08-19 20:40:52]

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.css (right):

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:1: /* Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/08/19 20:11:29, tmartino wrote:
> On 2016/08/19 at 17:10:52, moe wrote:
> > you can probably inline this entire file in welcome.html inside <style
> is="custom-style">
> 
> Is that the preferred way of doing it? It seemed like separating css/html/js
> into their own files was more common,

you can keep the css in a separate file if you want to. but you'd have to have
something like welcome_css.html declare your style in <style is="custom-style">
and then import that in welcome.html. but no css files anymore.

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:168: background: rgb(66, 133, 244);
On 2016/08/19 20:11:29, tmartino wrote:
> On 2016/08/19 at 17:10:52, moe wrote:
> > is this a standard google color? you're expected to reuse colors in:
> polymer/v1_0/components-chromium/paper-styles/color.html for the most part.
> 
> Yeah, it is, it's Google Blue 500. I actually tried including that file, but I
> wasn't able to get it to respect the variables for some reason. Is there
> something I need to do other than
> <link rel="import" href=[that url]> in the .html and background-color:
> var(--google-blue-500) in the CSS? A JS helper or the like?

if these styles are in a <style is='custom-style'></style> and color.html is
imported before, the variables will be respected. Polymer has no way of
replacing these variables when they're used outside of Polymer world.

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
ui/webui/resources/webui_resources.grd:190: file="images/google_logo.svg"
type="BINDATA" />
On 2016/08/19 20:11:29, tmartino wrote:
> On 2016/08/19 at 17:10:52, moe wrote:
> > This should probably be 'product_logo.svg'
> 
> Hmm? That's a different image. This is for a Google wordmark at the bottom of
> the page.

does the FRE in chromium also use the Google wordmark? if yes, this is probably
ok. but 'google' logos/wordmarks in chromium repo raise questions.

[tmartino, 2016-08-19 21:21:57]

Here's a very slightly outdated Chromium screenshot (not using Roboto yet, maybe
some minor spacing/font sizing differences, and the wordmark is a different
color, but otherwise the same):

http://i.imgur.com/wFrzc8S.png

Will reply to the second batch of comments in a minute.

[tmartino, 2016-08-19 22:03:16]

Added a new patchset which inlines the CSS.

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
ui/webui/resources/webui_resources.grd:190: file="images/google_logo.svg"
type="BINDATA" />
On 2016/08/19 at 20:40:52, moe wrote:
> On 2016/08/19 20:11:29, tmartino wrote:
> > On 2016/08/19 at 17:10:52, moe wrote:
> > > This should probably be 'product_logo.svg'
> > 
> > Hmm? That's a different image. This is for a Google wordmark at the bottom
of
> > the page.
> 
> does the FRE in chromium also use the Google wordmark? if yes, this is
probably ok. but 'google' logos/wordmarks in chromium repo raise questions.

Yes, it does. Incidentally, the svg file is already in the Chromium repo; I'm
just adding a handle to it from the WebUI context.

[Moe, 2016-08-22 20:40:52]

It's looking better and better. Just some more comments.

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/2252063002/diff/20001/ui/webui/resources/webu...
ui/webui/resources/webui_resources.grd:190: file="images/google_logo.svg"
type="BINDATA" />
On 2016/08/19 22:03:16, tmartino wrote:
> On 2016/08/19 at 20:40:52, moe wrote:
> > On 2016/08/19 20:11:29, tmartino wrote:
> > > On 2016/08/19 at 17:10:52, moe wrote:
> > > > This should probably be 'product_logo.svg'
> > > 
> > > Hmm? That's a different image. This is for a Google wordmark at the bottom
> of
> > > the page.
> > 
> > does the FRE in chromium also use the Google wordmark? if yes, this is
> probably ok. but 'google' logos/wordmarks in chromium repo raise questions.
> 
> Yes, it does. Incidentally, the svg file is already in the Chromium repo; I'm
> just adding a handle to it from the WebUI context.

Acknowledged.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:17: * {
override these only where needed.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:29: font-size: 87.5%;
Is this intentionally different form the default 81.25% specified in
text_defaults_md.css?

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:31: line-height: 171%;
Why do you need to fiddle with line-height so much? Can't we go with the default
line-height and use margins etc for spacing between elements?

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:37: a {
you probably want a google blue 500 or sth for this. also make sure the hover,
visited, and active states are covered too.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:93: animation: slideUpContent
600ms 1.8s cubic-bezier(.4,.2,0,1) both;
space after commas. here and elsewhere. others will definitely bring this up.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:104: font-size: 243%;
Can you try using em or rem for font size? I find it easier to understand.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:105: letter-spacing: -.44px;
is letter spacing really needed here?

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:128: background: #fff;
white.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:181: .action {
what happens if the buttons get focus?

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:194: font-size: 86%;
these font sizes seem so random. I think it is ok to deviate a little bit from
the mocks if it means more sensible numbers throughout code. (talk to chris and
alan first of course)

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/welcome_ui.cc:53:
html_source->AddResourcePath("logo.png", IDR_PRODUCT_LOGO_128);
I'm not sure whether this is the right resource to use or the ones used here:

https://cs.chromium.org/chromium/src/chrome/browser/resources/md_user_manager...

But either way, you should probably use image sets and a larger image for higher
res displays.

[tmartino, 2016-08-23 18:49:06]

OK, I've taken care of this round of comments as well. For the CSS, I went back
and redid the font sizes using a body default of 100% (16px), which led to MUCH
nicer values. I also switched these to ems per your request, and removed a lot
of line-height values, preferring to set margins instead. In general we now use
ems for all text sizing/spacing, with px used only for images and a few of the
top-level sizing elements (max-widths, page padding, and spacer size).

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:37: a {
On 2016/08/22 at 20:40:52, moe wrote:
> you probably want a google blue 500 or sth for this. also make sure the hover,
visited, and active states are covered too.

Moved to the .link class below. Hover is covered; active it's OK for it to be
undecorated; visited is meaningless in this context.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:93: animation: slideUpContent
600ms 1.8s cubic-bezier(.4,.2,0,1) both;
On 2016/08/22 at 20:40:52, moe wrote:
> space after commas. here and elsewhere. others will definitely bring this up.

Sorry, I totally didn't notice the animations had this problem as well. Done.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:181: .action {
On 2016/08/22 at 20:40:52, moe wrote:
> what happens if the buttons get focus?

The focus/active/hover behavior is all inherited from the paper-button. Focus
appears to bold the text, in this case.

[Moe, 2016-08-23 21:15:31]

lgtm with nit. Thanks tommy.

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2252063002/diff/60001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:181: .action {
On 2016/08/23 18:49:06, tmartino wrote:
> On 2016/08/22 at 20:40:52, moe wrote:
> > what happens if the buttons get focus?
> 
> The focus/active/hover behavior is all inherited from the paper-button. Focus
> appears to bold the text, in this case.

just make sure with Alan/Chris that's the behavior they want. afaik, buttons
should have a tint in focused state. look at md-settings/md-user-manager for
examples.

https://codereview.chromium.org/2252063002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2252063002/diff/80001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.html:11: <script
src="chrome://resources/js/cr.js"></script>
nit: include cr.js and util.js once you need them.

[tmartino, 2016-09-02 18:16:46]

tmartino@chromium.org changed reviewers:
+ michaelpg@chromium.org

[tmartino, 2016-09-02 18:16:46]

+michaelpg for OWNERS review.

[michaelpg, 2016-09-06 22:21:57]

welcome to webui! if I commented on something you've already discussed with moe,
just say "see previous" or something.

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/welcome/welcome.css (right):

https://codereview.chromium.org/2252063002/diff/20001/chrome/browser/resource...
chrome/browser/resources/welcome/welcome.css:1: /* Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/08/19 20:40:52, moe wrote:
> On 2016/08/19 20:11:29, tmartino wrote:
> > On 2016/08/19 at 17:10:52, moe wrote:
> > > you can probably inline this entire file in welcome.html inside <style
> > is="custom-style">
> > 
> > Is that the preferred way of doing it? It seemed like separating css/html/js
> > into their own files was more common,
> 
> you can keep the css in a separate file if you want to. but you'd have to have
> something like welcome_css.html declare your style in <style
is="custom-style">
> and then import that in welcome.html. but no css files anymore.

That's only for Polymer components that need scoped CSS. For normal resources
(including the main HTML file for Polymer-based pages) sticking with .css is
probably best.

https://codereview.chromium.org/2252063002/diff/120001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:1264: <!-- Welcome page (chrome://welcome)
strings -->
all resources in this CL: wrap in <if expr="not chromeos">

https://codereview.chromium.org/2252063002/diff/120001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:1265: <message
name="IDS_WELCOME_UI_WELCOME_HEADER" desc="A message which will appear as the
header on the Welcome UI if the user has never run Chromium before.">
<bikeshed>

We almost never use "_UI_" in string name.

Since this is the same header (just different context for different situations)
I would name them with the same prefixes, e.g.:

IDS_WELCOME_HEADER
IDS_WELCOME_HEADER_AFTER_FIRST_RUN

</bikeshed>

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:1: <html>
start with <!doctype html>

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:14: <style is="custom-style">
i don't think this needs to be an [is="custom-style"], since it's not
Polymer-related. we generally prefer to put CSS in a separate CSS file when
possible.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:17: box-sizing: border-box;
why do you need this?

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:21: font-size: 100%;
Why override 81.25%? Presumably that was selected for a reason.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:23: min-height: 100vh;
why do you need this?

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:24: overflow-y: hidden;
why make this unscrollable? if this is a side effect of min-height, try setting
margin: 0

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:85: text-align: center;
since this is flex, use "align-items: center;" instead

that will center things without affecting text-align, so you won't need to reset
text-align in text.

it also means you won't need "margin: 0 auto" everywhere.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:120: animation: fadeIn 600ms 500ms
both, spin 2.4s cubic-bezier(.4, .2, 0, 1) both;
wrap

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:133: animation: fadeIn 300ms 700ms
both, spin 2.4s cubic-bezier(.4, .2, 0, 1) both;
wrap

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:215: <p
class="heading">$i18n{headerText}</p>
normally we just use <div> instead of using <p> and then zeroing its margins

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:216: <p
class="subheading">$i18n{subheaderText}</p>
<if expr="_google_chrome"> or something like that

rather than having an empty <p> adding space in Chromium builds

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/welcome/welcome.js (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.js:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
no (c) in new copyright declarations

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (left):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:60: #include
"chrome/browser/ui/webui/version_ui.h"
wat

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:24: // web_ui->AddMessageHandler(new
WelcomeHandler());
don't leave commented-out code in (even with TODO. but keeping the TODO itself
is fine)

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:38: base::string16 subheader =
base::string16();
this is equivalent to

    base::string16 subheader;

right?

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:44:
html_source->AddString("subheaderText", subheader);
if you can limit the use of subheaderText to Google Chrome at compile-time, you
can just add the string in the previous block and leave it out entirely
otherwise

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/welcome_ui.h (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
throughout: no (c):
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.h:16: explicit WelcomeUI(content::WebUI*
web_ui, const GURL& url);
omit explicit for multi-arg constructors:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[tmartino, 2016-09-07 23:06:43]

Really helpful comments, thanks! :)

https://codereview.chromium.org/2252063002/diff/120001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:1264: <!-- Welcome page (chrome://welcome)
strings -->
On 2016/09/06 at 22:21:56, michaelpg wrote:
> all resources in this CL: wrap in <if expr="not chromeos">

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:1265: <message
name="IDS_WELCOME_UI_WELCOME_HEADER" desc="A message which will appear as the
header on the Welcome UI if the user has never run Chromium before.">
On 2016/09/06 at 22:21:56, michaelpg wrote:
> <bikeshed>
> 
> We almost never use "_UI_" in string name.
> 
> Since this is the same header (just different context for different
situations) I would name them with the same prefixes, e.g.:
> 
> IDS_WELCOME_HEADER
> IDS_WELCOME_HEADER_AFTER_FIRST_RUN
> 
> </bikeshed>

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:1: <html>
On 2016/09/06 at 22:21:56, michaelpg wrote:
> start with <!doctype html>

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:14: <style is="custom-style">
On 2016/09/06 at 22:21:56, michaelpg wrote:
> i don't think this needs to be an [is="custom-style"], since it's not
Polymer-related. we generally prefer to put CSS in a separate CSS file when
possible.

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:17: box-sizing: border-box;
On 2016/09/06 at 22:21:56, michaelpg wrote:
> why do you need this?

It was keeping the watermark in frame. However, setting the min-height more
precisely (see other comment thread) resolved the issue, so I've removed it.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:21: font-size: 100%;
On 2016/09/06 at 22:21:56, michaelpg wrote:
> Why override 81.25%? Presumably that was selected for a reason.

At normal settings, that 81.25% results in 13pt (as 100% is 16pt). That makes
sense as a default for settings environments, but not for a display page like
this. The design I'm trying to recreate used font sizes like 12, 14, 16, and 34,
and preserving those sizes resulted in hideous percentages relative to 13 (but
mostly nice values relative to 16). Thus, when sizing things relative to that
81.25% value, I ended up with really arbitrary values throughout my CSS.

This is the product of comments Moe made earlier in the thread, and offline
discussions we had, about trying to end up with sensible values throughout the
CSS.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:23: min-height: 100vh;
On 2016/09/06 at 22:21:56, michaelpg wrote:
> why do you need this?

The combination of a min-height here and a max-height in the .spacer class is
what actually causes the content to appear at the proper height. Without this,
the spacer does nothing and the content appears at the top of the frame.

I've changed it to 96 (same as the .spacer min-height) so that the watermark no
longer gets pushed out of frame without the border-box setting.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:24: overflow-y: hidden;
On 2016/09/06 at 22:21:56, michaelpg wrote:
> why make this unscrollable? if this is a side effect of min-height, try
setting margin: 0

Without this, the slide up animation for the content div causes a scrollbar to
flash (as the content, though invisible, may initially be partially offscreen).

The content fits within the viewport without the need for scrolling, even at
quite small window heights.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:85: text-align: center;
On 2016/09/06 at 22:21:56, michaelpg wrote:
> since this is flex, use "align-items: center;" instead
> 
> that will center things without affecting text-align, so you won't need to
reset text-align in text.
> 
> it also means you won't need "margin: 0 auto" everywhere.

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:120: animation: fadeIn 600ms 500ms
both, spin 2.4s cubic-bezier(.4, .2, 0, 1) both;
On 2016/09/06 at 22:21:56, michaelpg wrote:
> wrap

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:133: animation: fadeIn 300ms 700ms
both, spin 2.4s cubic-bezier(.4, .2, 0, 1) both;
On 2016/09/06 at 22:21:56, michaelpg wrote:
> wrap

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:215: <p
class="heading">$i18n{headerText}</p>
On 2016/09/06 at 22:21:56, michaelpg wrote:
> normally we just use <div> instead of using <p> and then zeroing its margins

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:216: <p
class="subheading">$i18n{subheaderText}</p>
On 2016/09/06 at 22:21:56, michaelpg wrote:
> <if expr="_google_chrome"> or something like that
> 
> rather than having an empty <p> adding space in Chromium builds

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/welcome/welcome.js (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.js:1: // Copyright (c) 2016 The
Chromium Authors. All rights reserved.
On 2016/09/06 at 22:21:56, michaelpg wrote:
> no (c) in new copyright declarations

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (left):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:60: #include
"chrome/browser/ui/webui/version_ui.h"
On 2016/09/06 at 22:21:56, michaelpg wrote:
> wat

Whoops, good catch. Originally had my include as the next line, and must have
accidentally grabbed the prior line when I cut/pasted it to the appropriate
place.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:24: // web_ui->AddMessageHandler(new
WelcomeHandler());
On 2016/09/06 at 22:21:57, michaelpg wrote:
> don't leave commented-out code in (even with TODO. but keeping the TODO itself
is fine)

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:38: base::string16 subheader =
base::string16();
On 2016/09/06 at 22:21:56, michaelpg wrote:
> this is equivalent to
> 
>     base::string16 subheader;
> 
> right?

N/A anymore, but yes.

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:44:
html_source->AddString("subheaderText", subheader);
On 2016/09/06 at 22:21:57, michaelpg wrote:
> if you can limit the use of subheaderText to Google Chrome at compile-time,
you can just add the string in the previous block and leave it out entirely
otherwise

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/welcome_ui.h (right):

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.h:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
On 2016/09/06 at 22:21:57, michaelpg wrote:
> throughout: no (c):
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done

https://codereview.chromium.org/2252063002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.h:16: explicit WelcomeUI(content::WebUI*
web_ui, const GURL& url);
On 2016/09/06 at 22:21:57, michaelpg wrote:
> omit explicit for multi-arg constructors:
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

Done

[michaelpg, 2016-09-09 05:36:26]

looking good, just a few more things

https://codereview.chromium.org/2252063002/diff/140001/chrome/app/chromium_st...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/2252063002/diff/140001/chrome/app/chromium_st...
chrome/app/chromium_strings.grd:1265: <if expr="not chromeos">
and not is_android and not is_ios?

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/welcome/welcome.css (right):

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.css:3: * found in the LICENSE file.
nit: close on line 3
 * found in the LICENSE file. */

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/welcome/welcome.html (right):

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:28: <if expr="_google_chrome">
don't ask me why, but we don't indent <if> normally in html/js, it's always
0-aligned and the contents are indented as if it wasn't there. eg:

<body>
  <section>
    <div></div>
<if>
    <div></div>
</if>
  </section>
</body>

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/welcome/welcome.html:39: <div class="watermark">
</div>

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/welcome_ui.cc (right):

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:19: const char* kEverywhereVariantPath =
"/1";
wrap in an anonymous namespace to prevent global variable creation

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.cc:29: bool is_everywhere_variant =
url.path().compare(kEverywhereVariantPath) == 0;
just use operator==

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/welcome_ui.h (right):

https://codereview.chromium.org/2252063002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/welcome_ui.h:13: 
nit: remove blank line

[tmartino, 2016-09-09 15:34:50]

Thanks Michael! I've taken care of those.

[michaelpg, 2016-09-09 20:53:15]

ok, lgtm assuming it all compiles :-)

[tmartino, 2016-09-09 20:55:22]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 20:56:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 20:59:45]

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

[commit-bot: I haz the power, 2016-09-09 20:59:45]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tmartino, 2016-09-12 16:03:21]

The CQ bit was checked by tmartino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 16:03:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 17:28:48]

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

[commit-bot: I haz the power, 2016-09-12 17:28:49]

Dry run: This issue passed the CQ dry run.

[tmartino, 2016-09-12 17:31:06]

tmartino@chromium.org changed reviewers:
+ pkasting@chromium.org

[tmartino, 2016-09-12 17:31:07]

Thanks Michael!

+pkasting for OWNERS approval on chrome/browser/ui/BUILD.gn
+oshima for OWNERS approval on chrome/app/theme/chrome_unscaled_resources.grd

(All other files are already covered.)

[tmartino, 2016-09-12 17:31:52]

tmartino@chromium.org changed reviewers:
+ oshima@chromium.org

[tmartino, 2016-09-12 17:31:53]

Whoops, second try at adding reviewers :)

+oshima for OWNERS approval on chrome/app/theme/chrome_unscaled_resources.grd

[oshima, 2016-09-12 21:49:40]

lgtm

[Peter Kasting, 2016-09-13 02:36:14]

LGTM

[tmartino, 2016-09-13 14:00:26]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2016-09-13 14:00:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
mahmadi@chromium.org, michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2252063002/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2016-09-13 14:00:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 14:05:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-13 14:07:55]

Description was changed from

==========
Adding client code for new desktop First Run Experience.

BUG=618454
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Adding client code for new desktop First Run Experience.

BUG=618454
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/e3f9c38e402ea28ea28cab54059ac52897ecc1b5
Cr-Commit-Position: refs/heads/master@{#418232}
==========

[commit-bot: I haz the power, 2016-09-13 14:07:56]

Patchset 10 (id:??) landed as
https://crrev.com/e3f9c38e402ea28ea28cab54059ac52897ecc1b5
Cr-Commit-Position: refs/heads/master@{#418232}