Implement newly designed sign-in related histograms for desktop platorms.

Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and signin_metrics::Reason. It implements three new histograms Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and Signin.SigninReason. These histograms could replace Signin.SigninSource histogram with extra capabilities.

https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBGermevJZ0sIlk6ko/edit?usp=sharingusp=sharing contains details.

BUG=532557
Committed: https://crrev.com/71bf6c90880518262efc373771eb621617d09c39
Cr-Commit-Position: refs/heads/master@{#363640}

[gogerald1, 2015-11-26 20:18:40]

Patchset #2 (id:20001) has been deleted

[gogerald1, 2015-11-26 22:07:08]

Patchset #2 (id:40001) has been deleted

[gogerald1, 2015-11-26 22:07:17]

Patchset #2 (id:60001) has been deleted

[gogerald1, 2015-11-26 22:07:31]

Patchset #2 (id:80001) has been deleted

[gogerald1, 2015-11-26 22:07:46]

Patchset #2 (id:100001) has been deleted

[gogerald1, 2015-11-26 22:07:56]

Patchset #2 (id:120001) has been deleted

[gogerald1, 2015-11-26 22:08:08]

Patchset #2 (id:140001) has been deleted

[gogerald1, 2015-11-26 22:08:19]

Patchset #2 (id:160001) has been deleted

[gogerald1, 2015-11-26 22:08:30]

Patchset #2 (id:180001) has been deleted

[gogerald1, 2015-11-26 22:08:58]

Patchset #1 (id:1) has been deleted

[gogerald1, 2015-11-27 18:13:45]

Patchset #1 (id:200001) has been deleted

[gogerald1, 2015-11-27 21:01:23]

Patchset #2 (id:240001) has been deleted

[gogerald1, 2015-11-27 21:01:33]

Patchset #2 (id:260001) has been deleted

[gogerald1, 2015-11-27 21:01:41]

Patchset #2 (id:280001) has been deleted

[gogerald1, 2015-11-27 21:01:53]

Patchset #2 (id:300001) has been deleted

[gogerald1, 2015-11-27 21:08:10]

Description was changed from

==========
draft for debug
draft for debug

BUG=
==========

to

==========
Implement newly designed sign-in related histograms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason as designed in
https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG....


BUG=
==========

[gogerald1, 2015-11-27 21:11:48]

Description was changed from

==========
Implement newly designed sign-in related histograms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason as designed in
https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG....


BUG=
==========

to

==========
Implement newly designed sign-in related histograms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. They replaced Signin.SigninSource histogram.
https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG....
contains details.


BUG=
==========

[gogerald1, 2015-11-27 21:12:22]

Description was changed from

==========
Implement newly designed sign-in related histograms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. They replaced Signin.SigninSource histogram.
https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG....
contains details.


BUG=
==========

to

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. They replaced Signin.SigninSource histogram.
https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG....
contains details.


BUG=
==========

[gogerald1, 2015-11-27 21:13:29]

Description was changed from

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. They replaced Signin.SigninSource histogram.
https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG....
contains details.


BUG=
==========

to

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities.
https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG....
contains details.


BUG=
==========

[gogerald1, 2015-11-27 21:14:01]

Description was changed from

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities.
https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG....
contains details.


BUG=
==========

to

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities. 

https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG...
contains details.


BUG=
==========

[gogerald1, 2015-11-27 21:16:36]

Description was changed from

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities. 

https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG...
contains details.


BUG=
==========

to

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities. 

https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG...
contains details.


BUG=532557
==========

[gogerald1, 2015-11-27 21:41:16]

Patchset #1 (id:220001) has been deleted

[gogerald1, 2015-11-27 21:41:35]

Patchset #1 (id:320001) has been deleted

[gogerald1, 2015-11-27 22:52:00]

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

[gogerald1, 2015-11-27 22:52:01]

Hi Roger,

Please help take look of this CL for newly sign-in related histograms. Will
modify it accordingly after making signin flow modal.

Ganggui,

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

Looks good Ganggui.  A few comments below.

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/principals_private/principals_private_api.cc
(right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/principals_private/principals_private_api.cc:40:
signin_metrics::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN);
This does not seem like the right access point.

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/signin/...
File chrome/browser/signin/signin_global_error.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/signin/...
chrome/browser/signin/signin_global_error.cc:102:
signin_metrics::ACCESS_POINT_SETTINGS);
I don't think this is the right access point.  How is this code path triggered?

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands.cc:1104: void ShowAvatarMenu(Browser*
browser) {
This function should take an access point argument.  Sometimes it is called from
the avatar bubble and sometimes it is called from the menu.

https://code.google.com/p/chromium/codesearch#search/&q=%5CWShowAvatarMenu%20...

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands.cc:1114:
signin_metrics::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN);
This seems to be called only from the menu?

https://code.google.com/p/chromium/codesearch#search/&q=ShowFastUserSwitcher%...

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/app_launcher_login_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/app_launcher_login_handler.cc:163:
chrome::ShowBrowserSignin(browser, signin_metrics::ACCESS_POINT_NTP_LINK);
I wonder if we need the same logic as above.  I know the old code did not do
that, but it could have been wrong.  Can you check if this is only possible from
ntp?

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:74:
signin_metrics::LogSigninAccessPoint(access_point, false);
Instead of a bool second arg, would be better to have an enum with values
STARTED and COMPLETED.  Or maybe even better to have separate functions:

signin_metrics::LogSigninAccessPointStarted(access_point);
signin_metrics::LogSigninAccessPointCompleted(access_point);

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:283:
signin_metrics::LogSigninReason(reason);
By this point, gaia sign in completed successfully, but chrome is not quite
signed in yet.  For example, for an enterprise sign in, the user could still
choose to abort sign in.

I wonder if it is better to log this in SigninManager::OnSignedIn().

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:312: reason ==
signin_metrics::REASON_UNLOCK)
Need { and } for if.

[gogerald1, 2015-12-03 01:18:05]

Patchset #3 (id:380001) has been deleted

[gogerald1, 2015-12-03 01:18:25]

Patchset #3 (id:400001) has been deleted

[gogerald1, 2015-12-03 17:49:04]

Hi Roger,

Thanks for quick review, I rebased this CL to support Minute maid sign in flow
and addressed your comments. Please help take another look.

Ganggui,

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/principals_private/principals_private_api.cc
(right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/principals_private/principals_private_api.cc:40:
signin_metrics::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN);
On 2015/12/02 20:33:48, Roger Tawa wrote:
> This does not seem like the right access point.

Use signin_metrics::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN here since it will bring
out sign in avatar bubble instead of Gaia sign in page as as we discussed.

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/signin/...
File chrome/browser/signin/signin_global_error.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/signin/...
chrome/browser/signin/signin_global_error.cc:102:
signin_metrics::ACCESS_POINT_SETTINGS);
On 2015/12/02 20:33:48, Roger Tawa wrote:
> I don't think this is the right access point.  How is this code path
triggered?

Done.

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands.cc:1104: void ShowAvatarMenu(Browser*
browser) {
On 2015/12/02 20:33:48, Roger Tawa wrote:
> This function should take an access point argument.  Sometimes it is called
from
> the avatar bubble and sometimes it is called from the menu.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=%5CWShowAvatarMenu%20...

Use signin_metrics::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN here since it will bring
out sign in avatar bubble instead of Gaia sign in page as as we discussed.

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/brow...
chrome/browser/ui/browser_commands.cc:1114:
signin_metrics::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN);
On 2015/12/02 20:33:48, Roger Tawa wrote:
> This seems to be called only from the menu?
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=ShowFastUserSwitcher%...

Use signin_metrics::ACCESS_POINT_AVATAR_BUBBLE_SIGN_IN here since it will bring
out sign in avatar bubble instead of Gaia sign in page as as we discussed.

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/app_launcher_login_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/app_launcher_login_handler.cc:163:
chrome::ShowBrowserSignin(browser, signin_metrics::ACCESS_POINT_NTP_LINK);
On 2015/12/02 20:33:48, Roger Tawa wrote:
> I wonder if we need the same logic as above.  I know the old code did not do
> that, but it could have been wrong.  Can you check if this is only possible
from
> ntp?

Yes, we should check it as above, it is an 'advanced' link in sign in promo
bubble of logn in container of a new page.

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:74:
signin_metrics::LogSigninAccessPoint(access_point, false);
On 2015/12/02 20:33:48, Roger Tawa wrote:
> Instead of a bool second arg, would be better to have an enum with values
> STARTED and COMPLETED.  Or maybe even better to have separate functions:
> 
> signin_metrics::LogSigninAccessPointStarted(access_point);
> signin_metrics::LogSigninAccessPointCompleted(access_point);

Done.

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler_impl.cc (right):

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:283:
signin_metrics::LogSigninReason(reason);
On 2015/12/02 20:33:49, Roger Tawa wrote:
> By this point, gaia sign in completed successfully, but chrome is not quite
> signed in yet.  For example, for an enterprise sign in, the user could still
> choose to abort sign in.
> 
> I wonder if it is better to log this in SigninManager::OnSignedIn().

Do it in one_click_signin_sync_starter

https://codereview.chromium.org/1473543002/diff/360001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler_impl.cc:312: reason ==
signin_metrics::REASON_UNLOCK)
On 2015/12/02 20:33:48, Roger Tawa wrote:
> Need { and } for if.

Done.

[Roger Tawa OOO till Jul 10th, 2015-12-03 18:18:07]

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

https://codereview.chromium.org/1473543002/diff/440001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:432: const GURL&
current_url = web_contents()->GetURL();
It seems the web contents could be null sometimes.  I think it might be safer to
pass the access point and reason to the ctor and save them as members.

[gogerald1, 2015-12-03 19:03:19]

Thanks for pointing it out, Update it, not notice that web contents may
disappear after creating 'InlineSigninHelper'.

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

https://codereview.chromium.org/1473543002/diff/440001/chrome/browser/ui/sync...
chrome/browser/ui/sync/one_click_signin_sync_starter.cc:432: const GURL&
current_url = web_contents()->GetURL();
On 2015/12/03 18:18:06, Roger Tawa wrote:
> It seems the web contents could be null sometimes.  I think it might be safer
to
> pass the access point and reason to the ctor and save them as members.

Done.

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

lgtm

Would be good to add a comment that the current_url_ is invalid only in tests.

[gogerald1, 2015-12-03 21:11:49]

gogerald@chromium.org changed reviewers:
+ asvitkine@chromium.org, bauerb@chromium.org, benwells@chromium.org,
rsesek@chromium.org, sky@chromium.org, vandebo@chromium.org

[gogerald1, 2015-12-03 21:11:50]

Hi All,

vandebo@chromium.org: Please review changes in 
chrome/browser/printing/*

asvitkine@chromium.org: Please review changes in 
tools/metrics/histograms/histograms.xml

rsesek@chromium.org: Please review changes in 
chrome/browser/ui/cocoa/*

bauerb@chromium.org: Please review changes in 
chrome/browser/resources/*
chrome/browser/ui/webui/*

benwells@chromium.org: Please review changes in 
chrome/browser/extensions/*
chrome/browser/ui/views/extensions/*

sky@chromium.org: Please review changes in 
chrome/browser/profiles/*
chrome/browser/ui/bookmarks/*
chrome/browser/ui/*
chrome/browser/ui/startup/*
chrome/browser/ui/views/frame/*
chrome/browser/ui/views/profiles/*
chrome/browser/ui/zoom/*
chrome/test/base/*

[benwells, 2015-12-03 22:33:13]

chrome/browser/extensions/*
chrome/browser/ui/views/extensions/*
both lgtm

[sky, 2015-12-03 22:45:28]

I did *not* look at c/b/ui/webui.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/profile...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/profile...
chrome/browser/profiles/host_zoom_map_browsertest.cc:247: GURL test_url =
ConstructTestServerURL(
How about a common function rather than duplicating this code.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_pages.cc:396: browser, GURL(signin::GetPromoURL(
Doesn't GetPromoURL return a GURL?

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:876:
signin_metrics::REASON_SIGNIN_PRIMARY_ACCOUNT, false);
What is meant by PRIMARY and SECONDARY accounts?
StartupBrowserCreatorImpl::AddStartupURLs may be called for multiple profiles.
I'm not sure what signin::ShouldShowPromoAtStartup does though.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:278:
signin_metrics::AccessPoint access_point_;
const

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:102:
signin_metrics::ACCESS_POINT_USER_MANAGER, signin_metrics::REASON_UNLOCK,
Isn't this reauth?

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:82: if (reason ==
signin_metrics::REASON_SIGNIN_PRIMARY_ACCOUNT) {
Isn't SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT now ...SECONDARY_ACCOUNT? Is this change
intentional?

https://codereview.chromium.org/1473543002/diff/480001/components/signin/core...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/1473543002/diff/480001/components/signin/core...
components/signin/core/browser/signin_metrics.h:115: enum AccessPoint {
As you're changing all this around it would be nice if you used enum class so
that you could forward declare these and not have to include the header in all
the .h files.

[vandebo (ex-Chrome), 2015-12-04 05:49:47]

vandebo@chromium.org changed reviewers:
+ thestig@chromium.org
- vandebo@chromium.org

[vandebo (ex-Chrome), 2015-12-04 05:50:16]

+thestig@

[Bernhard Bauer, 2015-12-04 11:06:24]

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/profile...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/profile...
chrome/browser/profiles/host_zoom_map_browsertest.cc:130: GURL
ConstructTestServerURL(const char* url_template) {
This method should probably take a base::StringPiece, so it can work with both a
string constant and a std::string object.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/resourc...
File chrome/browser/resources/options/sync_setup_overlay.js (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/resourc...
chrome/browser/resources/options/sync_setup_overlay.js:844:
SyncSetupOverlay.closeOverlay = function() {
If you are touching this code already, could you replace these with
cr.makePublic()? (See manage_profile_overlay.js for how to do that.) Thanks!

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:202: CHECK(access_point <
signin_metrics::ACCESS_POINT_MAX);
CHECK_LT for slightly nicer error messages.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:304: CHECK(false);
Move the condition into the CHECK? Also, seeing as the CHECK is always compile
in, the only way to reach the return statement below is if
GetValueForKeyInQuery() returns false, so you could return early in that case.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/app_launcher_login_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/app_launcher_login_handler.cc:162: if (browser) {
Return early if |browser| is null?

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/sync_setup_handler.cc:748: if (args != NULL) {
Just do `if (args)` directly.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/sync_setup_handler_unittest.cc:397:
handler_->OpenSyncSetup(NULL);
Use nullptr instead of null.

[Robert Sesek, 2015-12-04 17:32:39]

cocoa/ LGTM

[gogerald1, 2015-12-04 19:08:47]

Patchset #8 (id:520001) has been deleted

[gogerald1, 2015-12-04 19:31:14]

Patchset #8 (id:540001) has been deleted

[gogerald1, 2015-12-04 20:49:09]

Hi Scott and Bernhard,

Thanks for quick review feedback, please take another look.

Ganggui,

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/profile...
File chrome/browser/profiles/host_zoom_map_browsertest.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/profile...
chrome/browser/profiles/host_zoom_map_browsertest.cc:130: GURL
ConstructTestServerURL(const char* url_template) {
On 2015/12/04 11:06:24, Bernhard Bauer wrote:
> This method should probably take a base::StringPiece, so it can work with both
a
> string constant and a std::string object.

Acknowledged. Slightly prefer to use .c_str to convert std::string to c string,
it looks a little simpler and neat here.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/profile...
chrome/browser/profiles/host_zoom_map_browsertest.cc:247: GURL test_url =
ConstructTestServerURL(
On 2015/12/03 22:45:28, sky wrote:
> How about a common function rather than duplicating this code.

Done.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/resourc...
File chrome/browser/resources/options/sync_setup_overlay.js (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/resourc...
chrome/browser/resources/options/sync_setup_overlay.js:844:
SyncSetupOverlay.closeOverlay = function() {
On 2015/12/04 11:06:24, Bernhard Bauer wrote:
> If you are touching this code already, could you replace these with
> cr.makePublic()? (See manage_profile_overlay.js for how to do that.) Thanks!

Done.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:202: CHECK(access_point <
signin_metrics::ACCESS_POINT_MAX);
On 2015/12/04 11:06:24, Bernhard Bauer wrote:
> CHECK_LT for slightly nicer error messages.

Done.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:304: CHECK(false);
On 2015/12/04 11:06:24, Bernhard Bauer wrote:
> Move the condition into the CHECK? Also, seeing as the CHECK is always compile
> in, the only way to reach the return statement below is if
> GetValueForKeyInQuery() returns false, so you could return early in that case.

Done.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/chro...
File chrome/browser/ui/chrome_pages.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/chro...
chrome/browser/ui/chrome_pages.cc:396: browser, GURL(signin::GetPromoURL(
On 2015/12/03 22:45:28, sky wrote:
> Doesn't GetPromoURL return a GURL?

Done. Both the old and new code return a GURL.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/star...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/star...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:876:
signin_metrics::REASON_SIGNIN_PRIMARY_ACCOUNT, false);
On 2015/12/03 22:45:28, sky wrote:
> What is meant by PRIMARY and SECONDARY accounts?
> StartupBrowserCreatorImpl::AddStartupURLs may be called for multiple profiles.
> I'm not sure what signin::ShouldShowPromoAtStartup does though.

PRIMARY means sign in an account as primary account (no signed in account yet).
SECONDARY means add a secondary account to the primary account (when account
consistency is enabled). We should only show promo when there are no signed in
account.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:278:
signin_metrics::AccessPoint access_point_;
On 2015/12/03 22:45:28, sky wrote:
> const

Done.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/user_manager_view.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/user_manager_view.cc:102:
signin_metrics::ACCESS_POINT_USER_MANAGER, signin_metrics::REASON_UNLOCK,
On 2015/12/03 22:45:28, sky wrote:
> Isn't this reauth?

Here we unlocking the profile, the unlock process is an re-authentication
process, but we define the reason to be the purpose of getting the URL from user
point of view, so we say it is "UNLOCK".

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/app_launcher_login_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/app_launcher_login_handler.cc:162: if (browser) {
On 2015/12/04 11:06:24, Bernhard Bauer wrote:
> Return early if |browser| is null?

Done.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/sync_setup_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/sync_setup_handler.cc:748: if (args != NULL) {
On 2015/12/04 11:06:24, Bernhard Bauer wrote:
> Just do `if (args)` directly.

Done.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/sync_setup_handler_unittest.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/sync_setup_handler_unittest.cc:397:
handler_->OpenSyncSetup(NULL);
On 2015/12/04 11:06:24, Bernhard Bauer wrote:
> Use nullptr instead of null.

Done.

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1473543002/diff/480001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:82: if (reason ==
signin_metrics::REASON_SIGNIN_PRIMARY_ACCOUNT) {
On 2015/12/03 22:45:28, sky wrote:
> Isn't SOURCE_AVATAR_BUBBLE_ADD_ACCOUNT now ...SECONDARY_ACCOUNT? Is this
change
> intentional?

Yes, for other reasons there is an email key in the url. In previous code, all
the other sources are signing primary account except "ADD" and "REAUTH".

https://codereview.chromium.org/1473543002/diff/480001/components/signin/core...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/1473543002/diff/480001/components/signin/core...
components/signin/core/browser/signin_metrics.h:115: enum AccessPoint {
On 2015/12/03 22:45:28, sky wrote:
> As you're changing all this around it would be nice if you used enum class so
> that you could forward declare these and not have to include the header in all
> the .h files.

Done.

[Alexei Svitkine (slow), 2015-12-04 20:58:15]

https://codereview.chromium.org/1473543002/diff/560001/components/signin/core...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/1473543002/diff/560001/components/signin/core...
components/signin/core/browser/signin_metrics.cc:37:
UMA_HISTOGRAM_ENUMERATION("Signin.SigninReason", static_cast<int>(reason),
Please document this in histograms.xml.

https://codereview.chromium.org/1473543002/diff/560001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1473543002/diff/560001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44108: <histogram
name="Signin.SigninSource" enum="SigninSource">
Please mark this as <obsolete> given your CL is removing it.

[Lei Zhang, 2015-12-04 21:05:58]

On 2015/12/03 21:11:50, gogerald1 wrote:
> mailto:vandebo@chromium.org: Please review changes in 
> chrome/browser/printing/*

The chrome/browser/printing/ change looks trivial and it looks good, so I'll
just defer to sky@ who can approve all of chrome/.

[sky, 2015-12-04 23:41:59]

LGTM

[gogerald1, 2015-12-05 00:06:13]

Hi Alexei, updated histograms.xml, please help take another look.

https://codereview.chromium.org/1473543002/diff/560001/components/signin/core...
File components/signin/core/browser/signin_metrics.cc (right):

https://codereview.chromium.org/1473543002/diff/560001/components/signin/core...
components/signin/core/browser/signin_metrics.cc:37:
UMA_HISTOGRAM_ENUMERATION("Signin.SigninReason", static_cast<int>(reason),
On 2015/12/04 20:58:15, Alexei Svitkine (slow) wrote:
> Please document this in histograms.xml.

Done.

https://codereview.chromium.org/1473543002/diff/560001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1473543002/diff/560001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44108: <histogram
name="Signin.SigninSource" enum="SigninSource">
On 2015/12/04 20:58:15, Alexei Svitkine (slow) wrote:
> Please mark this as <obsolete> given your CL is removing it.

Done.

[Bernhard Bauer, 2015-12-07 12:14:55]

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/resourc...
File chrome/browser/resources/options/sync_setup_overlay.js (right):

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/resourc...
chrome/browser/resources/options/sync_setup_overlay.js:852: ]);
This line should be indented two spaces less.

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.cc (right):

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:308:
static_cast<signin_metrics::Reason>(reason) >=
Actually, split this up into multiple CHECK/CHECK_GE/CHECK_LT's? Sorry, I didn't
see that initially.

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:64: namespace signin_metrics {
Keep the namespaces ordered alphabetically. Also, do you need to specify an
underlying type?

[gogerald1, 2015-12-07 16:45:30]

Hi Bernhard, update your comments, PTAL.

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/resourc...
File chrome/browser/resources/options/sync_setup_overlay.js (right):

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/resourc...
chrome/browser/resources/options/sync_setup_overlay.js:852: ]);
On 2015/12/07 12:14:54, Bernhard Bauer wrote:
> This line should be indented two spaces less.

Done.

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/signin/...
File chrome/browser/signin/signin_promo.cc (right):

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/signin/...
chrome/browser/signin/signin_promo.cc:308:
static_cast<signin_metrics::Reason>(reason) >=
On 2015/12/07 12:14:54, Bernhard Bauer wrote:
> Actually, split this up into multiple CHECK/CHECK_GE/CHECK_LT's? Sorry, I
didn't
> see that initially.

Done.

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:64: namespace signin_metrics {
On 2015/12/07 12:14:54, Bernhard Bauer wrote:
> Keep the namespaces ordered alphabetically. Also, do you need to specify an
> underlying type?

Done. Specify underlying type to make explicit conversion clear.

[Bernhard Bauer, 2015-12-07 16:49:27]

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/resourc...
File chrome/browser/resources/options/sync_setup_overlay.js (right):

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/resourc...
chrome/browser/resources/options/sync_setup_overlay.js:852: ]);
On 2015/12/07 16:45:29, gogerald1 wrote:
> On 2015/12/07 12:14:54, Bernhard Bauer wrote:
> > This line should be indented two spaces less.
> 
> Done.

_Two_ spaces less. You removed four, so now it's two more 😃

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_window.h (right):

https://codereview.chromium.org/1473543002/diff/580001/chrome/browser/ui/brow...
chrome/browser/ui/browser_window.h:64: namespace signin_metrics {
On 2015/12/07 16:45:30, gogerald1 wrote:
> On 2015/12/07 12:14:54, Bernhard Bauer wrote:
> > Keep the namespaces ordered alphabetically. Also, do you need to specify an
> > underlying type?
> 
> Done. Specify underlying type to make explicit conversion clear.

Well, do we need to repeat this everywhere this enum is forward-declared? It
just seems somewhat redundant, as any place that actually deals with it probably
needs the full definition anyway.

[Bernhard Bauer, 2015-12-07 16:52:35]

Also, please make sure that chromium-reviews@ is on CC.

(If `git cl upload` didn't add it automatically, check its configuration.)

[gogerald1, 2015-12-07 17:47:25]

oops, silly mistake, suggestion accepted, see updates.

[Bernhard Bauer, 2015-12-07 17:56:01]

LGTM, thanks!

[Alexei Svitkine (slow), 2015-12-07 18:13:51]

lgtm % comment

https://codereview.chromium.org/1473543002/diff/620001/components/signin/core...
File components/signin/core/browser/signin_metrics.h (right):

https://codereview.chromium.org/1473543002/diff/620001/components/signin/core...
components/signin/core/browser/signin_metrics.h:108: SOURCE_OTHERS,
SOURCE_OTHERS is now replacing SOURCE_EXTENSION_INSTALL_BUBBLE.

Given that the other two enums here seem to need stable values, it seems weird
not have stable values for the other entry. I would suggest adding a unique
value here, maybe -1? Or some positive number that doesn't overlap with the
other entries.

I would also expand comment to mention that this was previously used in an UMA
histogram (give its name) but is no longer used for UMA. (Assuming this is
true?)

[gogerald1, 2015-12-07 19:38:49]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2015-12-07 19:38:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, benwells@chromium.org, rsesek@chromium.org,
sky@chromium.org, bauerb@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1473543002/#ps640001
(title: " ")

[commit-bot: I haz the power, 2015-12-07 19:40:54]

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

[commit-bot: I haz the power, 2015-12-07 20:59:59]

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

[commit-bot: I haz the power, 2015-12-07 21:00:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gogerald1, 2015-12-07 21:56:41]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2015-12-07 21:56:41]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
benwells@chromium.org, rogerta@chromium.org, sky@chromium.org,
asvitkine@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/1473543002/#ps660001
(title: "format")

[commit-bot: I haz the power, 2015-12-07 22:01:33]

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

[commit-bot: I haz the power, 2015-12-08 00:49:46]

Description was changed from

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities. 

https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG...
contains details.


BUG=532557
==========

to

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities. 

https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG...
contains details.


BUG=532557
==========

[commit-bot: I haz the power, 2015-12-08 00:49:47]

Committed patchset #13 (id:660001)

[commit-bot: I haz the power, 2015-12-08 00:50:53]

Description was changed from

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities. 

https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG...
contains details.


BUG=532557
==========

to

==========
Implement newly designed sign-in related histograms for desktop platorms.

This CL splits signin_metrics::Source into signin_metrics::AccessPoint and
signin_metrics::Reason. It implements three new histograms
Signin.SigninStartedAccessPoint, Signin.SigninCompletedAccessPoint and
Signin.SigninReason. These histograms could replace Signin.SigninSource
histogram with extra capabilities.

https://docs.google.com/a/google.com/document/d/1-gXYAMXXgsJhk6jxO55RuYJ00JBG...
contains details.

BUG=532557

Committed: https://crrev.com/71bf6c90880518262efc373771eb621617d09c39
Cr-Commit-Position: refs/heads/master@{#363640}
==========

[commit-bot: I haz the power, 2015-12-08 00:50:54]

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