Implement the new Sync Confirmation dialog on Linux and Windows.

Implement the new Sync Confirmation dialog on Linux and Windows.

This CL mostly implements the sync confirmation dialog that is shown at
the end of the tab modal signin flow described in the associated bug. It
allows the user to make an explicit decision about starting sync before
it's started and before sign in is completed.

TEST=
1. Enable the --enable-password-separated-signin-flow flag in
   chrome://flags and restart Chrome
2. In a non-signed in Chrome Profile, select "Sign in to Chrome" from
   the User Menu.
3. Enter valid credentials to sign in to Chrome, the sign in window
   should close and the sync confirmation window should be shown
4. From that window, clicking "Got it" should close the window and leave
   the user signed in with sync started. Clicking "Undo" should close
   the window and leave the user signed out with sync not started.

BUG=533004
Committed: https://crrev.com/72d9ed4e1e5a1b1c174958dce3d4b8b7a5402e67
Cr-Commit-Position: refs/heads/master@{#371907}

[anthonyvd, 2015-12-03 18:52:11]

anthonyvd@chromium.org changed reviewers:
+ rogerta@chromium.org

[anthonyvd, 2015-12-03 18:52:11]

Hi Roger, can you please take a first pass at this review of the sync
confirmation dialog? Besides some missing iconography, this should be pretty
much functionally complete.

Thanks!

[Roger Tawa OOO till Jul 10th, 2015-12-03 20:33:45]

lgtm, a couple of nits below.

https://codereview.chromium.org/1487283005/diff/20001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/1487283005/diff/20001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:403: }
Nit: use a switch() statement will all the enum values.  That way if new enum
values are added later, the compiler will complain.

https://codereview.chromium.org/1487283005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/signin_view_controller.cc (right):

https://codereview.chromium.org/1487283005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/signin_view_controller.cc:126:
web_view->LoadInitialURL(GURL("chrome://sync-confirmation"));
Use kChromeUISyncConfirmationURL.

[anthonyvd, 2015-12-03 21:48:03]

Thanks Roger, fixed those nits and adding other owners now.

https://codereview.chromium.org/1487283005/diff/20001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/1487283005/diff/20001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:403: }
On 2015/12/03 20:33:45, Roger Tawa wrote:
> Nit: use a switch() statement will all the enum values.  That way if new enum
> values are added later, the compiler will complain.

Done.

https://codereview.chromium.org/1487283005/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/signin_view_controller.cc (right):

https://codereview.chromium.org/1487283005/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/signin_view_controller.cc:126:
web_view->LoadInitialURL(GURL("chrome://sync-confirmation"));
On 2015/12/03 20:33:45, Roger Tawa wrote:
> Use kChromeUISyncConfirmationURL.

Done.

[anthonyvd, 2015-12-03 21:54:58]

anthonyvd@chromium.org changed reviewers:
+ estade@chromium.org, rsesek@chromium.org, sky@chromium.org

[anthonyvd, 2015-12-03 21:54:59]

Hello everyone, this CL adds the tab-modal sync confirmation dialog that is part
of the new tab-modal signin flow. It is the follow up to the CL here:
https://codereview.chromium.org/1413533009/ that implemented the actual modal
flow.

Can you please review the following files:

estade@: c/b/resources, c/b/ui/webui, and ui/webui
rsesek@: c/b/ui/cocoa
sky@: c/b and c/b/ui/views

Thanks a lot for your time and much appreciated feedback :)

[sky, 2015-12-04 00:29:21]

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_icon_util.cc (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:399: bool
GetImageURLWithSize(const GURL& old_url, int size, GURL* new_url) {
I realize you're just moving code here, but I have some comments on this that
follow. Is there test coverage?

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:404: if (components.size()
== 0)
empty

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:413: size_t pos =
old_spec.find(default_size_component);
Seems like you only care about the path component. Can you operate on that?
Also, I think rfind would be safer here.

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_icon_util.h (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.h:102: bool
GetImageURLWithSize(const GURL& old_url, int size, GURL* new_url);
I was confused by 'size' here. ThumbnailSize would be clearer.

[Evan Stade, 2015-12-04 01:46:57]

+dbeam in my stead. Dan much more up to date on WebUI stuff (has it all paged
into memory already)

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: * {
I don't think you should use this selector. Font is inherited so you can set
that just on html. Margin and padding should be added on more specific things.

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:77:
background-image: url(chrome://resources/images/check_circle.svg);
if you make this a relative path it will get inlined as a data url (this is
preferred).

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:133: .link {
I feel like you should be including CSS from some shared file rather than
defining a lot of it specifically for this dialog

[Evan Stade, 2015-12-04 01:47:25]

estade@chromium.org changed reviewers:
+ dbeam@chromium.org

[Evan Stade, 2015-12-04 01:47:25]

+dbeam for real this time

[Robert Sesek, 2015-12-04 17:29:58]

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:397: virtual void
ShowModalSyncConfirmationWindow() = 0;
This should have a comment (all other methods in this class do).

[anthonyvd, 2015-12-04 22:42:07]

Addressed this first batch of comments and added some questions of my own.

Thanks again for your time!

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_icon_util.cc (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:399: bool
GetImageURLWithSize(const GURL& old_url, int size, GURL* new_url) {
On 2015/12/04 00:29:20, sky wrote:
> I realize you're just moving code here, but I have some comments on this that
> follow. Is there test coverage?

Sure, I've added some test coverage. Might as well do it while I'm moving it!

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:404: if (components.size()
== 0)
On 2015/12/04 00:29:20, sky wrote:
> empty

Done.

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.cc:413: size_t pos =
old_spec.find(default_size_component);
On 2015/12/04 00:29:20, sky wrote:
> Seems like you only care about the path component. Can you operate on that?
> Also, I think rfind would be safer here.

Done.

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_avatar_icon_util.h (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_avatar_icon_util.h:102: bool
GetImageURLWithSize(const GURL& old_url, int size, GURL* new_url);
On 2015/12/04 00:29:21, sky wrote:
> I was confused by 'size' here. ThumbnailSize would be clearer.

Done.

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/resource...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: * {
On 2015/12/04 01:46:56, Evan Stade wrote:
> I don't think you should use this selector. Font is inherited so you can set
> that just on html. Margin and padding should be added on more specific things.

Done.

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:77:
background-image: url(chrome://resources/images/check_circle.svg);
On 2015/12/04 01:46:56, Evan Stade wrote:
> if you make this a relative path it will get inlined as a data url (this is
> preferred).

Maybe I'm missing something but I'm unable to make this a relative path. I think
it's because the root is different from the document's URL's root. For example,
"../resources/images/check_circle.svg" and
"../../resources/images/check_circle.svg" both resolve to
"chrome://sync-confirmation/resources/images/check_circle.svg" at runtime.

Is there another way?

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:133: .link {
On 2015/12/04 01:46:56, Evan Stade wrote:
> I feel like you should be including CSS from some shared file rather than
> defining a lot of it specifically for this dialog

Yeah it's the first time I dabble in webUI so I wouldn't be surprised if I'm
doing a few things the wrong way. Any idea which shared file I can take a look
at?

A lot of this styling is pretty unique in the sense that there's no other (to my
knowledge) dialog that looks very similar to this. It's styled to be consistent
with the sign in flow that precedes it, which is actually a server-side
resource. Because of that, I'm not sure there are many things that can be shared
with other webUI but I'd love to use them if there are.

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_window.h:397: virtual void
ShowModalSyncConfirmationWindow() = 0;
On 2015/12/04 17:29:58, Robert Sesek wrote:
> This should have a comment (all other methods in this class do).

Done.

[sky, 2015-12-04 23:47:38]

LGTM

[anthonyvd, 2016-01-06 18:10:26]

Gentle post-holiday ping to get this moving again.

Thanks for your time!

[Robert Sesek, 2016-01-06 19:30:08]

LGTM

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

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:2: <head>
Does this page display internationalized content? If so, you should probably
have a <meta charset="utf-8">

[Dan Beam, 2016-01-06 22:57:58]

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:502: if (url.host()
== "sync-confirmation")
why are you not using a constant here?

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:3: * found in
the LICENSE file.
* found in the LICENSE file. */

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:7: font-family:
'Roboto Regular', 'Helvetica Neue', 'Lucida Grande', sans-serif;
use text_defaults.css instead

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:40:
padding-left: 22px;
RTL

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:54: font-size:
13.5px;
use % for font-size so zoom works

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:86: left: 64px;
RTL

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:108:
border-width: 0;
use `border` shorthand

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:125:
padding-top: 6px;
use shorthand `padding` rule

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:129:
text-transform: uppercase;
is this mimicking material design style?  because if so, you should probably be
using Polymer instead

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

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:1: <html>
<!doctype html> before <html>

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:3: <link
rel="stylesheet" type="text/css" href="sync-confirmation.css"
media="screen"></link>
media="screen" isn't necessary unless you want to strip of all the CSS when
printing or something...

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:3: <link
rel="stylesheet" type="text/css" href="sync-confirmation.css"
media="screen"></link>
type="text/css" probably isn't necessary either

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:7: <script
type="text/javascript" src="sync-confirmation.js"></script>
remove type="text/javascript"

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:21: const int
kProfileImageSize = 128;
can you make this (and the size param to GetImageURLWithThumbnailSize) unsigned?

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:60: DCHECK(browser);
just move this DCHECK() to GetDesktopBrowser()

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:62:
SyncConfirmationUIClosed(LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
indent off

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:63:
browser->window()->CloseModalSigninWindow();
can you do something long the lines of:

  CloseModalSignInWindow(browser,
                         LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);

where

  void CloseModalSignInWindow(Browser* browser, ...) {
    LoginUIServiceFactory::GetForProfile(browser->profile())->
        SyncConfirmationUIClosed(...);
    browser->window()->CloseModalSignInWindow();
  }

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:71:
SyncConfirmationUIClosed(LoginUIService::CONFIGURE_SYNC_FIRST);
indent off

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:79:
SyncConfirmationUIClosed(LoginUIService::ABORT_SIGNIN);
indent off

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:97: }
no curlies

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.h (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:9: #include
"base/values.h"
forward declare base::ListValue instead of including base/values.h

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:18: 
// content::WebUIMessageHandler:

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:25: void
HandleConfirmMessage(const base::ListValue* args);
these should be Handle + MessageName (without "Message" at the end), i.e.
HandleConfirm

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:28: void
HandleGoToSettingsMessage(const base::ListValue* args);
doc comments about what these do, i.e.

  // Handles "confirm" message from the page. No arguments.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:30: void
SetUserImageURL(std::string url);
doc comment, shouldn't this be const std::string& url or a GURL()?

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_ui.h (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_ui.h:9: #include
"content/public/browser/web_ui_data_source.h"
why is this web_ui_data_source.h?  did you mean web_ui.h?

if you're trying to make content::WebUI exist you can just forward declare it
here, i.e.

namespace content {
class WebUI;
}

[anthonyvd, 2016-01-08 22:39:40]

anthonyvd@chromium.org changed reviewers:
+ achuith@chromium.org, treib@chromium.org

[anthonyvd, 2016-01-08 22:39:42]

Hello everyone, thanks a ton for the feedback.

This new patchset addresses most of the comments % a handful of questions of my
own. Could you please take a look?

Also, there were some minor changes in a few previously untouched file. achuith@
and treib@, can you please take a look at the following files:

achuith@: chrome/browser/chromeos/login/*
treib@: chrome/browser/supervised_user/*

Thanks again for your time!

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/1487283005/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:502: if (url.host()
== "sync-confirmation")
On 2016/01/06 22:57:57, Dan Beam wrote:
> why are you not using a constant here?

Slipped through the cracks, thanks.

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:3: * found in
the LICENSE file.
On 2016/01/06 22:57:57, Dan Beam wrote:
> * found in the LICENSE file. */

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:7: font-family:
'Roboto Regular', 'Helvetica Neue', 'Lucida Grande', sans-serif;
On 2016/01/06 22:57:57, Dan Beam wrote:
> use text_defaults.css instead

Acknowledged. Our mocks specify this exact font list to be consistent with the
sign in flow that precedes this dialog. text_defaults.css seem to be setting the
font to something else (at least on my linux machine it uses Ubuntu) and using
roboto.css just falls back to I *think* Times New Roman.

Is text_default.css supposed to be this exact list?

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:40:
padding-left: 22px;
On 2016/01/06 22:57:57, Dan Beam wrote:
> RTL

Not super familiar with this. Is ensuring the same padding on the right enough?
This is in a div that only displays a title.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:54: font-size:
13.5px;
On 2016/01/06 22:57:57, Dan Beam wrote:
> use % for font-size so zoom works

This all happens in a fixed-size tab-modal window, should zoom work? How would
it be triggered?

I don't think any of the other tab-modal dialogs allow zoom either.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:86: left: 64px;
On 2016/01/06 22:57:57, Dan Beam wrote:
> RTL

Again, not super familiar with this. This is a checkmark icon partially
overlapping the profile picture. Would it typically be affected by a RTL
culture?  If so, what's the typical way to handle this?

Thanks for the info :)

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:108:
border-width: 0;
On 2016/01/06 22:57:57, Dan Beam wrote:
> use `border` shorthand

Acknowledged.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:125:
padding-top: 6px;
On 2016/01/06 22:57:57, Dan Beam wrote:
> use shorthand `padding` rule

Acknowledged.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:129:
text-transform: uppercase;
On 2016/01/06 22:57:57, Dan Beam wrote:
> is this mimicking material design style?  because if so, you should probably
be
> using Polymer instead

Thanks for the heads up, I wasn't aware we had access to polymer in Chrome (I
don't do WebUI often ;) ).

Done.

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

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:1: <html>
On 2016/01/06 22:57:57, Dan Beam wrote:
> <!doctype html> before <html>

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:2: <head>
On 2016/01/06 19:30:08, Robert Sesek wrote:
> Does this page display internationalized content? If so, you should probably
> have a <meta charset="utf-8">

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:3: <link
rel="stylesheet" type="text/css" href="sync-confirmation.css"
media="screen"></link>
On 2016/01/06 22:57:57, Dan Beam wrote:
> type="text/css" probably isn't necessary either

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:3: <link
rel="stylesheet" type="text/css" href="sync-confirmation.css"
media="screen"></link>
On 2016/01/06 22:57:57, Dan Beam wrote:
> media="screen" isn't necessary unless you want to strip of all the CSS when
> printing or something...

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:7: <script
type="text/javascript" src="sync-confirmation.js"></script>
On 2016/01/06 22:57:57, Dan Beam wrote:
> remove type="text/javascript"

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:21: const int
kProfileImageSize = 128;
On 2016/01/06 22:57:57, Dan Beam wrote:
> can you make this (and the size param to GetImageURLWithThumbnailSize)
unsigned?

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:60: DCHECK(browser);
On 2016/01/06 22:57:58, Dan Beam wrote:
> just move this DCHECK() to GetDesktopBrowser()

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:62:
SyncConfirmationUIClosed(LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
On 2016/01/06 22:57:57, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:63:
browser->window()->CloseModalSigninWindow();
On 2016/01/06 22:57:57, Dan Beam wrote:
> can you do something long the lines of:
> 
>   CloseModalSignInWindow(browser,
>                          LoginUIService::SYNC_WITH_DEFAULT_SETTINGS);
> 
> where
> 
>   void CloseModalSignInWindow(Browser* browser, ...) {
>     LoginUIServiceFactory::GetForProfile(browser->profile())->
>         SyncConfirmationUIClosed(...);
>     browser->window()->CloseModalSignInWindow();
>   }

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:71:
SyncConfirmationUIClosed(LoginUIService::CONFIGURE_SYNC_FIRST);
On 2016/01/06 22:57:57, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:79:
SyncConfirmationUIClosed(LoginUIService::ABORT_SIGNIN);
On 2016/01/06 22:57:58, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:97: }
On 2016/01/06 22:57:57, Dan Beam wrote:
> no curlies

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.h (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:9: #include
"base/values.h"
On 2016/01/06 22:57:58, Dan Beam wrote:
> forward declare base::ListValue instead of including base/values.h

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:18: 
On 2016/01/06 22:57:58, Dan Beam wrote:
> // content::WebUIMessageHandler:

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:25: void
HandleConfirmMessage(const base::ListValue* args);
On 2016/01/06 22:57:58, Dan Beam wrote:
> these should be Handle + MessageName (without "Message" at the end), i.e.
> HandleConfirm

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:28: void
HandleGoToSettingsMessage(const base::ListValue* args);
On 2016/01/06 22:57:58, Dan Beam wrote:
> doc comments about what these do, i.e.
> 
>   // Handles "confirm" message from the page. No arguments.

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:30: void
SetUserImageURL(std::string url);
On 2016/01/06 22:57:58, Dan Beam wrote:
> doc comment, shouldn't this be const std::string& url or a GURL()?

Done.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/sync_confirmation_ui.h (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_ui.h:9: #include
"content/public/browser/web_ui_data_source.h"
On 2016/01/06 22:57:58, Dan Beam wrote:
> why is this web_ui_data_source.h?  did you mean web_ui.h?
> 
> if you're trying to make content::WebUI exist you can just forward declare it
> here, i.e.
> 
> namespace content {
> class WebUI;
> }

I moved some things around and missed this. It's still required in the .cc so
moved it there.

[Marc Treib, 2016-01-11 09:23:05]

https://codereview.chromium.org/1487283005/diff/100001/chrome/browser/supervi...
File
chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h
(right):

https://codereview.chromium.org/1487283005/diff/100001/chrome/browser/supervi...
chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h:35:
unsigned int GetDesiredImageSideLength() const override;
Using "unsigned" to mean "this will never be negative" is disallowed by the
style guide: https://google.github.io/styleguide/cppguide.html#Integer_Types

[anthonyvd, 2016-01-11 17:05:35]

https://codereview.chromium.org/1487283005/diff/100001/chrome/browser/supervi...
File
chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h
(right):

https://codereview.chromium.org/1487283005/diff/100001/chrome/browser/supervi...
chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h:35:
unsigned int GetDesiredImageSideLength() const override;
On 2016/01/11 09:23:05, Marc Treib wrote:
> Using "unsigned" to mean "this will never be negative" is disallowed by the
> style guide: https://google.github.io/styleguide/cppguide.html#Integer_Types

I took this to be a case where the value we're returning is semantically
unsigned (a size is conceptually >= 0). However, looking at the code in views::,
it seems you're right and sizes are all represented as signed integers.

Reverted this part of the change, thanks!

[Marc Treib, 2016-01-11 17:27:28]

https://codereview.chromium.org/1487283005/diff/100001/chrome/browser/supervi...
File
chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h
(right):

https://codereview.chromium.org/1487283005/diff/100001/chrome/browser/supervi...
chrome/browser/supervised_user/legacy/custodian_profile_downloader_service.h:35:
unsigned int GetDesiredImageSideLength() const override;
On 2016/01/11 17:05:35, anthonyvd wrote:
> On 2016/01/11 09:23:05, Marc Treib wrote:
> > Using "unsigned" to mean "this will never be negative" is disallowed by the
> > style guide: https://google.github.io/styleguide/cppguide.html#Integer_Types
> 
> I took this to be a case where the value we're returning is semantically
> unsigned (a size is conceptually >= 0). However, looking at the code in
views::,
> it seems you're right and sizes are all represented as signed integers.
> 
> Reverted this part of the change, thanks!

Aaand now you don't need my LGTM anymore ;)

[Dan Beam, 2016-01-13 23:45:23]

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

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: font-family:
'Roboto Regular', 'Helvetica Neue', 'Lucida Grande', sans-serif;
did you mean Roboto?  I don't know that 'Roboto Regular' is in roboto.css

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:45:
padding-top: 20px;
padding: 20px 24px;

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:50: font-size:
13px;
px -> em or %

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:82: top: 66px;
RTL

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

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:23: <div
class="sync-message" i18n-values=".innerHTML:syncConfirmationBody"></div>
<div class="sync-message"
    i18n-values=".innerHTML:syncConfirmationBody"></div>

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:25:
<paper-button id="confirmButton"
i18n-content="syncConfirmationConfirmLabel"></paper-button>
<paper-button id="confirmButton"
    i18n-content="syncConfirmationConfirmLabel"></paper-button>

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:26:
<paper-button id="undoButton"
i18n-content="syncConfirmationUndoLabel"></paper-button>
<paper-button id="undoButton"
    i18n-content="syncConfirmationUndoLabel"></paper-button>

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.js (right):

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.js:4: */
 * found in the LICENSE file. */

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:34: void
CloseModalSigninWindow(content::WebUI* web_ui,
nit: instead of putting this in an anonymous namespace and passing in a WebUI,
maybe just make it a private instance method with access to web_ui()

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:48:
AccountTrackerServiceFactory::GetForProfile(profile)->RemoveObserver(this);
is it safe to RemoveObserver() if it was never added?

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.h (right):

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:7: 
nit: #include <string>

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:50: // Sets the
profile picture shown in the dialog to the image at |url|
nit: end with .

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_ui.cc (right):

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_ui.cc:21:
source->OverrideContentSecurityPolicyObjectSrc("object-src *;");
^ I don't really know how CSP works, maybe this should be quickly glanced at by
somebody that does

[anthonyvd, 2016-01-18 21:40:08]

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

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: font-family:
'Roboto Regular', 'Helvetica Neue', 'Lucida Grande', sans-serif;
On 2016/01/13 23:45:22, Dan Beam wrote:
> did you mean Roboto?  I don't know that 'Roboto Regular' is in roboto.css

I did. It looks like I wasn't getting Roboto because it wasn't installed on my
system. Am I doing something wrong or do users have to have it on their systems
for Web UI to be able to display it?

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:45:
padding-top: 20px;
On 2016/01/13 23:45:22, Dan Beam wrote:
> padding: 20px 24px;

Done.

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:50: font-size:
13px;
On 2016/01/13 23:45:22, Dan Beam wrote:
> px -> em or %

Done. Changed the other font-size that were in px as well.

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:82: top: 66px;
On 2016/01/13 23:45:22, Dan Beam wrote:
> RTL

Still not sure what you mean by RTL here. Quoting from my comment on patchset 4:
"This is a checkmark icon partially overlapping the profile picture. Would it
typically be affected by a RTL culture?  If so, what's the typical way to handle
this?"

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

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:23: <div
class="sync-message" i18n-values=".innerHTML:syncConfirmationBody"></div>
On 2016/01/13 23:45:22, Dan Beam wrote:
> <div class="sync-message"
>     i18n-values=".innerHTML:syncConfirmationBody"></div>

Done.

Should the long <link> tags be broken up as well?

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:25:
<paper-button id="confirmButton"
i18n-content="syncConfirmationConfirmLabel"></paper-button>
On 2016/01/13 23:45:22, Dan Beam wrote:
> <paper-button id="confirmButton"
>     i18n-content="syncConfirmationConfirmLabel"></paper-button>

Done.

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:26:
<paper-button id="undoButton"
i18n-content="syncConfirmationUndoLabel"></paper-button>
On 2016/01/13 23:45:22, Dan Beam wrote:
> <paper-button id="undoButton"
>     i18n-content="syncConfirmationUndoLabel"></paper-button>

Done.

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.js (right):

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.js:4: */
On 2016/01/13 23:45:22, Dan Beam wrote:
>  * found in the LICENSE file. */

Done.

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:34: void
CloseModalSigninWindow(content::WebUI* web_ui,
On 2016/01/13 23:45:22, Dan Beam wrote:
> nit: instead of putting this in an anonymous namespace and passing in a WebUI,
> maybe just make it a private instance method with access to web_ui()

Done.

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:48:
AccountTrackerServiceFactory::GetForProfile(profile)->RemoveObserver(this);
On 2016/01/13 23:45:22, Dan Beam wrote:
> is it safe to RemoveObserver() if it was never added?

It looks like it is. ObserverList only performs the erase when it actually finds
the item:
https://code.google.com/p/chromium/codesearch#chromium/src/base/observer_list...

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.h (right):

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:7: 
On 2016/01/13 23:45:22, Dan Beam wrote:
> nit: #include <string>

Done.

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:50: // Sets the
profile picture shown in the dialog to the image at |url|
On 2016/01/13 23:45:22, Dan Beam wrote:
> nit: end with .

Done.

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_ui.cc (right):

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_ui.cc:21:
source->OverrideContentSecurityPolicyObjectSrc("object-src *;");
On 2016/01/13 23:45:22, Dan Beam wrote:
> ^ I don't really know how CSP works, maybe this should be quickly glanced at
by
> somebody that does

I based this code on some other web ui since it was the first time I was
building one. Turns out there's no need to override the CSP in this case.

Done.

[Dan Beam, 2016-01-21 03:57:33]

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:7: font-family:
'Roboto Regular', 'Helvetica Neue', 'Lucida Grande', sans-serif;
On 2016/01/08 22:39:41, anthonyvd wrote:
> On 2016/01/06 22:57:57, Dan Beam wrote:
> > use text_defaults.css instead
> 
> Acknowledged. Our mocks specify this exact font list to be consistent with the
> sign in flow that precedes this dialog. text_defaults.css seem to be setting
the
> font to something else (at least on my linux machine it uses Ubuntu) and using
> roboto.css just falls back to I *think* Times New Roman.
> 
> Is text_default.css supposed to be this exact list?

text_defaults_md.css**

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:40:
padding-left: 22px;
On 2016/01/08 22:39:41, anthonyvd wrote:
> On 2016/01/06 22:57:57, Dan Beam wrote:
> > RTL
> 
> Not super familiar with this. Is ensuring the same padding on the right
enough?
> This is in a div that only displays a title.

Chrome supports right-to-left UI languages like Hebrew and Arabic.  This
generally mirrors the UI when is makes sense.  So, if there's 22px of padding to
the left of a title in LTR, there should probably be 22px to the right of a
title in RTL (because it's as the "start" for the respective directionality). 
In fact, there's a -webkit-padding-start that does exactly that: translate to
padding-left in LTR and padding-right in RTL (effectively).

To check your UI in an RTL language:

Mac: change system language
Windows: probably also system language?
Linux: LANGUAGE=ar ./chrome
ChromeOS: ./chrome --lang=ar

if the tabs don't start at the right of your screen[1][2], you're not in RTL.

[1] http://i.imgur.com/OLVWiyn.png
[2] http://i.imgur.com/8l60GPL.png

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:54: font-size:
13.5px;
On 2016/01/08 22:39:41, anthonyvd wrote:
> On 2016/01/06 22:57:57, Dan Beam wrote:
> > use % for font-size so zoom works
> 
> This all happens in a fixed-size tab-modal window, should zoom work? How would
> it be triggered?
> 
> I don't think any of the other tab-modal dialogs allow zoom either.

oh, sorry: this might actually be a function of scaling when the user changes
the default text size from chrome://settings > Web Content.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:86: left: 64px;
On 2016/01/08 22:39:41, anthonyvd wrote:
> On 2016/01/06 22:57:57, Dan Beam wrote:
> > RTL
> 
> Again, not super familiar with this. This is a checkmark icon partially
> overlapping the profile picture. Would it typically be affected by a RTL
> culture?  If so, what's the typical way to handle this?
> 
> Thanks for the info :)

do you have screenshots?

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

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:23: <div
class="sync-message" i18n-values=".innerHTML:syncConfirmationBody"></div>
On 2016/01/18 21:40:08, anthonyvd wrote:
> On 2016/01/13 23:45:22, Dan Beam wrote:
> > <div class="sync-message"
> >     i18n-values=".innerHTML:syncConfirmationBody"></div>
> 
> Done.
> 
> Should the long <link> tags be broken up as well?

no (because they're URLs)

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: font-family:
'Roboto', 'Helvetica Neue', 'Lucida Grande', sans-serif;
you should still be using text_defaults.css for directionality and sizing (I
think) and just overriding with only 'Roboto'

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.html (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:2: <html>
this should probably be

<html i18n-values="dir:textdirection;lang:language">

like all other pages (generally)[1] for a11y purposes (lang) and RTL (dir).

to load "textdirection" and "language" keys into your loadTimeData, run
webui::SetLoadTimeDataDefaults() from C++[2][3].

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_...
[3]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:4: <meta
charset="utf-8">
why do you need this?

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:7: <link
rel="stylesheet" href="sync-confirmation.css"></link>
- should be _

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:11: <script
src="sync-confirmation.js"></script>
why do these need to be in the head? they will block rendering the page until
they're parsed/run

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:11: <script
src="sync-confirmation.js"></script>
- should be _

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_ui.cc (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_ui.cc:18:
content::WebUIDataSource::Create("sync-confirmation");
please use kChromeUISyncConfirmationHost like everybody else seems to do:
https://code.google.com/p/chromium/codesearch#search/&q=content::WebUIDataSou...

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_ui.cc:22:
source->AddResourcePath("sync-confirmation.js", IDR_SYNC_CONFIRMATION_JS);
replace - with _ in the file names

[anthonyvd, 2016-01-21 22:36:11]

Thanks for the feedback, here's a patchset that addresses it.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:7: font-family:
'Roboto Regular', 'Helvetica Neue', 'Lucida Grande', sans-serif;
On 2016/01/21 03:57:33, Dan Beam wrote:
> On 2016/01/08 22:39:41, anthonyvd wrote:
> > On 2016/01/06 22:57:57, Dan Beam wrote:
> > > use text_defaults.css instead
> > 
> > Acknowledged. Our mocks specify this exact font list to be consistent with
the
> > sign in flow that precedes this dialog. text_defaults.css seem to be setting
> the
> > font to something else (at least on my linux machine it uses Ubuntu) and
using
> > roboto.css just falls back to I *think* Times New Roman.
> > 
> > Is text_default.css supposed to be this exact list?
> 
> text_defaults_md.css**

Ah! That file was only added 6 days ago but it seems to be what I want. Thanks,
done!

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:40:
padding-left: 22px;
On 2016/01/21 03:57:33, Dan Beam wrote:
> On 2016/01/08 22:39:41, anthonyvd wrote:
> > On 2016/01/06 22:57:57, Dan Beam wrote:
> > > RTL
> > 
> > Not super familiar with this. Is ensuring the same padding on the right
> enough?
> > This is in a div that only displays a title.
> 
> Chrome supports right-to-left UI languages like Hebrew and Arabic.  This
> generally mirrors the UI when is makes sense.  So, if there's 22px of padding
to
> the left of a title in LTR, there should probably be 22px to the right of a
> title in RTL (because it's as the "start" for the respective directionality). 
> In fact, there's a -webkit-padding-start that does exactly that: translate to
> padding-left in LTR and padding-right in RTL (effectively).
> 
> To check your UI in an RTL language:
> 
> Mac: change system language
> Windows: probably also system language?
> Linux: LANGUAGE=ar ./chrome
> ChromeOS: ./chrome --lang=ar
> 
> if the tabs don't start at the right of your screen[1][2], you're not in RTL.
> 
> [1] http://i.imgur.com/OLVWiyn.png
> [2] http://i.imgur.com/8l60GPL.png

Thanks for the amazing info. I've switched to -webkit-padding-start.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:54: font-size:
13.5px;
On 2016/01/21 03:57:33, Dan Beam wrote:
> On 2016/01/08 22:39:41, anthonyvd wrote:
> > On 2016/01/06 22:57:57, Dan Beam wrote:
> > > use % for font-size so zoom works
> > 
> > This all happens in a fixed-size tab-modal window, should zoom work? How
would
> > it be triggered?
> > 
> > I don't think any of the other tab-modal dialogs allow zoom either.
> 
> oh, sorry: this might actually be a function of scaling when the user changes
> the default text size from chrome://settings > Web Content.

I see, that's indeed the case. However the problem then becomes that on a Chrome
with a larger text size, the content doesn't fit anymore.

I don't think any other tab modal dialog in Chrome handles different font sizes
like this though, and the Chrome sign in flow that precedes it just uses pixel
values as well. As such, I personally think specifying absolute values is the
way to go. Otherwise the dialog would be unusable anyways, see the screenshot
here:

https://drive.google.com/a/google.com/file/d/0B7c9zMzlLtKoaUpTOGJlUnNtdEU/vie...

WDYT?

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:86: left: 64px;
On 2016/01/21 03:57:33, Dan Beam wrote:
> On 2016/01/08 22:39:41, anthonyvd wrote:
> > On 2016/01/06 22:57:57, Dan Beam wrote:
> > > RTL
> > 
> > Again, not super familiar with this. This is a checkmark icon partially
> > overlapping the profile picture. Would it typically be affected by a RTL
> > culture?  If so, what's the typical way to handle this?
> > 
> > Thanks for the info :)
> 
> do you have screenshots?

Sure, here's what this looks like in RTL. The white-on-blue checkmark is
currently on the same side for both RTL and LTR. Should it be affected by text
direction?

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

https://codereview.chromium.org/1487283005/diff/140001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:23: <div
class="sync-message" i18n-values=".innerHTML:syncConfirmationBody"></div>
On 2016/01/21 03:57:33, Dan Beam wrote:
> On 2016/01/18 21:40:08, anthonyvd wrote:
> > On 2016/01/13 23:45:22, Dan Beam wrote:
> > > <div class="sync-message"
> > >     i18n-values=".innerHTML:syncConfirmationBody"></div>
> > 
> > Done.
> > 
> > Should the long <link> tags be broken up as well?
> 
> no (because they're URLs)

Acknowledged.

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: font-family:
'Roboto', 'Helvetica Neue', 'Lucida Grande', sans-serif;
On 2016/01/21 03:57:33, Dan Beam wrote:
> you should still be using text_defaults.css for directionality and sizing (I
> think) and just overriding with only 'Roboto'

It looks like using text_defaults_md.css does the trick.

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.html (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:2: <html>
On 2016/01/21 03:57:33, Dan Beam wrote:
> this should probably be
> 
> <html i18n-values="dir:textdirection;lang:language">
> 
> like all other pages (generally)[1] for a11y purposes (lang) and RTL (dir).
> 
> to load "textdirection" and "language" keys into your loadTimeData, run
> webui::SetLoadTimeDataDefaults() from C++[2][3].
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_...
> [3]
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Done.

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:4: <meta
charset="utf-8">
On 2016/01/21 03:57:33, Dan Beam wrote:
> why do you need this?

It was pointed to me that it was required for internationalized content. Is it
not the case if "lang" is set like you described in the previous comment?

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:7: <link
rel="stylesheet" href="sync-confirmation.css"></link>
On 2016/01/21 03:57:33, Dan Beam wrote:
> - should be _

Done.

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:11: <script
src="sync-confirmation.js"></script>
On 2016/01/21 03:57:33, Dan Beam wrote:
> why do these need to be in the head? they will block rendering the page until
> they're parsed/run

Done.

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:11: <script
src="sync-confirmation.js"></script>
On 2016/01/21 03:57:33, Dan Beam wrote:
> - should be _

Done.

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_ui.cc (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_ui.cc:18:
content::WebUIDataSource::Create("sync-confirmation");
On 2016/01/21 03:57:33, Dan Beam wrote:
> please use kChromeUISyncConfirmationHost like everybody else seems to do:
>
https://code.google.com/p/chromium/codesearch#search/&q=content::WebUIDataSou...

Done.

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_ui.cc:22:
source->AddResourcePath("sync-confirmation.js", IDR_SYNC_CONFIRMATION_JS);
On 2016/01/21 03:57:33, Dan Beam wrote:
> replace - with _ in the file names

Done.

[Dan Beam, 2016-01-22 01:26:58]

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:86: left: 64px;
On 2016/01/21 22:36:10, anthonyvd wrote:
> On 2016/01/21 03:57:33, Dan Beam wrote:
> > On 2016/01/08 22:39:41, anthonyvd wrote:
> > > On 2016/01/06 22:57:57, Dan Beam wrote:
> > > > RTL
> > > 
> > > Again, not super familiar with this. This is a checkmark icon partially
> > > overlapping the profile picture. Would it typically be affected by a RTL
> > > culture?  If so, what's the typical way to handle this?
> > > 
> > > Thanks for the info :)
> > 
> > do you have screenshots?
> 
> Sure, here's what this looks like in RTL. The white-on-blue checkmark is
> currently on the same side for both RTL and LTR. Should it be affected by text
> direction?

did you mean to paste a link here?

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:37:
padding-right: 24px;
note: this was adding left/right padding at both ends

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.html (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:4: <meta
charset="utf-8">
On 2016/01/21 22:36:11, anthonyvd wrote:
> On 2016/01/21 03:57:33, Dan Beam wrote:
> > why do you need this?
> 
> It was pointed to me that it was required for internationalized content. Is it
> not the case if "lang" is set like you described in the previous comment?

it seems that lang does affect this[1], but it's not clear to me how (seems like
there'd be some recommended charset per lang, but with a default fallback of
utf-8?).  i'm unable to coherently parse this[2], and the w3c agrees with
you[3].

how about this: you add this <meta charset="utf-8"> for now and I'll look into
adding a "network" response header for all webui pages?  a header supposedly
takes precedence over a <meta> tag and would allow us to fix in a more central
place.

[1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta#attr-charset
[2]
https://html.spec.whatwg.org/multipage/syntax.html#determining-the-character-...
[3] https://www.w3.org/International/questions/qa-html-encoding-declarations.en

https://codereview.chromium.org/1487283005/diff/200001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1487283005/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:10921: +        Your bookmarks, history,
passwords and other settings will be synced to your Google Account so you can
use them wherever you use Chrome. You can change this in <ph
name="BEGIN_LINK">&lt;a id="settingsLink" href="#"&gt;Settings&lt;/a&gt;.</ph>
ugh, bummer (using innerHTML for this and stuffing HTML and IDs into
translations :/)

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: font-size:
100%;
wait, why are you doing this?  to override this rule?
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:26: width:
448px;
where are these dimensions from?  do they assume English?

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:30:
-webkit-padding-start: 24px;
this only adds padding on one end (the start)

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:69:
.checkmark-bubble {
are you duplicating <paper-checkbox>?

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:71:
background-image: url(//resources/images/check_circle.svg);
nit: use a local path (i.e
../../../../ui/webui/resources/images/check_circle.svg) to inline instead

* doesn't hit disk (inlined at build time rather than loaded from disk when
requested)
* doesn't require being added to a grd / getting a URL

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:77: left: 64px;
still want to see how this is used

fwiw: blink does not mirror the checkmark, https://jsfiddle.net/actupb3y/

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:57: }
do you have any tests for this handler? content::TestWebUI sure is useful for
this

https://codereview.chromium.org/1487283005/diff/200001/ui/webui/resources/web...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/1487283005/diff/200001/ui/webui/resources/web...
ui/webui/resources/webui_resources.grd:197: file="images/check_circle.svg"
type="BINDATA" />
why are you adding this?

[anthonyvd, 2016-01-22 17:19:57]

I'm currently adding tests to the handler but here are my replies to the
comments in the meantime since a few of them have additional questions.

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/60001/chrome/browser/resource...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:86: left: 64px;
On 2016/01/22 01:26:57, Dan Beam wrote:
> On 2016/01/21 22:36:10, anthonyvd wrote:
> > On 2016/01/21 03:57:33, Dan Beam wrote:
> > > On 2016/01/08 22:39:41, anthonyvd wrote:
> > > > On 2016/01/06 22:57:57, Dan Beam wrote:
> > > > > RTL
> > > > 
> > > > Again, not super familiar with this. This is a checkmark icon partially
> > > > overlapping the profile picture. Would it typically be affected by a RTL
> > > > culture?  If so, what's the typical way to handle this?
> > > > 
> > > > Thanks for the info :)
> > > 
> > > do you have screenshots?
> > 
> > Sure, here's what this looks like in RTL. The white-on-blue checkmark is
> > currently on the same side for both RTL and LTR. Should it be affected by
text
> > direction?
> 
> did you mean to paste a link here?

Well, this is embarassing!

Here it is:
https://drive.google.com/a/google.com/file/d/0B7c9zMzlLtKodVM3QnlaUkhFUUE/vie...

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:37:
padding-right: 24px;
On 2016/01/22 01:26:57, Dan Beam wrote:
> note: this was adding left/right padding at both ends

Acknowledged.

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.html (right):

https://codereview.chromium.org/1487283005/diff/160001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.html:4: <meta
charset="utf-8">
On 2016/01/22 01:26:57, Dan Beam wrote:
> On 2016/01/21 22:36:11, anthonyvd wrote:
> > On 2016/01/21 03:57:33, Dan Beam wrote:
> > > why do you need this?
> > 
> > It was pointed to me that it was required for internationalized content. Is
it
> > not the case if "lang" is set like you described in the previous comment?
> 
> it seems that lang does affect this[1], but it's not clear to me how (seems
like
> there'd be some recommended charset per lang, but with a default fallback of
> utf-8?).  i'm unable to coherently parse this[2], and the w3c agrees with
> you[3].
> 
> how about this: you add this <meta charset="utf-8"> for now and I'll look into
> adding a "network" response header for all webui pages?  a header supposedly
> takes precedence over a <meta> tag and would allow us to fix in a more central
> place.
> 
> [1]
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/meta#attr-charset
> [2]
>
https://html.spec.whatwg.org/multipage/syntax.html#determining-the-character-...
> [3]
https://www.w3.org/International/questions/qa-html-encoding-declarations.en

Sounds good, thanks!

https://codereview.chromium.org/1487283005/diff/200001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1487283005/diff/200001/chrome/app/generated_r...
chrome/app/generated_resources.grd:10921: +        Your bookmarks, history,
passwords and other settings will be synced to your Google Account so you can
use them wherever you use Chrome. You can change this in <ph
name="BEGIN_LINK">&lt;a id="settingsLink" href="#"&gt;Settings&lt;/a&gt;.</ph>
On 2016/01/22 01:26:57, Dan Beam wrote:
> ugh, bummer (using innerHTML for this and stuffing HTML and IDs into
> translations :/)

Agreed. IIRC I built it this way because that's how I've seen it in other
places. Is there a better way?

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: font-size:
100%;
On 2016/01/22 01:26:58, Dan Beam wrote:
> wait, why are you doing this?  to override this rule?
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

Yes, although I should have adjusted my em values instead, my bad!

This goes back to our discussion about font size though. Since increasing the
font size makes the dialog unuseable (since it doesn't resize), should font-size
values be in px instead of em?

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:26: width:
448px;
On 2016/01/22 01:26:58, Dan Beam wrote:
> where are these dimensions from?  do they assume English?

They're from our designer mocks. I don't think they assume English, I believe
the design wants the modal to be always the same size regardless of language.

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:30:
-webkit-padding-start: 24px;
On 2016/01/22 01:26:58, Dan Beam wrote:
> this only adds padding on one end (the start)

Done.

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:69:
.checkmark-bubble {
On 2016/01/22 01:26:58, Dan Beam wrote:
> are you duplicating <paper-checkbox>?

I'm not. This isn't a checkbox, it's the white checkmark-in-blue-bubble that
decorates the profile picture in this screenshot:
https://drive.google.com/corp/drive/u/0/folders/0B7c9zMzlLtKoN2E3Y05NblBFc2s

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:71:
background-image: url(//resources/images/check_circle.svg);
On 2016/01/22 01:26:57, Dan Beam wrote:
> nit: use a local path (i.e
> ../../../../ui/webui/resources/images/check_circle.svg) to inline instead
> 
> * doesn't hit disk (inlined at build time rather than loaded from disk when
> requested)
> * doesn't require being added to a grd / getting a URL
 
background-image: url(../../../../ui/webui/resources/images/check_circle.svg);

doesnt seem to work. The image isn't displayed and inspecting the CSS at runtime
shows the exact same path. Do I need to call something on the web UI to resolve
this or am I doing something else wrong?

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:77: left: 64px;
On 2016/01/22 01:26:58, Dan Beam wrote:
> still want to see how this is used
> 
> fwiw: blink does not mirror the checkmark, https://jsfiddle.net/actupb3y/

See above comment for a screenshot and some explanation. It's not actually a
checkbox, it's just decoration over the profile picture.

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:57: }
On 2016/01/22 01:26:58, Dan Beam wrote:
> do you have any tests for this handler? content::TestWebUI sure is useful for
> this

I'll add some.

https://codereview.chromium.org/1487283005/diff/200001/ui/webui/resources/web...
File ui/webui/resources/webui_resources.grd (right):

https://codereview.chromium.org/1487283005/diff/200001/ui/webui/resources/web...
ui/webui/resources/webui_resources.grd:197: file="images/check_circle.svg"
type="BINDATA" />
On 2016/01/22 01:26:58, Dan Beam wrote:
> why are you adding this?

To use it for the profile picture checkmark, although I might not need it if we
can figure out while the inline path isn't working as discussed in the comment
in sync_confirmation.css

[anthonyvd, 2016-01-26 17:37:25]

Unit tests added. 

Thanks for taking a look at the comments in my previous reply as well!

[Dan Beam, 2016-01-26 19:54:20]

overall: looking good!

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:65: if
(accounts.size() == 0)
nit: if (accounts.empty())

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc
(right):

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:27: const
int kExpectedProfileImageSize = 128;
why are you copying this?

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:53: }
what are any of these doing?  is this just to increase visibility?  if so:

 public:
  using SyncConfirmationHandler::HandleConfirm;

should suffice

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:56: class
SyncConfirmationHandlerTest : public BrowserWithTestWindowTest {
this is actually a browser test (i.e. _unittest.cc -> _browsertest.cc,
unit_tests -> browser_tests)

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:58:
SyncConfirmationHandlerTest() : web_ui_(new content::TestWebUI) {}
why do you need to heap allocate |web_ui_|?

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:71: new
OneClickSigninSyncStarter(
how is this deleted?

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:117: return
new DialogTestBrowserWindow;
who owns this?

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:147:
EXPECT_EQ(1UL, web_ui()->call_data().size());
can you drop the L from all these if it's not necessary?

[anthonyvd, 2016-01-26 22:00:39]

Thanks for taking a look. Comments addressed!

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler.cc (right):

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:65: if
(accounts.size() == 0)
On 2016/01/26 19:54:20, Dan Beam wrote:
> nit: if (accounts.empty())

Done.

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc
(right):

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:27: const
int kExpectedProfileImageSize = 128;
On 2016/01/26 19:54:20, Dan Beam wrote:
> why are you copying this?

It's the value I expect the image dimensions to have. I don't want it to be
public in the handler and I also want the test to break if it changes somewhere
else because that would break the layout of the dialog.

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:53: }
On 2016/01/26 19:54:20, Dan Beam wrote:
> what are any of these doing?  is this just to increase visibility?  if so:
> 
>  public:
>   using SyncConfirmationHandler::HandleConfirm;
> 
> should suffice

Neat, didn't think about that. Thanks!

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:56: class
SyncConfirmationHandlerTest : public BrowserWithTestWindowTest {
On 2016/01/26 19:54:20, Dan Beam wrote:
> this is actually a browser test (i.e. _unittest.cc -> _browsertest.cc,
> unit_tests -> browser_tests)

I think it's actually a BrowserTest. All subclasses of BrowserWithTestWindowTest
are in *_unittests.cc files and the inheritance chain doesn't go through
InProcessBrowserTest.

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:58:
SyncConfirmationHandlerTest() : web_ui_(new content::TestWebUI) {}
On 2016/01/26 19:54:20, Dan Beam wrote:
> why do you need to heap allocate |web_ui_|?

I need it to be destroyed in TearDown before
BrowserWithTestWindowTest::TearDown. Not having it there was an oversight, I
changed it to a scoped_ptr and called reset() in TearDown.

Thanks!

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:71: new
OneClickSigninSyncStarter(
On 2016/01/26 19:54:20, Dan Beam wrote:
> how is this deleted?

Instances of this class delete themselves when they're done.

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:117: return
new DialogTestBrowserWindow;
On 2016/01/26 19:54:20, Dan Beam wrote:
> who owns this?

The test class owns it and BrowserWithTestWindowTest::TearDown() takes care of
cleaning it up.

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:147:
EXPECT_EQ(1UL, web_ui()->call_data().size());
On 2016/01/26 19:54:20, Dan Beam wrote:
> can you drop the L from all these if it's not necessary?

Done.

[Dan Beam, 2016-01-26 22:40:51]

lgtm

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc
(right):

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:27: const
int kExpectedProfileImageSize = 128;
On 2016/01/26 22:00:39, anthonyvd wrote:
> On 2016/01/26 19:54:20, Dan Beam wrote:
> > why are you copying this?
> 
> It's the value I expect the image dimensions to have. I don't want it to be
> public in the handler and I also want the test to break if it changes
somewhere
> else because that would break the layout of the dialog.

i think we generally discourage change detection tests in chrome, but they also
exist, so whatevs

https://codereview.chromium.org/1487283005/diff/240001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler_unittest.cc:56: class
SyncConfirmationHandlerTest : public BrowserWithTestWindowTest {
On 2016/01/26 22:00:39, anthonyvd wrote:
> On 2016/01/26 19:54:20, Dan Beam wrote:
> > this is actually a browser test (i.e. _unittest.cc -> _browsertest.cc,
> > unit_tests -> browser_tests)
> 
> I think it's actually a BrowserTest. All subclasses of
BrowserWithTestWindowTest
> are in *_unittests.cc files and the inheritance chain doesn't go through
> InProcessBrowserTest.

i assume you mean "actually a testing::Test" or "Unit test" -- OK, I didn't know
this doesn't touch InProcessBrowserTests.  _unittest.cc is fine

[achuithb, 2016-01-26 22:49:26]

On 2016/01/08 22:39:42, anthonyvd wrote:
> Hello everyone, thanks a ton for the feedback.
> 
> This new patchset addresses most of the comments % a handful of questions of
my
> own. Could you please take a look?
> 
> Also, there were some minor changes in a few previously untouched file.
achuith@
> and treib@, can you please take a look at the following files:
> 
> achuith@: chrome/browser/chromeos/login/*
> treib@: chrome/browser/supervised_user/*
> 
> Thanks again for your time!

I believe my review is no longer needed - let me know if there's something I
should be looking at.

[anthonyvd, 2016-01-27 14:25:53]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-01-27 14:25:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, rsesek@chromium.org, sky@chromium.org, treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1487283005/#ps260001
(title: "Address unit test comments.")

[commit-bot: I haz the power, 2016-01-27 14:26:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1487283005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1487283005/260001

[commit-bot: I haz the power, 2016-01-27 14:28:57]

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

[commit-bot: I haz the power, 2016-01-27 14:28:58]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[anthonyvd, 2016-01-27 22:38:30]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2016-01-27 22:38:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, sky@chromium.org, rsesek@chromium.org, treib@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1487283005/#ps320001
(title: "Fix Mac/ChromeOS builds and iOS tests.")

[commit-bot: I haz the power, 2016-01-27 22:40:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1487283005/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1487283005/320001

[commit-bot: I haz the power, 2016-01-27 23:06:34]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-01-27 23:07:53]

Description was changed from

==========
Implement the new Sync Confirmation dialog on Linux and Windows.

This CL mostly implements the sync confirmation dialog that is shown at
the end of the tab modal signin flow described in the associated bug. It
allows the user to make an explicit decision about starting sync before
it's started and before sign in is completed.

TEST=
1. Enable the --enable-password-separated-signin-flow flag in
   chrome://flags and restart Chrome
2. In a non-signed in Chrome Profile, select "Sign in to Chrome" from
   the User Menu.
3. Enter valid credentials to sign in to Chrome, the sign in window
   should close and the sync confirmation window should be shown
4. From that window, clicking "Got it" should close the window and leave
   the user signed in with sync started. Clicking "Undo" should close
   the window and leave the user signed out with sync not started.

BUG=533004
==========

to

==========
Implement the new Sync Confirmation dialog on Linux and Windows.

This CL mostly implements the sync confirmation dialog that is shown at
the end of the tab modal signin flow described in the associated bug. It
allows the user to make an explicit decision about starting sync before
it's started and before sign in is completed.

TEST=
1. Enable the --enable-password-separated-signin-flow flag in
   chrome://flags and restart Chrome
2. In a non-signed in Chrome Profile, select "Sign in to Chrome" from
   the User Menu.
3. Enter valid credentials to sign in to Chrome, the sign in window
   should close and the sync confirmation window should be shown
4. From that window, clicking "Got it" should close the window and leave
   the user signed in with sync started. Clicking "Undo" should close
   the window and leave the user signed out with sync not started.

BUG=533004

Committed: https://crrev.com/72d9ed4e1e5a1b1c174958dce3d4b8b7a5402e67
Cr-Commit-Position: refs/heads/master@{#371907}
==========

[commit-bot: I haz the power, 2016-01-27 23:07:55]

Patchset 17 (id:??) landed as
https://crrev.com/72d9ed4e1e5a1b1c174958dce3d4b8b7a5402e67
Cr-Commit-Position: refs/heads/master@{#371907}

[Dan Beam, 2016-03-04 00:26:48]

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):

https://codereview.chromium.org/1487283005/diff/200001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:6: font-size:
100%;
On 2016/01/22 17:19:56, anthonyvd wrote:
> On 2016/01/22 01:26:58, Dan Beam wrote:
> > wait, why are you doing this?  to override this rule?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
> 
> Yes, although I should have adjusted my em values instead, my bad!
> 
> This goes back to our discussion about font size though. Since increasing the
> font size makes the dialog unuseable (since it doesn't resize), should
font-size
> values be in px instead of em?

i think the <body> (default font-size) should be 81.25% (equivalent to 0.8125em)
and anything bigger should multiple up (i.e. 150%)