When the user signs in, make sure to set cookies on all reachable google

When the user signs in, make sure to set cookies on all reachable google
properties too.

BUG=397199
TBR=nkostylev@chromium.org

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

[Roger Tawa OOO till Jul 10th, 2014-08-04 17:50:46]

Hi Hui,

Please take a look.  Thanks.

[guohui, 2014-08-05 19:14:08]

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher.h (right):

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.h:151: // The |external_cc_result| string can
specify the result of connetion checks
nits: rename external_cc_result to external_connection_check_result or
connection_check_result for better readability.

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.cc (right):

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:77: // TODO(rogerta): I have no idea
how to long to wait before timing out.
nits: extra to

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:265:
result_fetcher_.GetExternalCcResult());
is it guaranteed that all eternal cc results are available at this point?

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.h (right):

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:47: class ExternalCcResultFetcher :
public GaiaAuthConsumer,
nits: i think in general we do not allow non-standard abbreviations in Chrome
code, so expand all Cc to ConnectionCheck?

[guohui, 2014-08-06 20:49:58]

lgtm

as discussed offline, i am good with just adding log to track how well the
current best-effort approach work, and logging could be added in a separate CL.

[guohui, 2014-08-06 20:52:28]

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.cc (right):

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:265:
result_fetcher_.GetExternalCcResult());
could you also add comments here that it is not guaranteed that the external cc
result is fully available at this point?

[Roger Tawa OOO till Jul 10th, 2014-08-08 19:29:53]

Thanks Hui, please take a look.  I added metrics as we discussed offline.

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/gaia_a...
File google_apis/gaia/gaia_auth_fetcher.h (right):

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/gaia_a...
google_apis/gaia/gaia_auth_fetcher.h:151: // The |external_cc_result| string can
specify the result of connetion checks
On 2014/08/05 19:14:07, guohui wrote:
> nits: rename external_cc_result to external_connection_check_result or
> connection_check_result for better readability.

I think I'll leave it as is, becauce "Cc" is the official name of the URL
parameter.  See line 366 in gaia_auth_fetcher.cc.

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.cc (right):

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:77: // TODO(rogerta): I have no idea
how to long to wait before timing out.
On 2014/08/05 19:14:07, guohui wrote:
> nits: extra to

Done.

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:265:
result_fetcher_.GetExternalCcResult());
On 2014/08/05 19:14:07, guohui wrote:
> is it guaranteed that all eternal cc results are available at this point?

Nope.  We don't want to delay the merge session, so any that have responded will
be included, any that haven't will not.  For checks that have not completed, the
worst case is that the user sees a "sign in" button.  If they click that button,
they will be signed in automatically, no need to retype username or password.

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:265:
result_fetcher_.GetExternalCcResult());
On 2014/08/06 20:52:27, guohui wrote:
> could you also add comments here that it is not guaranteed that the external
cc
> result is fully available at this point?

Done.

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.h (right):

https://codereview.chromium.org/424233002/diff/190001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:47: class ExternalCcResultFetcher :
public GaiaAuthConsumer,
On 2014/08/05 19:14:07, guohui wrote:
> nits: i think in general we do not allow non-standard abbreviations in Chrome
> code, so expand all Cc to ConnectionCheck? 

As before, Cc is the actual name of parameter, so I'll leave it as is.

[guohui, 2014-08-08 19:35:15]

still LGTM.

[Roger Tawa OOO till Jul 10th, 2014-08-08 20:08:48]

Hi Alexei,

Can you please review histograms.xml?

Mike: fyi I added a new histogram to measure how well the externalccresult
works.

[Mike Lerman, 2014-08-08 20:20:27]

A couple of histograms comments.

https://codereview.chromium.org/424233002/diff/210001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/424233002/diff/210001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28211: +<histogram
name="Signin.Reconciler.AllExternalCcResultCompleted">
add attribute enum="BooleanSuccess"

https://codereview.chromium.org/424233002/diff/210001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28214: +    Count how often all external
connection have been successfully checked
nit: connection(s)

[Roger Tawa OOO till Jul 10th, 2014-08-08 20:47:01]

Thanks Mike.

https://codereview.chromium.org/424233002/diff/210001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/424233002/diff/210001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28211: +<histogram
name="Signin.Reconciler.AllExternalCcResultCompleted">
On 2014/08/08 20:20:27, Mike Lerman wrote:
> add attribute enum="BooleanSuccess"

Done.

https://codereview.chromium.org/424233002/diff/210001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28214: +    Count how often all external
connection have been successfully checked
On 2014/08/08 20:20:27, Mike Lerman wrote:
> nit: connection(s)

Done.

[Alexei Svitkine (slow), 2014-08-08 21:16:49]

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.cc (right):

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:114: // Only up to the first 16
characters of the response are important.
Why? The comment should provide more insight than just the code.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:124: delete source;
Why is |source| only deleted within the if?

What happens to it if the if is false? Is it leaked?

Are you sure it's correct to delete it? If so, should probably have a comment
about it.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:145: 
Nit: Remove extra line.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.h (right):

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:47: class ExternalCcResultFetcher :
public GaiaAuthConsumer,
Is there a reason this class is in the public section rather than in the private
section?

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:77: void TimeoutForTests() {
Nit: Non-trivial methods (those not using hacker_style) should be implemented
out of line.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:102: };
Nit: DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/424233002/diff/230001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/424233002/diff/230001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28215: +    Count how often all external
connections have been successfully checked
This description doesn't tell me what a value of "true" or "false" means for
this histogram.

I suggest either adding a more descriptive enum or fixing the description.

[Roger Tawa OOO till Jul 10th, 2014-08-09 15:58:28]

Hi Alexei,

Please take another look.  See comments below.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.cc (right):

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:114: // Only up to the first 16
characters of the response are important.
On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> Why? The comment should provide more insight than just the code.

Because the gaia protocol says only the first 16 characters are important.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:124: delete source;
On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> Why is |source| only deleted within the if?
> 
> What happens to it if the if is false? Is it leaked?
> 
> Are you sure it's correct to delete it? If so, should probably have a comment
> about it.

Because source == fetcher_[url].second (see DCHECK at line 122) and because url
is being removed from |fetchers_|.  In all other cases, the fetcher will be
deleted when |fetchers_| is cleaned up in CleanupTransientState().

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:145: 
On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> Nit: Remove extra line.

Done.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.h (right):

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:47: class ExternalCcResultFetcher :
public GaiaAuthConsumer,
On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> Is there a reason this class is in the public section rather than in the
private
> section?

Yes for unit tests.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:77: void TimeoutForTests() {
On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> Nit: Non-trivial methods (those not using hacker_style) should be implemented
> out of line.

It is trivial.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:102: };
On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/424233002/diff/230001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/424233002/diff/230001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28215: +    Count how often all external
connections have been successfully checked
On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> This description doesn't tell me what a value of "true" or "false" means for
> this histogram.
> 
> I suggest either adding a more descriptive enum or fixing the description.

I don't think a more descriptive enum is appropriate/needed here.  This
description seems pretty clear to me.  If you'd like to suggest a better string
I'll replace it.

[Alexei Svitkine (slow), 2014-08-11 14:04:51]

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.cc (right):

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:114: // Only up to the first 16
characters of the response are important.
On 2014/08/09 15:58:28, Roger Tawa wrote:
> On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > Why? The comment should provide more insight than just the code.
> 
> Because the gaia protocol says only the first 16 characters are important.

Okay, can you add that info to the comment? i.e. "are important." -> "are
important according to the protocol."

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:124: delete source;
On 2014/08/09 15:58:28, Roger Tawa wrote:
> On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > Why is |source| only deleted within the if?
> > 
> > What happens to it if the if is false? Is it leaked?
> > 
> > Are you sure it's correct to delete it? If so, should probably have a
comment
> > about it.
> 
> Because source == fetcher_[url].second (see DCHECK at line 122) and because
url
> is being removed from |fetchers_|.  In all other cases, the fetcher will be
> deleted when |fetchers_| is cleaned up in CleanupTransientState().

Got it. Thanks for the explanation. It might be good to mention this in a
comment for future readers, but I'll leave it up to you.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:125: if (fetchers_.size() == 0) {
Nit: empty()

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.h (right):

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:47: class ExternalCcResultFetcher :
public GaiaAuthConsumer,
On 2014/08/09 15:58:28, Roger Tawa wrote:
> On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > Is there a reason this class is in the public section rather than in the
> private
> > section?
> 
> Yes for unit tests.

Makes sense. Please add a line to the comment to mention this. "Declared in the
public section for unit tests."

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:77: void TimeoutForTests() {
On 2014/08/09 15:58:28, Roger Tawa wrote:
> On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > Nit: Non-trivial methods (those not using hacker_style) should be
implemented
> > out of line.
> 
> It is trivial.

I don't think it qualifiers per our style guide:

http://www.chromium.org/developers/coding-style#TOC-Inline-functions

https://codereview.chromium.org/424233002/diff/230001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/424233002/diff/230001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28215: +    Count how often all external
connections have been successfully checked
On 2014/08/09 15:58:28, Roger Tawa wrote:
> On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > This description doesn't tell me what a value of "true" or "false" means for
> > this histogram.
> > 
> > I suggest either adding a more descriptive enum or fixing the description.
> 
> I don't think a more descriptive enum is appropriate/needed here.  This
> description seems pretty clear to me.  If you'd like to suggest a better
string
> I'll replace it.

How about changing to the following wording:

Records whether all external connections have been successfully checked
(a value of "true") or not ("false") when the reconciler attempts to perform
MergeSession.

--

(Note that the previous wording of "counts how often" isn't quite right since
that's a description of the aggregation part of UMA pipelines/dashboard, not of
the single trigger of the histogram.)

[Roger Tawa OOO till Jul 10th, 2014-08-11 20:20:55]

Hi Alexei, please take another look.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.cc (right):

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:114: // Only up to the first 16
characters of the response are important.
On 2014/08/11 14:04:51, Alexei Svitkine wrote:
> On 2014/08/09 15:58:28, Roger Tawa wrote:
> > On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > > Why? The comment should provide more insight than just the code.
> > 
> > Because the gaia protocol says only the first 16 characters are important.
> 
> Okay, can you add that info to the comment? i.e. "are important." -> "are
> important according to the protocol."

Fixed in previous patchset.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:124: delete source;
On 2014/08/11 14:04:51, Alexei Svitkine wrote:
> On 2014/08/09 15:58:28, Roger Tawa wrote:
> > On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > > Why is |source| only deleted within the if?
> > > 
> > > What happens to it if the if is false? Is it leaked?
> > > 
> > > Are you sure it's correct to delete it? If so, should probably have a
> comment
> > > about it.
> > 
> > Because source == fetcher_[url].second (see DCHECK at line 122) and because
> url
> > is being removed from |fetchers_|.  In all other cases, the fetcher will be
> > deleted when |fetchers_| is cleaned up in CleanupTransientState().
> 
> Got it. Thanks for the explanation. It might be good to mention this in a
> comment for future readers, but I'll leave it up to you.

Done.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.cc:125: if (fetchers_.size() == 0) {
On 2014/08/11 14:04:50, Alexei Svitkine wrote:
> Nit: empty()

Done.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
File google_apis/gaia/merge_session_helper.h (right):

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:47: class ExternalCcResultFetcher :
public GaiaAuthConsumer,
On 2014/08/11 14:04:51, Alexei Svitkine wrote:
> On 2014/08/09 15:58:28, Roger Tawa wrote:
> > On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > > Is there a reason this class is in the public section rather than in the
> > private
> > > section?
> > 
> > Yes for unit tests.
> 
> Makes sense. Please add a line to the comment to mention this. "Declared in
the
> public section for unit tests."

Done.

https://codereview.chromium.org/424233002/diff/230001/google_apis/gaia/merge_...
google_apis/gaia/merge_session_helper.h:77: void TimeoutForTests() {
On 2014/08/11 14:04:51, Alexei Svitkine wrote:
> On 2014/08/09 15:58:28, Roger Tawa wrote:
> > On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > > Nit: Non-trivial methods (those not using hacker_style) should be
> implemented
> > > out of line.
> > 
> > It is trivial.
> 
> I don't think it qualifiers per our style guide:
> 
> http://www.chromium.org/developers/coding-style#TOC-Inline-functions

Done.

https://codereview.chromium.org/424233002/diff/230001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/424233002/diff/230001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28215: +    Count how often all external
connections have been successfully checked
On 2014/08/11 14:04:51, Alexei Svitkine wrote:
> On 2014/08/09 15:58:28, Roger Tawa wrote:
> > On 2014/08/08 21:16:48, Alexei Svitkine wrote:
> > > This description doesn't tell me what a value of "true" or "false" means
for
> > > this histogram.
> > > 
> > > I suggest either adding a more descriptive enum or fixing the description.
> > 
> > I don't think a more descriptive enum is appropriate/needed here.  This
> > description seems pretty clear to me.  If you'd like to suggest a better
> string
> > I'll replace it.
> 
> How about changing to the following wording:
> 
> Records whether all external connections have been successfully checked
> (a value of "true") or not ("false") when the reconciler attempts to perform
> MergeSession.
> 
> --
> 
> (Note that the previous wording of "counts how often" isn't quite right since
> that's a description of the aggregation part of UMA pipelines/dashboard, not
of
> the single trigger of the histogram.)
> 

Done.

[Alexei Svitkine (slow), 2014-08-12 14:21:56]

LGTM

[Roger Tawa OOO till Jul 10th, 2014-08-12 14:53:16]

The CQ bit was checked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-08-12 14:53:21]

The CQ bit was unchecked by rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2014-08-13 01:20:54]

The CQ bit was checked by rogerta@chromium.org

[commit-bot: I haz the power, 2014-08-13 01:32:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rogerta@chromium.org/424233002/330001

[commit-bot: I haz the power, 2014-08-13 05:13:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-13 09:58:53]

Change committed as 289244