Account reconcilor: Use cookie store cookie changed subscription.

Account reconcilor: Use cookie store cookie changed subscription.

This CL changes the account reconcilor to use the new cookie store
API that allows clients to adds callback for cookie changed events.
It adds logic to the chrome sign-in client to register for cookie
changed events on the IO thread and call the cookie changed callback
on the main thread.

Note that in order to access the cookie store, we need to use the
URL context which can only be used on the IO thread. Subscribing for
cookie change events requires thus a thread jump to the IO thread on
all platforms.

BUG=NONE

Committed: https://crrev.com/a9d6912af5c098c91e85cd6eda7a7d65d4c9c028
Cr-Commit-Position: refs/heads/master@{#302639}

[msarda, 2014-10-30 17:38:47]

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

[msarda, 2014-10-30 17:38:48]

Please take a look.

[msarda, 2014-10-31 09:06:27]

Please take a look.

[msarda, 2014-10-31 12:16:52]

msarda@chromium.org changed reviewers:
+ droger@chromium.org

[msarda, 2014-10-31 12:16:52]

Please take a look.

[droger, 2014-10-31 12:42:02]

This CL is very similar to the other one, I only have minor comments.

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.cc (right):

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:84: // On iOS, the
cookie store can only be accessed from the IO thread. As
This comment may need to be updated.

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:17: public
base::SupportsWeakPtr<SigninCookieChangedSubscription> {
Maybe add a comment saying that this class must only be used from SigninClient
(and maybe this could be enforced by DEPS, if we are very paranoid). This way
you don't have to worry about exposing implementation details (such as
SupportsWeakPtr).

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:20: const
net::CookieStore::CookieChangedCallback& callback);
explicit.

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:21: virtual
~SigninCookieChangedSubscription();
s/virtual/override/

or maybe even better:
s/virtual/final/
I'm not very familiar with final though, so not sure.

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
components/signin/core/browser/signin_client.h:36: // The subcription for cookie
changed notifications.
Indent.

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
components/signin/core/browser/signin_client.h:37: class
CookieChangedSubscription {
Do we need this class?

An alternative to this would be to use directly the concrete implementation, and
use "friend" to mask the implementation details.

I don't have a strong opinion either way.

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
components/signin/core/browser/signin_client.h:39: virtual
~CookieChangedSubscription() {};
Should this be pure virtual?

[msarda, 2014-10-31 13:18:40]

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.cc (right):

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:84: // On iOS, the
cookie store can only be accessed from the IO thread. As
On 2014/10/31 12:42:01, droger wrote:
> This comment may need to be updated.

Done.

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:17: public
base::SupportsWeakPtr<SigninCookieChangedSubscription> {
On 2014/10/31 12:42:01, droger wrote:
> Maybe add a comment saying that this class must only be used from SigninClient
> (and maybe this could be enforced by DEPS, if we are very paranoid). This way
> you don't have to worry about exposing implementation details (such as
> SupportsWeakPtr).

Is that really so important that this class supports weak pointer and that this
is visible to the users?

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:20: const
net::CookieStore::CookieChangedCallback& callback);
On 2014/10/31 12:42:01, droger wrote:
> explicit.

Done.

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:21: virtual
~SigninCookieChangedSubscription();
On 2014/10/31 12:42:01, droger wrote:
> s/virtual/override/
> 
> or maybe even better:
> s/virtual/final/
> I'm not very familiar with final though, so not sure.

I used override instead of virtual

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
components/signin/core/browser/signin_client.h:36: // The subcription for cookie
changed notifications.
On 2014/10/31 12:42:01, droger wrote:
> Indent.

Done.

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
components/signin/core/browser/signin_client.h:37: class
CookieChangedSubscription {
On 2014/10/31 12:42:01, droger wrote:
> Do we need this class?
> 
> An alternative to this would be to use directly the concrete implementation,
and
> use "friend" to mask the implementation details.
> 
> I don't have a strong opinion either way.

This interface lives in //components. The implementation lives in //chrome. I
think it is clearer to have this class definition here?

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
components/signin/core/browser/signin_client.h:39: virtual
~CookieChangedSubscription() {};
On 2014/10/31 12:42:01, droger wrote:
> Should this be pure virtual?

I tried this yesterday and it did not build. What would be the benefit for being
pure virtual?

[droger, 2014-10-31 13:56:09]

lgtm

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/100001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:17: public
base::SupportsWeakPtr<SigninCookieChangedSubscription> {
On 2014/10/31 13:18:40, msarda wrote:
> On 2014/10/31 12:42:01, droger wrote:
> > Maybe add a comment saying that this class must only be used from
SigninClient
> > (and maybe this could be enforced by DEPS, if we are very paranoid). This
way
> > you don't have to worry about exposing implementation details (such as
> > SupportsWeakPtr).
> 
> Is that really so important that this class supports weak pointer and that
this
> is visible to the users?

No I think it is fine, especially since you pass a
SigninClient::CookieChangedSubscription to the user, and not this type.

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
components/signin/core/browser/signin_client.h:37: class
CookieChangedSubscription {
On 2014/10/31 13:18:40, msarda wrote:
> On 2014/10/31 12:42:01, droger wrote:
> > Do we need this class?
> > 
> > An alternative to this would be to use directly the concrete implementation,
> and
> > use "friend" to mask the implementation details.
> > 
> > I don't have a strong opinion either way.
> 
> This interface lives in //components. The implementation lives in //chrome. I
> think it is clearer to have this class definition here?

The implementation could be in components too (I know you use content/ but you
could remove it).
Anyway I don't have a strong opinion, I'm fine with this

https://codereview.chromium.org/695553002/diff/100001/components/signin/core/...
components/signin/core/browser/signin_client.h:39: virtual
~CookieChangedSubscription() {};
On 2014/10/31 13:18:40, msarda wrote:
> On 2014/10/31 12:42:01, droger wrote:
> > Should this be pure virtual?
> 
> I tried this yesterday and it did not build. What would be the benefit for
being
> pure virtual?

I thought it would be nice to prevent people to instantiate this class directly
(as this would very likely be a bug).

[msarda, 2014-11-04 10:04:14]

The last patch should fix the threading issue. Please take another look.

[msarda, 2014-11-04 10:04:48]

Patchset #8 (id:140001) has been deleted

[droger, 2014-11-04 10:31:36]

Looks like a couple unittests are now failing.

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:25: #include
"content/public/browser/notification_source.h"
Do we need these includes?

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.cc (right):

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:63:
content::BrowserThread::DeleteOnIOThread::Destruct(
Optional:
This assumes that the network thread is the IO thread.
It would be cleaner to keep a reference to the network task runner and use it
here.

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:90:
content::BrowserThread::PostTask(
I think this would be better:
context_getter->GetNetworkTaskRunner()->PostTask():

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:19: struct
SubscriptionHolder {
Should this be private?
(To make it private you may need RegisterForCookieChangesOnIOThread() and
RunAsyncOnCookieChanged() to be static class methods on
SigninCookieChangedSubscription.)

[msarda, 2014-11-04 12:06:06]

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:25: #include
"content/public/browser/notification_source.h"
On 2014/11/04 10:31:36, droger wrote:
> Do we need these includes?

Done.

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.cc (right):

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:63:
content::BrowserThread::DeleteOnIOThread::Destruct(
On 2014/11/04 10:31:36, droger wrote:
> Optional:
> This assumes that the network thread is the IO thread.
> It would be cleaner to keep a reference to the network task runner and use it
> here.

Done.

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:90:
content::BrowserThread::PostTask(
On 2014/11/04 10:31:36, droger wrote:
> I think this would be better:
> context_getter->GetNetworkTaskRunner()->PostTask():

Done.

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/180001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:19: struct
SubscriptionHolder {
On 2014/11/04 10:31:36, droger wrote:
> Should this be private?
> (To make it private you may need RegisterForCookieChangesOnIOThread() and
> RunAsyncOnCookieChanged() to be static class methods on
> SigninCookieChangedSubscription.)

Done.

[droger, 2014-11-04 12:44:49]

LGTM!

only nits.

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:206: return
scoped_ptr<CookieChangedSubscription>(subscription.release());
return subscription;

or if it doesn't work:
return subscription.Pass();

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.cc (right):

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:52: if
(network_task_runner->BelongsToCurrentThread()) {
Somewhat unrelated, fyi:

The code you need exists in:
base/memory/ref_counted_delete_on_message_loop.h

But you cannot use it because it takes a MessageLoopProxy instead of a
SingleThreadedTaskRunner.
There's nothing in that code that requires a MessageLoopProxy, and it would
probably be nice to change it to use SingleThreadedTaskRunner, as it would be
more generic.

I am not sure why the original author did it this way... oh wait... that's me.

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:69: void
SigninCookieChangedSubscription::RegisterForCookieChangedNotifications(
Optional nit:
remove this function and inline it in the constructor.

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:53:
scoped_refptr<net::URLRequestContextGetter> context_getter_;
Why not just keep a reference on the SingleThreadTaskRunner?

[msarda, 2014-11-04 12:53:19]

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_signin_client.cc (right):

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/c...
chrome/browser/signin/chrome_signin_client.cc:206: return
scoped_ptr<CookieChangedSubscription>(subscription.release());
On 2014/11/04 12:44:49, droger wrote:
> return subscription;
> 
> or if it doesn't work:
> return subscription.Pass();

Done.

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.cc (right):

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:52: if
(network_task_runner->BelongsToCurrentThread()) {
On 2014/11/04 12:44:49, droger wrote:
> Somewhat unrelated, fyi:
> 
> The code you need exists in:
> base/memory/ref_counted_delete_on_message_loop.h
> 
> But you cannot use it because it takes a MessageLoopProxy instead of a
> SingleThreadedTaskRunner.
> There's nothing in that code that requires a MessageLoopProxy, and it would
> probably be nice to change it to use SingleThreadedTaskRunner, as it would be
> more generic.
> 
> I am not sure why the original author did it this way... oh wait... that's me.

:)

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.cc:69: void
SigninCookieChangedSubscription::RegisterForCookieChangedNotifications(
On 2014/11/04 12:44:49, droger wrote:
> Optional nit:
> remove this function and inline it in the constructor.

I prefer it like this as it gives more scope to what this method does.

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:53:
scoped_refptr<net::URLRequestContextGetter> context_getter_;
On 2014/11/04 12:44:49, droger wrote:
> Why not just keep a reference on the SingleThreadTaskRunner?

No particular reason. It made more sense to me when coding keep the context
getter. Is there any particular reason why this is bad?

[droger, 2014-11-04 13:08:40]

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/200001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:53:
scoped_refptr<net::URLRequestContextGetter> context_getter_;
On 2014/11/04 12:53:19, msarda wrote:
> On 2014/11/04 12:44:49, droger wrote:
> > Why not just keep a reference on the SingleThreadTaskRunner?
> 
> No particular reason. It made more sense to me when coding keep the context
> getter. Is there any particular reason why this is bad?

You are retaining the object and thus you may extend the lifetime.
In practice I think it does not make a lot of difference though.

[Roger Tawa OOO till Jul 10th, 2014-11-04 16:24:24]

lgtm

For my own info, how does this fix the race condition on shutdown?

https://codereview.chromium.org/695553002/diff/220001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/220001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:29: void
OnCookieChanged(const net::CanonicalCookie& cookie, bool removed);
Nit: any way to make this private?

https://codereview.chromium.org/695553002/diff/220001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:57:
scoped_ptr<SubscriptionHolder> subscription_holder_io_;
Nit: comment says "netwotk thread", variable has "io" in name.  Maybe change
comment?

https://codereview.chromium.org/695553002/diff/220001/components/signin/core/...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/695553002/diff/220001/components/signin/core/...
components/signin/core/browser/signin_client.h:39: virtual
~CookieChangedSubscription(){};
Nit: space before {}

[msarda, 2014-11-04 17:50:15]

https://codereview.chromium.org/695553002/diff/220001/chrome/browser/signin/s...
File chrome/browser/signin/signin_cookie_changed_subscription.h (right):

https://codereview.chromium.org/695553002/diff/220001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:29: void
OnCookieChanged(const net::CanonicalCookie& cookie, bool removed);
On 2014/11/04 16:24:24, Roger Tawa wrote:
> Nit: any way to make this private?

Done.

https://codereview.chromium.org/695553002/diff/220001/chrome/browser/signin/s...
chrome/browser/signin/signin_cookie_changed_subscription.h:57:
scoped_ptr<SubscriptionHolder> subscription_holder_io_;
On 2014/11/04 16:24:24, Roger Tawa wrote:
> Nit: comment says "netwotk thread", variable has "io" in name.  Maybe change
> comment?

Not done as discussed off-line.

https://codereview.chromium.org/695553002/diff/220001/components/signin/core/...
File components/signin/core/browser/signin_client.h (right):

https://codereview.chromium.org/695553002/diff/220001/components/signin/core/...
components/signin/core/browser/signin_client.h:39: virtual
~CookieChangedSubscription(){};
On 2014/11/04 16:24:24, Roger Tawa wrote:
> Nit: space before {}

Done.

[msarda, 2014-11-04 17:50:21]

The CQ bit was checked by msarda@chromium.org

[commit-bot: I haz the power, 2014-11-04 17:50:42]

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

[commit-bot: I haz the power, 2014-11-04 19:22:19]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2014-11-04 19:23:00]

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