Enhanced Sync Confirmation modal

Enhanced Sync Confirmation modal

BUG=595349

[Moe, 2016-03-17 16:08:48]

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

[anthonyvd, 2016-03-17 20:23:41]

Hi Moe, I mostly looked at the C++ but this is on the right track. Some initial
comments.

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/resources/sy...
File chrome/browser/resources/sync_confirmation/sync_confirmation.js (right):

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/resources/sy...
chrome/browser/resources/sync_confirmation/sync_confirmation.js:9:
chrome.send('confirm', [$('activityControlsCheckbox').checked]);
This should also happen in onGoToSettings since that option also confirms sign
in, it just holds off on starting sync until after the user has confirmed
settings.

I imagine we'd want both chrome://settings and the activity page to open in that
case, although you should double check with designers.

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/ui/sync/one_...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/ui/sync/one_...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:401: case
LoginUIService::SYNC_WITH_ACTIVITY_CONTROLS: {
This code is probably changing a bit in
https://codereview.chromium.org/1806193003/. Since this only happens under the
new tab-modal signin flow, and that flow always shows the confirmation dialog,
I'm not sure you'll need this new enum value.

Also, keep in mind that wanting to see activity controls isn't mutually
exclusive with wanting to be taken to settings to configure sync (see my comment
above in sync_confirmation.js)

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/ui/sync/one_...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:613: void
OneClickSigninSyncStarter::StartSync() {
I'd rename this to SetFirstSetupComplete or something similar since it's more
precise. At this point sync is already "starting" but it's waiting on some
confirmation.

https://codereview.chromium.org/1806353002/diff/1/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/1806353002/diff/1/chrome/common/url_constants...
chrome/common/url_constants.h:349: extern const char
kGoogleAccountActivityControlsURL[];
Can you add a comment for this please?

[Moe, 2016-03-18 21:50:09]

Patchset #2 (id:20001) has been deleted

[Moe, 2016-03-18 21:53:40]

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/resources/sy...
File chrome/browser/resources/sync_confirmation/sync_confirmation.js (right):

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/resources/sy...
chrome/browser/resources/sync_confirmation/sync_confirmation.js:9:
chrome.send('confirm', [$('activityControlsCheckbox').checked]);
On 2016/03/17 20:23:41, anthonyvd (OoO until April 2) wrote:
> This should also happen in onGoToSettings since that option also confirms sign
> in, it just holds off on starting sync until after the user has confirmed
> settings.
> 
> I imagine we'd want both chrome://settings and the activity page to open in
that
> case, although you should double check with designers.

Done.

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/ui/sync/one_...
File chrome/browser/ui/sync/one_click_signin_sync_starter.cc (right):

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/ui/sync/one_...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:401: case
LoginUIService::SYNC_WITH_ACTIVITY_CONTROLS: {
On 2016/03/17 20:23:41, anthonyvd (OoO until April 2) wrote:
> This code is probably changing a bit in
> https://codereview.chromium.org/1806193003/. Since this only happens under the
> new tab-modal signin flow, and that flow always shows the confirmation dialog,
> I'm not sure you'll need this new enum value.
> 
> Also, keep in mind that wanting to see activity controls isn't mutually
> exclusive with wanting to be taken to settings to configure sync (see my
comment
> above in sync_confirmation.js)

Acknowledged.

https://codereview.chromium.org/1806353002/diff/1/chrome/browser/ui/sync/one_...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:613: void
OneClickSigninSyncStarter::StartSync() {
On 2016/03/17 20:23:41, anthonyvd (OoO until April 2) wrote:
> I'd rename this to SetFirstSetupComplete or something similar since it's more
> precise. At this point sync is already "starting" but it's waiting on some
> confirmation.

Done.

https://codereview.chromium.org/1806353002/diff/1/chrome/common/url_constants.h
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/1806353002/diff/1/chrome/common/url_constants...
chrome/common/url_constants.h:349: extern const char
kGoogleAccountActivityControlsURL[];
On 2016/03/17 20:23:41, anthonyvd (OoO until April 2) wrote:
> Can you add a comment for this please?

Done.

[anthonyvd, 2016-03-19 19:50:58]

this looks good so far, just a couple comments on my end.

Off to Owners review you go!

https://codereview.chromium.org/1806353002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/1806353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:26:
const int kSyncConfirmationDialogHeight = 511;
Just a heads up to reviewers that I'm making this a dynamic height in
https://codereview.chromium.org/1814513002/ to be landed in a couple weeks when
I come back from vacation.

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

https://codereview.chromium.org/1806353002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:61: const
base::ListValue* args);
I'd parse |args| in the callers and make this just a bool. This way we can call
this function from somewhere that doesn't have a base::ListValue* more easily if
need be.

[Moe, 2016-03-21 20:46:18]

mahmadi@chromium.org changed reviewers:
+ rkaplow@chromium.org, rogerta@chromium.org, sky@chromium.org

[Moe, 2016-03-21 20:47:28]

Hi, Please review my changes for the enhanced sync confirmation screen.

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

https://codereview.chromium.org/1806353002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:61: const
base::ListValue* args);
On 2016/03/19 19:50:58, anthonyvd (OoO until April 2) wrote:
> I'd parse |args| in the callers and make this just a bool. This way we can
call
> this function from somewhere that doesn't have a base::ListValue* more easily
if
> need be.

Done.

[sky, 2016-03-21 22:49:05]

I only looked at c/b/ui/views. That LGTM

[rkaplow, 2016-03-22 14:57:01]

https://codereview.chromium.org/1806353002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1806353002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12675: <action
name="Signin_Signin_WithActivityControlsCheckbox">
user actions generally are related to a specific action a user made.

Would this make more sense as a binary histogram which would record the
open_activity_controls_url state? As it is, there is no easy baseline (i.e. to
see how after open_activity_controls_url==false occurs).

Maybe you could share more what you're trying to learn.

[Moe, 2016-03-22 20:41:32]

https://codereview.chromium.org/1806353002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1806353002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12675: <action
name="Signin_Signin_WithActivityControlsCheckbox">
On 2016/03/22 14:57:01, rkaplow wrote:
> user actions generally are related to a specific action a user made.
> 
> Would this make more sense as a binary histogram which would record the
> open_activity_controls_url state? As it is, there is no easy baseline (i.e. to
> see how after open_activity_controls_url==false occurs).
> 
> Maybe you could share more what you're trying to learn.

I spoke to the PM. We would like to know the number of times the user checks the
checkbox and goes on to complete the signin. So ideally, we would like to record
this action before/after signin_signin_withdefaultsyncsettings and
signin_signin_withadvancedsyncsettings to use in User Action Sequence graphs.
And also, we're not interested in knowing if they simply check/uncheck the
checkbox. I think the way the action is being recorded here achieves this goal.

[Moe, 2016-03-22 20:55:43]

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

[Moe, 2016-03-22 20:56:32]

+estade@

[rkaplow, 2016-03-22 21:10:59]

lgtm

https://codereview.chromium.org/1806353002/diff/60001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1806353002/diff/60001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:12675: <action
name="Signin_Signin_WithActivityControlsCheckbox">
On 2016/03/22 20:41:32, Moe wrote:
> On 2016/03/22 14:57:01, rkaplow wrote:
> > user actions generally are related to a specific action a user made.
> > 
> > Would this make more sense as a binary histogram which would record the
> > open_activity_controls_url state? As it is, there is no easy baseline (i.e.
to
> > see how after open_activity_controls_url==false occurs).
> > 
> > Maybe you could share more what you're trying to learn.
> 
> I spoke to the PM. We would like to know the number of times the user checks
the
> checkbox and goes on to complete the signin. So ideally, we would like to
record
> this action before/after signin_signin_withdefaultsyncsettings and
> signin_signin_withadvancedsyncsettings to use in User Action Sequence graphs.
> And also, we're not interested in knowing if they simply check/uncheck the
> checkbox. I think the way the action is being recorded here achieves this
goal.


I'm not sure I entirely agree. If you want to know how often people are checking
the checkbox, it's much better to have a baseline so you can understand the
number. Absolute counts are generally not as useful since it van vary with the
population - this is why we encourage all metrics to have baselines when
possible, and baselining by the number of times people complete the flow seems
natural.

You can leave the action you added so you can see what percentage of
withdefaultsyncsetting ended with the checkbox being checked for the sequenence
analyzer, but I think it would be better to also have a histogram which measures
what % of the time the checkbox is checked.

[Roger Tawa OOO till Jul 10th, 2016-03-23 01:14:15]

Hi Moe,

Can you add a BUG= to the description?  Thanks.

https://codereview.chromium.org/1806353002/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/1806353002/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:41:
DCHECK(!continue_url_.is_empty() || !activity_controls_url.is_empty());
This DCHECK verifies that both URLs are not empty, but allow any one of them to
be empty.  Is that the desired behaviour?

https://codereview.chromium.org/1806353002/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_observer.h (right):

https://codereview.chromium.org/1806353002/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_observer.h:27: // done.
Can you add docs for the new args.  In particular the new URL.  Is it valid for
it to be empty?  Does it have empty if |continue_url| is not empty?

[Moe, 2016-03-23 20:41:06]

Description was changed from

==========
Enhanced Sync Confirmation modal
==========

to

==========
Enhanced Sync Confirmation modal

BUG=595349
==========

[Moe, 2016-03-24 00:02:41]

https://codereview.chromium.org/1806353002/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_observer.cc (right):

https://codereview.chromium.org/1806353002/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_observer.cc:41:
DCHECK(!continue_url_.is_empty() || !activity_controls_url.is_empty());
On 2016/03/23 01:14:15, Roger Tawa wrote:
> This DCHECK verifies that both URLs are not empty, but allow any one of them
to
> be empty.  Is that the desired behaviour?

Right now, OneClickSigninSyncObserver is used for navigating to the
|continue_url_| once Sync is initialized. My intention is to use it for
navigating to the |activity_controls_url| as well depending on which one is
passed in. Depending on where the observer is created, only one non-empty url is
passed in. I added a check in the if condition below to make sure we don't take
any action if the |continue_url| is empty.

also, in OneClickSigninSyncStarter::OnSyncConfirmationUIClosed web_content is
NULL, that's why I'm passing a Profile* to fetch the ProfileSyncService

https://codereview.chromium.org/1806353002/diff/60001/chrome/browser/ui/sync/...
File chrome/browser/ui/sync/one_click_signin_sync_observer.h (right):

https://codereview.chromium.org/1806353002/diff/60001/chrome/browser/ui/sync/...
chrome/browser/ui/sync/one_click_signin_sync_observer.h:27: // done.
On 2016/03/23 01:14:15, Roger Tawa wrote:
> Can you add docs for the new args.  In particular the new URL.  Is it valid
for
> it to be empty?  Does it have empty if |continue_url| is not empty?

Done.

[Roger Tawa OOO till Jul 10th, 2016-03-24 15:14:43]

lgtm

Thanks Moe.

[Evan Stade, 2016-03-29 22:50:21]

sorry for the delay. Feel free to ping me faster next time (like 24 hours after
I don't respond).

https://codereview.chromium.org/1806353002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/1806353002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:26:
const int kSyncConfirmationDialogHeight = 511;
doesn't seem like this should be hardcoded

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

https://codereview.chromium.org/1806353002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:118:
LoginUIService::SyncConfirmationUIClosedResults results,
nit: SyncConfirmationUIClosedResults made me think this was a struct. It should
be renamed SyncConfirmationUiClosedResult and |results| should be |result| (here
and many places).

Also, not a nit: maybe make this a bitmask, so you are calling with, e.g.,

LoginUIService::CONFIGURE_SYNC_FIRST |
LoginUIService::OPEN_ACTIVITY_CONTROLS_URL

instead of forcing all callers to update, often with a mysterious "false".

[Moe, 2016-03-30 18:07:48]

https://codereview.chromium.org/1806353002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/1806353002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:26:
const int kSyncConfirmationDialogHeight = 511;
On 2016/03/29 22:50:21, Evan Stade wrote:
> doesn't seem like this should be hardcoded

anthonyvd@ is working on making these dynamic. He will resume his work once he's
back from vacation next week.

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

https://codereview.chromium.org/1806353002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/sync_confirmation_handler.cc:118:
LoginUIService::SyncConfirmationUIClosedResults results,
On 2016/03/29 22:50:21, Evan Stade wrote:
> nit: SyncConfirmationUIClosedResults made me think this was a struct. It
should
> be renamed SyncConfirmationUiClosedResult and |results| should be |result|
(here
> and many places).
> 
> Also, not a nit: maybe make this a bitmask, so you are calling with, e.g.,
> 
> LoginUIService::CONFIGURE_SYNC_FIRST |
> LoginUIService::OPEN_ACTIVITY_CONTROLS_URL
> 
> instead of forcing all callers to update, often with a mysterious "false".

Done.

[Evan Stade, 2016-03-30 22:40:48]

https://codereview.chromium.org/1806353002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

https://codereview.chromium.org/1806353002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/login_ui_service.h:46:
DEFAULT_SETTINGS_OPEN_ACTIVITY_CONTROLS_URL = SYNC_WITH_DEFAULT_SETTINGS |
don't add these, instead you must change switch/case statements to if-else
waterfalls

[Moe, 2016-03-31 19:09:29]

https://codereview.chromium.org/1806353002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

https://codereview.chromium.org/1806353002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/login_ui_service.h:46:
DEFAULT_SETTINGS_OPEN_ACTIVITY_CONTROLS_URL = SYNC_WITH_DEFAULT_SETTINGS |
On 2016/03/30 22:40:48, Evan Stade wrote:
> don't add these, instead you must change switch/case statements to if-else
> waterfalls

Done.

[Evan Stade, 2016-03-31 19:30:52]

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

[Evan Stade, 2016-03-31 19:30:53]

+dbeam for css

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

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:11: color:
rgb(51, 103, 214);
i feel like link colors should be coming from a shared location +dbeam

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/login_ui_service.h:38: OPEN_ACTIVITY_CONTROLS_URL
= 1U << 0,
do you need these Us?

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/login_ui_service.h:89: // Called when the sync
settings confirmation UI is closed.
can you add some docs about what result is, now that the type is not as helpful?

[Dan Beam, 2016-03-31 20:13:32]

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

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:11: color:
rgb(51, 103, 214);
On 2016/03/31 19:30:53, Evan Stade wrote:
> i feel like link colors should be coming from a shared location +dbeam

agree generally with estade@

1) where is the same constant also used?
2) is this the same as an existing --google or --paper color?

[Moe, 2016-03-31 21:00:49]

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

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:11: color:
rgb(51, 103, 214);
On 2016/03/31 20:13:32, Dan Beam wrote:
> On 2016/03/31 19:30:53, Evan Stade wrote:
> > i feel like link colors should be coming from a shared location +dbeam
> 
> agree generally with estade@
> 
> 1) where is the same constant also used?
> 2) is this the same as an existing --google or --paper color?

1) this constant is used in one other place for hyperlinks. But in general, we
don't seem to have a shared place for MD pages that styles hyperlinks with
--google, --paper, or any other color. (chrome_shared.css does it but it's not
recommended in MD pages anymore)
2) yes it's the same as --google-blue-700

Something that would work would be to move the rule to <style
is="custom-style"></style> in sync_confirmation.html and use --google-blue-700
instead.

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/login_ui_service.h (right):

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/login_ui_service.h:38: OPEN_ACTIVITY_CONTROLS_URL
= 1U << 0,
On 2016/03/31 19:30:53, Evan Stade wrote:
> do you need these Us?

They're definitely not needed. I thought it's probably a good idea if the number
literals are also unsigned.

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/login_ui_service.h:89: // Called when the sync
settings confirmation UI is closed.
On 2016/03/31 19:30:53, Evan Stade wrote:
> can you add some docs about what result is, now that the type is not as
helpful?

Done.

[Dan Beam, 2016-04-01 18:07:07]

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

https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/sync_confirmation/sync_confirmation.css:11: color:
rgb(51, 103, 214);
On 2016/03/31 21:00:49, Moe wrote:
> On 2016/03/31 20:13:32, Dan Beam wrote:
> > On 2016/03/31 19:30:53, Evan Stade wrote:
> > > i feel like link colors should be coming from a shared location +dbeam
> > 
> > agree generally with estade@
> > 
> > 1) where is the same constant also used?
> > 2) is this the same as an existing --google or --paper color?
> 
> 1) this constant is used in one other place for hyperlinks. But in general, we
> don't seem to have a shared place for MD pages that styles hyperlinks with
> --google, --paper, or any other color. (chrome_shared.css does it but it's not
> recommended in MD pages anymore)

we may not have one /yet/, but it's not for a lack of wanting one:
https://codereview.chromium.org/1840043003/

> 2) yes it's the same as --google-blue-700
> 
> Something that would work would be to move the rule to <style
> is="custom-style"></style> in sync_confirmation.html and use --google-blue-700
> instead.

I'd be the happiest with any way that reduces copying constants, makes code
easier to maintain, and enforces consistency across webui pages.

[Moe, 2016-04-04 13:27:44]

On 2016/04/01 18:07:07, Dan Beam wrote:
>
https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/resourc...
> File chrome/browser/resources/sync_confirmation/sync_confirmation.css (right):
> 
>
https://codereview.chromium.org/1806353002/diff/120001/chrome/browser/resourc...
> chrome/browser/resources/sync_confirmation/sync_confirmation.css:11: color:
> rgb(51, 103, 214);
> On 2016/03/31 21:00:49, Moe wrote:
> > On 2016/03/31 20:13:32, Dan Beam wrote:
> > > On 2016/03/31 19:30:53, Evan Stade wrote:
> > > > i feel like link colors should be coming from a shared location +dbeam
> > > 
> > > agree generally with estade@
> > > 
> > > 1) where is the same constant also used?
> > > 2) is this the same as an existing --google or --paper color?
> > 
> > 1) this constant is used in one other place for hyperlinks. But in general,
we
> > don't seem to have a shared place for MD pages that styles hyperlinks with
> > --google, --paper, or any other color. (chrome_shared.css does it but it's
not
> > recommended in MD pages anymore)
> 
> we may not have one /yet/, but it's not for a lack of wanting one:
> https://codereview.chromium.org/1840043003/
> 
> > 2) yes it's the same as --google-blue-700
> > 
> > Something that would work would be to move the rule to <style
> > is="custom-style"></style> in sync_confirmation.html and use
--google-blue-700
> > instead.
> 
> I'd be the happiest with any way that reduces copying constants, makes code
> easier to maintain, and enforces consistency across webui pages.

Thanks for sharing the CL Dan! I'm especially interested in knowing the
resolution on your comment about the best way to declare shared styles. IMO,
using "custom-style" element in the top level html seems to have advantages over
using regular <style> element. in case of
https://codereview.chromium.org/1840043003/ it will eliminate the need for a new
class and html file.

[Evan Stade, 2016-04-04 22:22:17]

chrome/browser/ui/webui/ lgtm (resources/ stuff I assume Dan is reviewing)

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

https://codereview.chromium.org/1806353002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:60: void
CloseModalSigninWindow(uint32_t result);
nit: comment?

[Moe, 2016-04-04 23:13:06]

Thank you Evan!

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

https://codereview.chromium.org/1806353002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:60: void
CloseModalSigninWindow(uint32_t result);
On 2016/04/04 22:22:17, Evan Stade wrote:
> nit: comment?

Done.

[Moe, 2016-04-04 23:13:06]

Thank you Evan!

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

https://codereview.chromium.org/1806353002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/sync_confirmation_handler.h:60: void
CloseModalSigninWindow(uint32_t result);
On 2016/04/04 22:22:17, Evan Stade wrote:
> nit: comment?

Done.

[Dan Beam, 2016-04-04 23:48:38]

can this UI show on chromium?  if so: we should probably have the chromium logo
as well, no?  i only see a chrome branded logo currently.

https://codereview.chromium.org/1806353002/diff/160001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/one_click_signin_sync_observer.h (right):

https://codereview.chromium.org/1806353002/diff/160001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_observer.h:61: Browser* browser_;
nit: arguably define the ownership here (i.e. // Weak reference.)

https://codereview.chromium.org/1806353002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/1806353002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:26:
const int kSyncConfirmationDialogHeight = 511;
i think hardcoded heights based on localized, dynamic web content is a silly
idea overall, but i guess this modal UI needs some initial size to render at

[Moe, 2016-04-05 17:14:42]

Hi Dan, We had a discussion with the Sync team about the string and and the
logo. The conclusion was that using "Chrome Sync" as the title of the section
and Chrome logo in the Sync confirmation UI in chromium is "ok" since they
signal that user is interacting with Google services.

https://codereview.chromium.org/1806353002/diff/160001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/one_click_signin_sync_observer.h (right):

https://codereview.chromium.org/1806353002/diff/160001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_observer.h:61: Browser* browser_;
On 2016/04/04 23:48:38, Dan Beam wrote:
> nit: arguably define the ownership here (i.e. // Weak reference.)

Done.

https://codereview.chromium.org/1806353002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
(right):

https://codereview.chromium.org/1806353002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc:26:
const int kSyncConfirmationDialogHeight = 511;
On 2016/04/04 23:48:38, Dan Beam wrote:
> i think hardcoded heights based on localized, dynamic web content is a silly
> idea overall, but i guess this modal UI needs some initial size to render at

anthonyvd@ is working on a fix to make these dynamic. his fix should be going in
before the branch point.

[Dan Beam, 2016-04-06 06:39:08]

lgtm

[Moe, 2016-04-06 13:38:02]

Thank you Dan!

[Moe, 2016-04-06 14:12:46]

Patchset #10 (id:200001) has been deleted

[Evan Stade, 2016-04-07 21:38:36]

I just saw this bubble in action and filed crbug.com/601595. I don't think this
should be WebUI.

[Moe, 2016-04-07 22:27:10]

On 2016/04/07 21:38:36, Evan Stade wrote:
> I just saw this bubble in action and filed crbug.com/601595. I don't think
this
> should be WebUI.

@evan, I'll CC the PM on the bug. However, I'm closing this due to design
changes that would make a good chunk of this patch redundant. I'll open another
CL. Thanks for the reviews everyone!