Cross Device Promo - Main Eligibility Flow

Implement a desktop promo for sign in that targets users who sign in on other devices.

This CL implements:
- the flow that detects if users are eligible for the promo, up to the point of making the remote call to determine if they sync elsewhere
- A rough-out of the code that makes the remote call

This previous CL implemented the calls necessary to mint access tokens:
https://codereview.chromium.org/973953002

Work to do includes: Wiring up against the new Sync Endpoint, and building out any and all UI.

Design Doc for the promo:
https://docs.google.com/document/d/1ZmKjU24zAYuo38zFwJxUwJk_0JJ8b0UohkCGI41QnBI/edit

Design Doc for the new Sync endpoint:
https://docs.google.com/document/d/1pT1lyJLlr9x8kyp1Ns-GlyNySq1Nl0wWRnGX2kQ-pD4/edit

BUG=463611
Committed: https://crrev.com/fea4db61440df5f16ebb287295cb4e9406f17ab4
Cr-Commit-Position: refs/heads/master@{#330732}

[Mike Lerman, 2015-05-11 14:47:00]

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

[Mike Lerman, 2015-05-11 14:47:01]

Hi Anthony,

I'm still building out more tests, but it would be great to start having another
pair of eyes on this. Thanks!

Mike

[anthonyvd, 2015-05-14 18:24:21]

Hi Mike, this looks awesome :)

I'm still looking at it but here's a first batch of comments so I don't keep you
waiting too long!

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

https://codereview.chromium.org/1087933002/diff/230001/components/signin/core...
components/signin/core/browser/signin_metrics.h:147: ELIGIBLE,
nit: Maybe put this one first, this way if more reason for ineligibility are
added in the future, ineligible reasons will always be clumped together in the
histogram.

https://codereview.chromium.org/1087933002/diff/410001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1087933002/diff/410001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1386:
CrossDevicePromoFactory::GetForProfile(last_active)->
Random thought but would there be a way to do that coupling the other way? It
feels like it would make more sense for the promo to observe events instead of
having the ProfileManager make this call but I'm not sure that actually makes
sense so feel free to correct me :)

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:87: SetParameterFromVariation(
nit: can we move those 4 SetParameterFromVariation calls out of the if
condition? It's getting pretty massive.

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:165: Init();
I'm not sure I understand how that state could change between the moment the
promo is constructed and a call to CheckPromoEligibilty.

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:53: bool CheckPromoEligibility();
Should this be private?

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:102: base::TimeDelta
single_account_in_profile_;
It's not super obvious in the implementation that this refers to a TimeDelta.
Maybe rename it to something a little more descriptive.

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_factory.cc (right):

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_factory.cc:61: #if
!defined(OS_CHROMEOS) && !defined(OS_IOS) && !defined(OS_ANDROID)
In the ProfileManager, the call to CrossDevicePromoFactory::GetForProfile is
#ifdef'd for !defined(OS_CHROMEOS) only. Is it because the ProfileManager
doesn't exist on iOS and Android or should if have the same set of #ifdefs?

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:37: 0.1, // 20%
Seems like the comment is off here. Should be 10%

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:39: 1000 * 15, // 15
minutes.
That seems like 15 seconds, not 15 minutes.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:50: return
error.state() == GoogleServiceAuthError::CONNECTION_FAILED ||
There's a GoogleServiceAuthError::IsTransientError method that could maybe
replace this, although it doesn't include REQUEST_CANCELED.

I'd still prefer return error.IsTransientError() || error.state() ==
GoogleServiceAuthError::REQUEST_CANCELED; if we treat REQUEST_CANCELED as
transient.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:148: //    
observer_->OnFetchDeviceActivityFailure();
It seems like the observers could be notified twice. Let's say
source->GetStatus().is_success() at line 108 returns false, response_status
would be net::RESPONSE_CODE_INVALID so if the max retry count was reached, both
the OnFetchDeviceActivityFailure calls would be made.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:169: if
(++fetcher_retries_ < kMaxFetcherRetries &&
Is it expected that fetcher_retries_ represents the retries across all fetches?
I was expecting at most kMaxFetcherRetries retries for each fetch operations but
the way the code is at the moment the count is never reset when a fetch
succeeds.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:190:
IsTransientError(error)) {
nit: I think this condition fits on a single line.

[Mike Lerman, 2015-05-14 20:25:33]

Thanks Anthony!

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

https://codereview.chromium.org/1087933002/diff/230001/components/signin/core...
components/signin/core/browser/signin_metrics.h:147: ELIGIBLE,
On 2015/05/14 18:24:19, anthonyvd wrote:
> nit: Maybe put this one first, this way if more reason for ineligibility are
> added in the future, ineligible reasons will always be clumped together in the
> histogram.

Done.

https://codereview.chromium.org/1087933002/diff/410001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1087933002/diff/410001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1386:
CrossDevicePromoFactory::GetForProfile(last_active)->
On 2015/05/14 18:24:20, anthonyvd wrote:
> Random thought but would there be a way to do that coupling the other way? It
> feels like it would make more sense for the promo to observe events instead of
> having the ProfileManager make this call but I'm not sure that actually makes
> sense so feel free to correct me :)

Hmmm I almost could. The CrossDevicePromo could be a BrowserListObserver, but
then I wouldn't be able to access the previous value of ProfileActiveTime, since
I have no way to control which observer gets fired first. I'd have to store
another copy of something like that date, in another pref.

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:87: SetParameterFromVariation(
On 2015/05/14 18:24:20, anthonyvd wrote:
> nit: can we move those 4 SetParameterFromVariation calls out of the if
> condition? It's getting pretty massive.

how's this?

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:165: Init();
On 2015/05/14 18:24:20, anthonyvd wrote:
> I'm not sure I understand how that state could change between the moment the
> promo is constructed and a call to CheckPromoEligibilty.

It happens all the times in tests ;) Beyond that, you're right: the variations
will never change, and you can only ever opt out, not back in. I'll leave a
comment.

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:53: bool CheckPromoEligibility();
On 2015/05/14 18:24:20, anthonyvd wrote:
> Should this be private?

Done.

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:102: base::TimeDelta
single_account_in_profile_;
On 2015/05/14 18:24:20, anthonyvd wrote:
> It's not super obvious in the implementation that this refers to a TimeDelta.
> Maybe rename it to something a little more descriptive.

How's this?

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_factory.cc (right):

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_factory.cc:61: #if
!defined(OS_CHROMEOS) && !defined(OS_IOS) && !defined(OS_ANDROID)
On 2015/05/14 18:24:20, anthonyvd wrote:
> In the ProfileManager, the call to CrossDevicePromoFactory::GetForProfile is
> #ifdef'd for !defined(OS_CHROMEOS) only. Is it because the ProfileManager
> doesn't exist on iOS and Android or should if have the same set of #ifdefs?

That profilemanager call is within a larger !defined(OS_ANDROID) &&
!defined(OS_IOS) block.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:37: 0.1, // 20%
On 2015/05/14 18:24:20, anthonyvd wrote:
> Seems like the comment is off here. Should be 10%

Done.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:39: 1000 * 15, // 15
minutes.
On 2015/05/14 18:24:20, anthonyvd wrote:
> That seems like 15 seconds, not 15 minutes.

*60. Done.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:50: return
error.state() == GoogleServiceAuthError::CONNECTION_FAILED ||
On 2015/05/14 18:24:20, anthonyvd wrote:
> There's a GoogleServiceAuthError::IsTransientError method that could maybe
> replace this, although it doesn't include REQUEST_CANCELED.
> 
> I'd still prefer return error.IsTransientError() || error.state() ==
> GoogleServiceAuthError::REQUEST_CANCELED; if we treat REQUEST_CANCELED as
> transient.

Didn't know about it! Thanks. Done.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:148: //    
observer_->OnFetchDeviceActivityFailure();
On 2015/05/14 18:24:20, anthonyvd wrote:
> It seems like the observers could be notified twice. Let's say
> source->GetStatus().is_success() at line 108 returns false, response_status
> would be net::RESPONSE_CODE_INVALID so if the max retry count was reached,
both
> the OnFetchDeviceActivityFailure calls would be made.

True. Added some returns.

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:169: if
(++fetcher_retries_ < kMaxFetcherRetries &&
On 2015/05/14 18:24:20, anthonyvd wrote:
> Is it expected that fetcher_retries_ represents the retries across all
fetches?
> I was expecting at most kMaxFetcherRetries retries for each fetch operations
but
> the way the code is at the moment the count is never reset when a fetch
> succeeds.

It is supposed to be across all fetches. The fetches happen one after the other,
so logically are a single operation. That's why we have a single backoff
instance and we InformOfRequest() on the same instance for all fetches. The
count is reset when the whole process kicks off in Start().

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:190:
IsTransientError(error)) {
On 2015/05/14 18:24:20, anthonyvd wrote:
> nit: I think this condition fits on a single line.

I think you're right.

[Mike Lerman, 2015-05-15 14:12:01]

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

[Mike Lerman, 2015-05-15 14:12:02]

I'll add Roger now to start looking at this, now that you've had a first look
over Anthony and didn't find anything major.

[Roger Tawa OOO till Jul 10th, 2015-05-15 16:01:40]

Looks good, a few nits.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:26: // converstion could not
succeed.
typo: converstion

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:29: base::TimeDelta&
local_parameter,
const & for line 28, pointer for out arg on line 29.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:58: VLOG(1) <<
"CrossDevicePromo::CrossDevicePromo.";
nit: DCHECK pointers are good?

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:109:
UMA_HISTOGRAM_BOOLEAN("Signin.XDevicePromo.Initialized", false);
Is it worth making this histogram an enum to differentiate between line 109 and
line 93 and line 116?  How will we use this metric?  Should the UMA call be put
into a static helper in this file?

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:141: MarkPromoInactive();
Stop any fetchers too?

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:177: if
(!signin_manager_->GetAuthenticatedUsername().empty()) {
Use IsAuthenticated() instead of !GetAuthenticatedUsername().empty()

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:237: void
CrossDevicePromo::UpdateLastActiveTime(
nit: should this be called something like UpdateStartLastBrowsingSessionTime(),
or maybe MarkPromoActiveIf()?

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:298: if (rpc_throttle_ &&
base::RandGenerator(100) < rpc_throttle_) {
Should be calculated each time or only once per chrome run?

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:335:
most_recent_last_active.ToInternalValue());
nit: extra space.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:356:
signin_metrics::LogXDevicePromoEligible(signin_metrics::ELIGIBLE);
It seems that not all calls to uma logs are protected with if (track_metrics_). 
What's the reason for that?

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:388: const GoogleServiceAuthError&
error) {
Too many spaces?

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:401:
prefs::kCrossDevicePromoObservedSingleAccountCookie)){
nit: space before {

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:20: public
DeviceActivityFetcher::Observer {
Should have a class description comment.

https://codereview.chromium.org/1087933002/diff/470001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1087933002/diff/470001/chrome/chrome_browser....
chrome/chrome_browser.gypi:839: 'browser/signin/cross_device_promo_factory.h',
If these files are here, I don't think we need #ifdef if the factory code.

https://codereview.chromium.org/1087933002/diff/470001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/470001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:51: error.state() ==
GoogleServiceAuthError::REQUEST_CANCELED;
Hmm, from our discussion this morning :-)  Still not sure if
error.IsTransientError() should handle canceled or not.

[Mike Lerman, 2015-05-15 20:47:35]

Thanks, Roger! Comments incorporated.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:26: // converstion could not
succeed.
On 2015/05/15 16:01:39, Roger Tawa wrote:
> typo: converstion

Done.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:29: base::TimeDelta&
local_parameter,
On 2015/05/15 16:01:40, Roger Tawa wrote:
> const & for line 28, pointer for out arg on line 29.

Done.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:58: VLOG(1) <<
"CrossDevicePromo::CrossDevicePromo.";
On 2015/05/15 16:01:40, Roger Tawa wrote:
> nit: DCHECK pointers are good?

Done.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:109:
UMA_HISTOGRAM_BOOLEAN("Signin.XDevicePromo.Initialized", false);
On 2015/05/15 16:01:40, Roger Tawa wrote:
> Is it worth making this histogram an enum to differentiate between line 109
and
> line 93 and line 116?  How will we use this metric?  Should the UMA call be
put
> into a static helper in this file?

I'll split it into three: OPTED_OUT, NO_VARIATIONS_CONFIG, INITIALIZED.

This will be helpful in helping us figure out the state of people out in the
wild: We'll want to slice the population into what %age opted out, %age that's
signed in, %age that's multi-account... and %age that didn't initialize since
they couldn't get a finch config.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:141: MarkPromoInactive();
On 2015/05/15 16:01:40, Roger Tawa wrote:
> Stop any fetchers too?

Yes. Added a Stop method to the DeviceActivityFetcher.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:177: if
(!signin_manager_->GetAuthenticatedUsername().empty()) {
On 2015/05/15 16:01:40, Roger Tawa wrote:
> Use IsAuthenticated() instead of !GetAuthenticatedUsername().empty()

Done.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:237: void
CrossDevicePromo::UpdateLastActiveTime(
On 2015/05/15 16:01:40, Roger Tawa wrote:
> nit: should this be called something like
UpdateStartLastBrowsingSessionTime(),
> or maybe MarkPromoActiveIf()?

I opted for: MaybeBrowsingSessionStarted

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:298: if (rpc_throttle_ &&
base::RandGenerator(100) < rpc_throttle_) {
On 2015/05/15 16:01:40, Roger Tawa wrote:
> Should be calculated each time or only once per chrome run?

Once per Chrome/per client makes sense, that way we correctly can test for
browser sessions and device activity within a Chrome session. This is done up in
Init().

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:335:
most_recent_last_active.ToInternalValue());
On 2015/05/15 16:01:40, Roger Tawa wrote:
> nit: extra space.

Done.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:356:
signin_metrics::LogXDevicePromoEligible(signin_metrics::ELIGIBLE);
On 2015/05/15 16:01:40, Roger Tawa wrote:
> It seems that not all calls to uma logs are protected with if
(track_metrics_). 
> What's the reason for that?

Ah, I don't need track_metrics_ anymore! It's never set to false.

This was needed because there were flows in CheckPromoEligibility() where I log
an UMA stat and, in the same flow, fire a fetch for DeviceActivity. After a
recent refactoring, there's no scenario where that happens.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:388: const GoogleServiceAuthError&
error) {
On 2015/05/15 16:01:40, Roger Tawa wrote:
> Too many spaces?

Done.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:401:
prefs::kCrossDevicePromoObservedSingleAccountCookie)){
On 2015/05/15 16:01:40, Roger Tawa wrote:
> nit: space before {

Done.

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/470001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:20: public
DeviceActivityFetcher::Observer {
On 2015/05/15 16:01:40, Roger Tawa wrote:
> Should have a class description comment.

Done.

https://codereview.chromium.org/1087933002/diff/470001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1087933002/diff/470001/chrome/chrome_browser....
chrome/chrome_browser.gypi:839: 'browser/signin/cross_device_promo_factory.h',
On 2015/05/15 16:01:40, Roger Tawa wrote:
> If these files are here, I don't think we need #ifdef if the factory code.

Great point! done.

https://codereview.chromium.org/1087933002/diff/470001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/470001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:51: error.state() ==
GoogleServiceAuthError::REQUEST_CANCELED;
On 2015/05/15 16:01:40, Roger Tawa wrote:
> Hmm, from our discussion this morning :-)  Still not sure if
> error.IsTransientError() should handle canceled or not.

That's a good point. With the addition of a Stop() method which called
GaiaAuthFetcher->CancelRequest()... if you trace down, that calls
URLFetcherCore()->CancelURLRequest, which sets the status to REQUEST_CANCELED.
So we'd end up hitting our own retry logic here!

I'll remove REQUEST_CANCELED - and this may be a good reason NOT to include
REQUEST_CANCELED in IsTransientError().

[Roger Tawa OOO till Jul 10th, 2015-05-15 21:23:22]

lgtm

As for error.IsTransient(), Brian already committed that change :-)  Lets talk
on tuesday.

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:20: // targets who sign into Chrome
on other devices, who are not signed in locally
target who -> targets those who ?

[Mike Lerman, 2015-05-19 14:47:33]

mlerman@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Mike Lerman, 2015-05-19 14:47:35]

Thanks, Roger.

Alexei, can you please take a look at histograms.xml? All the logging macros are
in signin_metrics.cc and called from cross_device_promo.cc, for your
convenience.

Thanks!

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:20: // targets who sign into Chrome
on other devices, who are not signed in locally
On 2015/05/15 21:23:22, Roger Tawa wrote:
> target who -> targets those who ?

Done.

[anthonyvd, 2015-05-19 15:07:38]

Couple nits, otherwise you've got my non-owner lgtm.

Thanks for this, can't wait to use it to build the rest of the promo :)

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:87: SetParameterFromVariation(
On 2015/05/14 20:25:33, Mike Lerman wrote:
> On 2015/05/14 18:24:20, anthonyvd wrote:
> > nit: can we move those 4 SetParameterFromVariation calls out of the if
> > condition? It's getting pretty massive.
> 
> how's this?

More readable, thanks :)

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:165: Init();
On 2015/05/14 20:25:33, Mike Lerman wrote:
> On 2015/05/14 18:24:20, anthonyvd wrote:
> > I'm not sure I understand how that state could change between the moment the
> > promo is constructed and a call to CheckPromoEligibilty.
> 
> It happens all the times in tests ;) Beyond that, you're right: the variations
> will never change, and you can only ever opt out, not back in. I'll leave a
> comment.

Ah, makes sense!

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/430001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:102: base::TimeDelta
single_account_in_profile_;
On 2015/05/14 20:25:33, Mike Lerman wrote:
> On 2015/05/14 18:24:20, anthonyvd wrote:
> > It's not super obvious in the implementation that this refers to a
TimeDelta.
> > Maybe rename it to something a little more descriptive.
> 
> How's this?

Perfect!

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/430001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:169: if
(++fetcher_retries_ < kMaxFetcherRetries &&
On 2015/05/14 20:25:33, Mike Lerman wrote:
> On 2015/05/14 18:24:20, anthonyvd wrote:
> > Is it expected that fetcher_retries_ represents the retries across all
> fetches?
> > I was expecting at most kMaxFetcherRetries retries for each fetch operations
> but
> > the way the code is at the moment the count is never reset when a fetch
> > succeeds.
> 
> It is supposed to be across all fetches. The fetches happen one after the
other,
> so logically are a single operation. That's why we have a single backoff
> instance and we InformOfRequest() on the same instance for all fetches. The
> count is reset when the whole process kicks off in Start().

Makes total sense, thanks for the info :)

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:128: if (throttle_percent &&
base::RandGenerator(100) < throttle_percent)
nit: Why not just is_throttled_ = throttle_percent && base::RandGenerator(100) <
throttle_percent ?

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:341: for (size_t i = 0; i <
devices.size(); i++) {
nit: Could use a range-based for here. Or even std::max_element :

base::Time most_recent_last_active = std::max_element(
  devices.begin(), devices.end(), 
  [] (const DeviceActivityFetcher::DeviceActivity& first, 
      const DeviceActivityFetcher::DeviceActivity& second) { 
    return first.last_active < second.last_active; 
  })->last_active;

https://codereview.chromium.org/1087933002/diff/490001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/490001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:49: bool
IsTransientError(const GoogleServiceAuthError& error) {
You can probably just use error.IsTransientError() inline now. No need for this
function.

[Alexei Svitkine (slow), 2015-05-19 15:23:49]

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:119: std::string throttle =
variations::GetVariationParamValue(
Maybe add a helper function - GetCrosDevicePromoParam(name) - so you don't have
to have this call in several places that hardcode the study name?

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:120: "CrossDevicePromo",
"RPCThrottle");
Nit: Indent 2 more. Maybe just run "git cl format"?

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:130: else
Nit: Is this branch needed - since the ctor inits it to false?

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:172: base::Time::Now()) {
Nit: Fits on line above?

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:231: } else {
Nit: No else after return

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:275: start_last_browsing_session_ =
base::Time::Now();
Nit: You call Now() several times in this function - maybe make a local?

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:341: for (size_t i = 0; i <
devices.size(); i++) {
Nit: Use C++11 for loop

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:401:
prefs::kCrossDevicePromoObservedSingleAccountCookie)) {
Nit: Since you're testing both of these here and below, maybe make local vars -
to make the ifs more readable, e.g.

bool single_account = ...;
bool has_pref = ...;

if (!single_account && has_pref) {
} else if (single_account && !has_pref) {
}

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_factory.h (right):

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_factory.h:38: };
DISALLOW_COPY_AND_ASSIGN()?

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_unittest.cc (right):

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:40: }
Nit: // namespace

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:67: };
DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:309:
prefs::kCrossDevicePromoObservedSingleAccountCookie));
Nit: Wrong indent. "git cl format"?

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:49: bool
IsTransientError(const GoogleServiceAuthError& error) {
Nit: This doesn't seem much more readable that the call it wraps. Remove?

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:108: // TODO(mlerman):
Uncomment the code below once we have a working proto.
Reference a crbug?

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.h (right):

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:27: base::Time
last_active;
Nit: Wrong indent.

[Mike Lerman, 2015-05-19 16:53:17]

Ah, the glory of git cl format.

Thanks for the feedback!

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:128: if (throttle_percent &&
base::RandGenerator(100) < throttle_percent)
On 2015/05/19 15:07:37, anthonyvd wrote:
> nit: Why not just is_throttled_ = throttle_percent && base::RandGenerator(100)
<
> throttle_percent ?

Done.

https://codereview.chromium.org/1087933002/diff/490001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:341: for (size_t i = 0; i <
devices.size(); i++) {
On 2015/05/19 15:07:37, anthonyvd wrote:
> nit: Could use a range-based for here. Or even std::max_element :
> 
> base::Time most_recent_last_active = std::max_element(
>   devices.begin(), devices.end(), 
>   [] (const DeviceActivityFetcher::DeviceActivity& first, 
>       const DeviceActivityFetcher::DeviceActivity& second) { 
>     return first.last_active < second.last_active; 
>   })->last_active;
> 

Sure - that looks fun! :)

https://codereview.chromium.org/1087933002/diff/490001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/490001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:49: bool
IsTransientError(const GoogleServiceAuthError& error) {
On 2015/05/19 15:07:37, anthonyvd wrote:
> You can probably just use error.IsTransientError() inline now. No need for
this
> function.

Done.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:119: std::string throttle =
variations::GetVariationParamValue(
On 2015/05/19 15:23:48, Alexei Svitkine wrote:
> Maybe add a helper function - GetCrosDevicePromoParam(name) - so you don't
have
> to have this call in several places that hardcode the study name?

I only call it in two places, since I've already refactored the 4 references
above into SetParameterFromVariation. I think it's readable as is, if that's
ok...?
I can change it if you feel strongly.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:120: "CrossDevicePromo",
"RPCThrottle");
On 2015/05/19 15:23:48, Alexei Svitkine wrote:
> Nit: Indent 2 more. Maybe just run "git cl format"?

fair ;)

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:130: else
On 2015/05/19 15:23:48, Alexei Svitkine wrote:
> Nit: Is this branch needed - since the ctor inits it to false?

reworked as per anthony.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:172: base::Time::Now()) {
On 2015/05/19 15:23:48, Alexei Svitkine wrote:
> Nit: Fits on line above?

Done.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:231: } else {
On 2015/05/19 15:23:48, Alexei Svitkine wrote:
> Nit: No else after return

Done.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:275: start_last_browsing_session_ =
base::Time::Now();
On 2015/05/19 15:23:48, Alexei Svitkine wrote:
> Nit: You call Now() several times in this function - maybe make a local?

Done.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:341: for (size_t i = 0; i <
devices.size(); i++) {
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> Nit: Use C++11 for loop

Changed as per anthony's comment to std::max_element

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:401:
prefs::kCrossDevicePromoObservedSingleAccountCookie)) {
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> Nit: Since you're testing both of these here and below, maybe make local vars
-
> to make the ifs more readable, e.g.
> 
> bool single_account = ...;
> bool has_pref = ...;
> 
> if (!single_account && has_pref) {
> } else if (single_account && !has_pref) {
> }

I like! Done.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_factory.h (right):

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_factory.h:38: };
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> DISALLOW_COPY_AND_ASSIGN()?

Done.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_unittest.cc (right):

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:40: }
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> Nit: // namespace

Done.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:67: };
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/1087933002/diff/510001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:309:
prefs::kCrossDevicePromoObservedSingleAccountCookie));
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> Nit: Wrong indent. "git cl format"?

Done.

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:49: bool
IsTransientError(const GoogleServiceAuthError& error) {
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> Nit: This doesn't seem much more readable that the call it wraps. Remove?

Done.

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:108: // TODO(mlerman):
Uncomment the code below once we have a working proto.
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> Reference a crbug?

Done.

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.h (right):

https://codereview.chromium.org/1087933002/diff/510001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:27: base::Time
last_active;
On 2015/05/19 15:23:49, Alexei Svitkine wrote:
> Nit: Wrong indent.

Done.

[Alexei Svitkine (slow), 2015-05-19 17:10:38]

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:168: base::Time::Now()) {
Formatting seems off. But perhaps this is what git cl format gave? If the
latter, maybe make a local var for the pref value to make this if more readable.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:208:
base::RandGenerator(delay_until_next_list_devices_.InMinutes());
Is there a reason to clamp to minute values, as opposed to e.g. seconds?

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:211:
minutes_until_next_list_devices))
Nit: Can you make a local var for some of this inner goop, to make this more
readable?

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:235:
prefs_->GetInt64(prefs::kCrossDevicePromoNextFetchListDevicesTime));
Make maybe make a helper - GetTimePref(pref_name) to wrap this? You seem to use
it a lot.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:338: const
DeviceActivityFetcher::DeviceActivity& second) {
Can you use const auto& first here?

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:356: base::Time::Now();
Nit: Make a local var for Now() at the top of the function, since you use it in
3 places.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:32: explicit
CrossDevicePromo(SigninManager* signin_manager,
Document ownership of params.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:37: void Shutdown() override;
Nit: Add a comment mentioning what this is overriding.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:42: // Overriden from
GaiaCookieManagerService::Observer
Nit: I think preferred style for these comments is just:

// GaiaCookieManagerService::Observer:

Also, can this and the two following it be private?

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:90: SigninManager* signin_manager_;
Please document ownership of these.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:124: bool initialized_;
Nit: Should probably be the first member.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_unittest.cc (right):

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:53: active ? set_active_++
: set_inactive_++;
Just use an if.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:56: bool IsActive() {
return active_; }
Nit: Just use a hacker_style accessor - is_active() and name the member
is_active_. Same for the ones below - which probably aren't currently well named
(i.e. what is set_active_ - it's clearly not a set or a bool ;).

https://codereview.chromium.org/1087933002/diff/550001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/550001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:18: const char*
kSyncListDevicesScope =
Chromium style prefers const char kFoo[]. Fix here and the two things below.

https://codereview.chromium.org/1087933002/diff/550001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:21: const char*
kListDevicesEndpoint = "http://127.0.0.1";
What? Should this have a comment/TODO?

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

https://codereview.chromium.org/1087933002/diff/550001/components/signin/core...
components/signin/core/browser/signin_metrics.cc:113:
CrossDevicePromoEligibility::NUM_CROSS_DEVICE_PROMO_ELIGIBILITY_METRICS);
Nit: No need for the enum name prefix. Here and below.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36758: +    How long a browsing session
was measured to be. Note that a minimum duration
Mention when/how this is logged.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36759: +    is configured in the Finch
experiment "CrossDevicePromo". Desktop
Finch is an internal codename. In Chromium, use either just "experiment", or
"field trial".

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36770: +    CrossDevicePromo. We need
this to estimate QPS for RPC calls.
Mention when this is logged.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36780: +    Device Sign In Promo.
Mention when this is logged.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36789: +    Tracks if profiles
initialized the XDevicePromo, and if not, why.
Ditto.

[Mike Lerman, 2015-05-19 18:17:35]

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:168: base::Time::Now()) {
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Formatting seems off. But perhaps this is what git cl format gave? If the
> latter, maybe make a local var for the pref value to make this if more
readable.

I just reformatted it manually.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:208:
base::RandGenerator(delay_until_next_list_devices_.InMinutes());
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Is there a reason to clamp to minute values, as opposed to e.g. seconds?

It seemed the appropriate level, given this value will likely be around 28 days
or more. Only need a certain level of granularity within that, really.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:211:
minutes_until_next_list_devices))
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Nit: Can you make a local var for some of this inner goop, to make this more
> readable?

Done.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:235:
prefs_->GetInt64(prefs::kCrossDevicePromoNextFetchListDevicesTime));
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Make maybe make a helper - GetTimePref(pref_name) to wrap this? You seem to
use
> it a lot.

I do, and I like. thanks!

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:338: const
DeviceActivityFetcher::DeviceActivity& second) {
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Can you use const auto& first here?

MSVC compiler (I'm working on windows) didn't like that. It's a function
parameter not a loop variable, and you can't use auto for function parameters.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:356: base::Time::Now();
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Nit: Make a local var for Now() at the top of the function, since you use it
in
> 3 places.

Done.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.h (right):

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:32: explicit
CrossDevicePromo(SigninManager* signin_manager,
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Document ownership of params.

Done

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:37: void Shutdown() override;
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Nit: Add a comment mentioning what this is overriding.

Ah, thanks. done.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:42: // Overriden from
GaiaCookieManagerService::Observer
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Nit: I think preferred style for these comments is just:
> 
> // GaiaCookieManagerService::Observer:
> 
> Also, can this and the two following it be private?

Almost - but not quite, for two reasons.
1) It's a lie, since you could always upcast the reference to the Observer type
and then access the methods, since the Observers declare them as public.
2) I refer to at least one of these (OnFetchDeviceActivityUpdated) within unit
tests.

So I could make them private, and then either upcast in my unit tests or make a
test-only exposing helper method, but this seemed better. Happy to be told
that's wrong :)

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:90: SigninManager* signin_manager_;
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Please document ownership of these.

Done.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.h:124: bool initialized_;
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Nit: Should probably be the first member.

Done.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_unittest.cc (right):

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:53: active ? set_active_++
: set_inactive_++;
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Just use an if.

Really? so much longer! but ok.

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_unittest.cc:56: bool IsActive() {
return active_; }
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> Nit: Just use a hacker_style accessor - is_active() and name the member
> is_active_. Same for the ones below - which probably aren't currently well
named
> (i.e. what is set_active_ - it's clearly not a set or a bool ;).

Done. The others are now times_set_active and times_set_inactive.

https://codereview.chromium.org/1087933002/diff/550001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/550001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:18: const char*
kSyncListDevicesScope =
On 2015/05/19 17:10:38, Alexei Svitkine wrote:
> Chromium style prefers const char kFoo[]. Fix here and the two things below.

Done.

https://codereview.chromium.org/1087933002/diff/550001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:21: const char*
kListDevicesEndpoint = "http://127.0.0.1";
On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> What? Should this have a comment/TODO?

TODOed.

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

https://codereview.chromium.org/1087933002/diff/550001/components/signin/core...
components/signin/core/browser/signin_metrics.cc:113:
CrossDevicePromoEligibility::NUM_CROSS_DEVICE_PROMO_ELIGIBILITY_METRICS);
On 2015/05/19 17:10:38, Alexei Svitkine wrote:
> Nit: No need for the enum name prefix. Here and below.

Done.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36758: +    How long a browsing session
was measured to be. Note that a minimum duration
On 2015/05/19 17:10:38, Alexei Svitkine wrote:
> Mention when/how this is logged.

Done.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36759: +    is configured in the Finch
experiment "CrossDevicePromo". Desktop
On 2015/05/19 17:10:38, Alexei Svitkine wrote:
> Finch is an internal codename. In Chromium, use either just "experiment", or
> "field trial".

Done.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36770: +    CrossDevicePromo. We need
this to estimate QPS for RPC calls.
On 2015/05/19 17:10:38, Alexei Svitkine wrote:
> Mention when this is logged.

Done.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36780: +    Device Sign In Promo.
On 2015/05/19 17:10:38, Alexei Svitkine wrote:
> Mention when this is logged.

Done.

https://codereview.chromium.org/1087933002/diff/550001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36789: +    Tracks if profiles
initialized the XDevicePromo, and if not, why.
On 2015/05/19 17:10:38, Alexei Svitkine wrote:
> Ditto.

Done.

[anthonyvd, 2015-05-19 18:57:50]

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo.cc (right):

https://codereview.chromium.org/1087933002/diff/550001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo.cc:338: const
DeviceActivityFetcher::DeviceActivity& second) {
On 2015/05/19 18:17:34, Mike Lerman wrote:
> On 2015/05/19 17:10:37, Alexei Svitkine wrote:
> > Can you use const auto& first here?
> 
> MSVC compiler (I'm working on windows) didn't like that. It's a function
> parameter not a loop variable, and you can't use auto for function parameters.

Indeed, auto type deduction in lambda parameters is only standard as of c++14.

[Alexei Svitkine (slow), 2015-05-19 19:59:44]

lgtm % remaining comments

https://codereview.chromium.org/1087933002/diff/570001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_factory.h (right):

https://codereview.chromium.org/1087933002/diff/570001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_factory.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2012? 

http://www.quickmeme.com/img/98/98b39b21ff008afaa5cba1e79fc611b2ddb64b3f44f57...

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:19: // necessary
parameters for calling the ListDevices endpoint, once live.
crbug please

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.h (right):

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:28: std::string name;
Add #include <string>

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:45: // Overridden from
GaiaAuthConsumer
Nit: Use the other comment format I mentioned.

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:51: // Overridden from
URLFetcherDelegate
Ditto.

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:60:
scoped_ptr<GaiaAuthFetcher> gaia_auth_fetcher_;
include scoped_ptr header

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

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/signin_metrics.h:215: void
LogBrowsingSessionDuration(const base::Time& previous_activity);
nit: previous_activity_time

https://codereview.chromium.org/1087933002/diff/570001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1087933002/diff/570001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36770: +    CrossDevicePromo. Logged
every time a Browser becomes active. We need this
"Browser" is ambiguous here. Do you mean the Browser class in Chromium, which
would be the Browser window - or the Browser process? Clarify please.

[Mike Lerman, 2015-05-19 20:42:53]

Thanks Mr Svitkine!

[Mike Lerman, 2015-05-19 20:54:04]

Dun dunn dunn

https://codereview.chromium.org/1087933002/diff/570001/chrome/browser/signin/...
File chrome/browser/signin/cross_device_promo_factory.h (right):

https://codereview.chromium.org/1087933002/diff/570001/chrome/browser/signin/...
chrome/browser/signin/cross_device_promo_factory.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2015/05/19 19:59:44, Alexei Svitkine wrote:
> 2012? 
> 
>
http://www.quickmeme.com/img/98/98b39b21ff008afaa5cba1e79fc611b2ddb64b3f44f57...

Done.

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.cc (right):

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.cc:19: // necessary
parameters for calling the ListDevices endpoint, once live.
On 2015/05/19 19:59:44, Alexei Svitkine wrote:
> crbug please

Done.

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
File components/signin/core/browser/device_activity_fetcher.h (right):

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:28: std::string name;
On 2015/05/19 19:59:44, Alexei Svitkine wrote:
> Add #include <string>

Done.

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:45: // Overridden from
GaiaAuthConsumer
On 2015/05/19 19:59:44, Alexei Svitkine wrote:
> Nit: Use the other comment format I mentioned.

Done.

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:51: // Overridden from
URLFetcherDelegate
On 2015/05/19 19:59:44, Alexei Svitkine wrote:
> Ditto.

Done.

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/device_activity_fetcher.h:60:
scoped_ptr<GaiaAuthFetcher> gaia_auth_fetcher_;
On 2015/05/19 19:59:44, Alexei Svitkine wrote:
> include scoped_ptr header

Done.

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

https://codereview.chromium.org/1087933002/diff/570001/components/signin/core...
components/signin/core/browser/signin_metrics.h:215: void
LogBrowsingSessionDuration(const base::Time& previous_activity);
On 2015/05/19 19:59:44, Alexei Svitkine wrote:
> nit: previous_activity_time

Done.

https://codereview.chromium.org/1087933002/diff/570001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1087933002/diff/570001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:36770: +    CrossDevicePromo. Logged
every time a Browser becomes active. We need this
On 2015/05/19 19:59:44, Alexei Svitkine wrote:
> "Browser" is ambiguous here. Do you mean the Browser class in Chromium, which
> would be the Browser window - or the Browser process? Clarify please.

Done.

[Mike Lerman, 2015-05-20 12:30:53]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-05-20 12:30:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, anthonyvd@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1087933002/#ps590001
(title: "alexei's final comments")

[commit-bot: I haz the power, 2015-05-20 12:31:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087933002/590001

[commit-bot: I haz the power, 2015-05-20 13:03:33]

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

[commit-bot: I haz the power, 2015-05-20 13:03:34]

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

[Mike Lerman, 2015-05-20 13:18:15]

The CQ bit was checked by mlerman@chromium.org

[Mike Lerman, 2015-05-20 13:18:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, asvitkine@chromium.org, anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/1087933002/#ps610001
(title: "build.gn")

[commit-bot: I haz the power, 2015-05-20 13:18:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1087933002/610001

[commit-bot: I haz the power, 2015-05-20 14:13:15]

Committed patchset #32 (id:610001)

[commit-bot: I haz the power, 2015-05-20 14:14:13]

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