Expanding signin histogram coverage.

Improving histogram logging for the signin histogram. 

Creating new histograms for all the ways to get into the sign in flow, including one click sign in. Also making sure all the histogram logging is correct.


BUG=174759
TEST=Try different sign in flows, ensure that appropriate histograms appear in about:histograms. Histograms should be Signin.*Actions.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185407

[jwd, 2013-02-27 20:27:02]

Here's a preliminary version of the code Roger.

[Roger Tawa OOO till Jul 10th, 2013-02-27 20:43:22]

lgtm

https://codereview.chromium.org/12362002/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12362002/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:985: }
To compare the urls, it might be better to do the same as line 995 below:

url.ReplaceComponents(replacements) ==
    GURL(GaiaUrls::GetInstance()->service_login_url())

[jwd, 2013-02-28 15:55:45]

PTAL, added a function to log histograms for all sources.

https://codereview.chromium.org/12362002/diff/1/chrome/browser/ui/sync/one_cl...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12362002/diff/1/chrome/browser/ui/sync/one_cl...
chrome/browser/ui/sync/one_click_signin_helper.cc:985: }
On 2013/02/27 20:43:22, Roger Tawa wrote:
> To compare the urls, it might be better to do the same as line 995 below:
> 
> url.ReplaceComponents(replacements) ==
>     GURL(GaiaUrls::GetInstance()->service_login_url())

Done.

[Roger Tawa OOO till Jul 10th, 2013-02-28 16:32:26]

Looks good.

https://codereview.chromium.org/12362002/diff/5001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12362002/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:345: std::string
histogram_prefix;
var not needed.

[jwd, 2013-02-28 19:16:05]

PTAL, this should be all the logging done.

https://codereview.chromium.org/12362002/diff/5001/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12362002/diff/5001/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:345: std::string
histogram_prefix;
On 2013/02/28 16:32:26, Roger Tawa wrote:
> var not needed.

Done.

[Roger Tawa OOO till Jul 10th, 2013-02-28 20:23:54]

https://codereview.chromium.org/12362002/diff/10001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12362002/diff/10001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:167: SyncPromoUI::Source
source;
Is this extra member needed?  It does not seem used in StartSync.

https://codereview.chromium.org/12362002/diff/10001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

https://codereview.chromium.org/12362002/diff/10001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.h:211: SyncPromoUI::Source
advanced_source_;
Maybe call this member original_source_ ?  Also, it should be initialized in the
ctor.

[jwd, 2013-02-28 20:45:38]

I've tested it, it all appears to be working.

https://codereview.chromium.org/12362002/diff/10001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12362002/diff/10001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.cc:167: SyncPromoUI::Source
source;
On 2013/02/28 20:23:54, Roger Tawa wrote:
> Is this extra member needed?  It does not seem used in StartSync.

Done.

https://codereview.chromium.org/12362002/diff/10001/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper.h (right):

https://codereview.chromium.org/12362002/diff/10001/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper.h:211: SyncPromoUI::Source
advanced_source_;
On 2013/02/28 20:23:54, Roger Tawa wrote:
> Maybe call this member original_source_ ?  Also, it should be initialized in
the
> ctor.

Done.

[Roger Tawa OOO till Jul 10th, 2013-02-28 20:54:27]

lgtm

Thanks Jesse!  One nit below, up to you if want to change or not.

For posterity though, please do fix the title of the CL from "fixing presub" to
something similar to the description.

https://codereview.chromium.org/12362002/diff/6005/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12362002/diff/6005/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:200:
LogOneClickHistogramValue(action);
Nit: would it be better to check:

     action != one_click_signin::HISTOGRAM_MAX

https://codereview.chromium.org/12362002/diff/6005/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:978: switched_to_advanced_ =
false;
Set original_source_ back to unknown.

[jwd, 2013-02-28 21:01:49]

https://codereview.chromium.org/12362002/diff/6005/chrome/browser/ui/sync/one...
File chrome/browser/ui/sync/one_click_signin_helper.cc (right):

https://codereview.chromium.org/12362002/diff/6005/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:200:
LogOneClickHistogramValue(action);
On 2013/02/28 20:54:27, Roger Tawa wrote:
> Nit: would it be better to check:
> 
>      action != one_click_signin::HISTOGRAM_MAX

Ya, that's nicer.
Done.

https://codereview.chromium.org/12362002/diff/6005/chrome/browser/ui/sync/one...
chrome/browser/ui/sync/one_click_signin_helper.cc:978: switched_to_advanced_ =
false;
On 2013/02/28 20:54:27, Roger Tawa wrote:
> Set original_source_ back to unknown.

Done.

[commit-bot: I haz the power, 2013-02-28 21:08:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/12362002/6007

[commit-bot: I haz the power, 2013-03-01 02:10:03]

Change committed as 185407