components/signin: convert GaiaCookieManagerService to use CanonicalCookie's Create() method.

components/signin: convert GaiaCookieManagerService to use CanonicalCookie's Create() method.

This patch converts the usage of CanonicalCookie's constructor in
GaiaCookieManagerService::ForceOnCookieChangedProcessing() to use
CanonicalCookie::Create() method.

We are doing this because the Create() method validates the parameters
and we thus we can test the return value to see if it has passed or not.
Whereas we can not do this when using the constructor.

BUG=57061
TEST=components_unittests
R=mmenke@chromium.org,rogerta@chromium.org

Committed: https://crrev.com/94486086f25fbf5a1441ac32eb972fc9686801ac
Cr-Commit-Position: refs/heads/master@{#406097}

[tfarina, 2016-07-01 12:20:10]

https://codereview.chromium.org/2113863002/diff/20001/components/signin/core/...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/2113863002/diff/20001/components/signin/core/...
components/signin/core/browser/gaia_cookie_manager_service.cc:382: google_url,
kGaiaCookieName, std::string(), google_url.host(),
Matt, I had to keep host() in order to keep the unit test happy, because the
default value for it has a leading dot, see
https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_urls.cc?sq=package...,
but looks like this value can be changed from command line. Should I do
something to handle that possibility or with host() will we go well in any case?

[mmenke, 2016-07-01 16:21:01]

https://codereview.chromium.org/2113863002/diff/20001/components/signin/core/...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/2113863002/diff/20001/components/signin/core/...
components/signin/core/browser/gaia_cookie_manager_service.cc:382: google_url,
kGaiaCookieName, std::string(), google_url.host(),
On 2016/07/01 12:20:10, tfarina wrote:
> Matt, I had to keep host() in order to keep the unit test happy, because the
> default value for it has a leading dot, see
>
https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_urls.cc?sq=package...,
> but looks like this value can be changed from command line. Should I do
> something to handle that possibility or with host() will we go well in any
case?

Hrm...that's not a valid URL, and is apparently breaking something as a result.

Rogerta:  Why is kDefaultGoogleUrl not using a valid domain name?

[mmenke, 2016-07-15 18:47:18]

https://codereview.chromium.org/2113863002/diff/20001/components/signin/core/...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/2113863002/diff/20001/components/signin/core/...
components/signin/core/browser/gaia_cookie_manager_service.cc:382: google_url,
kGaiaCookieName, std::string(), google_url.host(),
On 2016/07/01 16:21:00, mmenke wrote:
> On 2016/07/01 12:20:10, tfarina wrote:
> > Matt, I had to keep host() in order to keep the unit test happy, because the
> > default value for it has a leading dot, see
> >
>
https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_urls.cc?sq=package...,
> > but looks like this value can be changed from command line. Should I do
> > something to handle that possibility or with host() will we go well in any
> case?
> 
> Hrm...that's not a valid URL, and is apparently breaking something as a
result.
> 
> Rogerta:  Why is kDefaultGoogleUrl not using a valid domain name?

So here's my suggestion (Need to have one of the owners verify it's sane,
though):

replace the url with http://google.com (i.e., remove the dot), and in this code,
replace google_url.host() with: "." + google_url.host()

[tfarina, 2016-07-18 18:59:30]

The CQ bit was checked by tfarina@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 19:00:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 19:20:15]

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

[commit-bot: I haz the power, 2016-07-18 19:20:16]

Dry run: Try jobs failed on following builders:
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[tfarina, 2016-07-18 19:31:43]

The CQ bit was checked by tfarina@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-18 19:32:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tfarina, 2016-07-18 20:00:07]

https://codereview.chromium.org/2113863002/diff/20001/components/signin/core/...
File components/signin/core/browser/gaia_cookie_manager_service.cc (right):

https://codereview.chromium.org/2113863002/diff/20001/components/signin/core/...
components/signin/core/browser/gaia_cookie_manager_service.cc:382: google_url,
kGaiaCookieName, std::string(), google_url.host(),
On 2016/07/15 18:47:18, mmenke wrote:
> On 2016/07/01 16:21:00, mmenke wrote:
> > On 2016/07/01 12:20:10, tfarina wrote:
> > > Matt, I had to keep host() in order to keep the unit test happy, because
the
> > > default value for it has a leading dot, see
> > >
> >
>
https://cs.chromium.org/chromium/src/google_apis/gaia/gaia_urls.cc?sq=package...,
> > > but looks like this value can be changed from command line. Should I do
> > > something to handle that possibility or with host() will we go well in any
> > case?
> > 
> > Hrm...that's not a valid URL, and is apparently breaking something as a
> result.
> > 
> > Rogerta:  Why is kDefaultGoogleUrl not using a valid domain name?
> 
> So here's my suggestion (Need to have one of the owners verify it's sane,
> though):
> 
> replace the url with http://google.com (i.e., remove the dot), and in this
code,
> replace google_url.host() with: "." + google_url.host()

Done.

[mmenke, 2016-07-18 20:02:25]

LGTM, thanks!  And sorry for the delay - buried in reviews last week, after my
vacation.

[commit-bot: I haz the power, 2016-07-18 20:38:28]

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

[commit-bot: I haz the power, 2016-07-18 20:38:29]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2016-07-18 20:44:58]

lgtm

[tfarina, 2016-07-18 21:06:23]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2016-07-18 21:07:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 21:15:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-18 21:15:29]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 21:16:40]

Description was changed from

==========
components/signin: convert GaiaCookieManagerService to use CanonicalCookie's
Create() method.

This patch converts the usage of CanonicalCookie's constructor in
GaiaCookieManagerService::ForceOnCookieChangedProcessing() to use
CanonicalCookie::Create() method.

We are doing this because the Create() method validates the parameters
and we thus we can test the return value to see if it has passed or not.
Whereas we can not do this when using the constructor.

BUG=57061
TEST=components_unittests
R=mmenke@chromium.org,rogerta@chromium.org
==========

to

==========
components/signin: convert GaiaCookieManagerService to use CanonicalCookie's
Create() method.

This patch converts the usage of CanonicalCookie's constructor in
GaiaCookieManagerService::ForceOnCookieChangedProcessing() to use
CanonicalCookie::Create() method.

We are doing this because the Create() method validates the parameters
and we thus we can test the return value to see if it has passed or not.
Whereas we can not do this when using the constructor.

BUG=57061
TEST=components_unittests
R=mmenke@chromium.org,rogerta@chromium.org

Committed: https://crrev.com/94486086f25fbf5a1441ac32eb972fc9686801ac
Cr-Commit-Position: refs/heads/master@{#406097}
==========

[commit-bot: I haz the power, 2016-07-18 21:16:41]

Patchset 4 (id:??) landed as
https://crrev.com/94486086f25fbf5a1441ac32eb972fc9686801ac
Cr-Commit-Position: refs/heads/master@{#406097}