Extract Profile-independent GCMService from GCMProfileService

Extract Profile-independent GCMService from GCMProfileService

This CL moves most of the GCMProfileService functionality to a new
GCMService class that does not depend on Profile. GCMProfileService
becomes a subclass of GCMService that adds Profile-specific lifetime
management and control over the service via user preferences.

The CL is a prerequisite for Chrome OS device policy pushing, which will
need to instantiate a Tango connection using device-wide GAIA credentials
not tied to any user or Profile.

The CL also fixes a few subtle bugs, such as GCMProfileService::IOWorker
being shut down on the wrong thread and unit tests looking at the wrong
GCMProfileService when trying to verify correct behavior.

BUG=362083
TEST=Updated unit tests
R=atwilson@chromium.org, dcheng@chromium.org, jianli@chromium.org, yoz@chromium.org

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

[bartfab (slow), 2014-04-10 14:55:57]

Hi Jian,

Can you please review:
  chrome/browser/services/gcm/*
  chrome/browser/extensions/api/gcm/*

Hi Daniel,

Can you please review chrome/browser/invalidation/*?

Hi Yoyo,

Can you please do an owner review of chrome/browser/extensions/*?

Hi Drew,

Can you please do an owner review of:
  chrome/browser/sync/profile_sync_service.cc
  sync/*

[jianli, 2014-04-10 20:39:55]

Have you made sure all tests passed on trybots, esp. memory bots?

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/extension...
chrome/browser/extensions/api/gcm/gcm_api.cc:101: gcm::GCMService*
GcmApiFunction::GCMService() const {
Why changing it to GCMService? The gcm API is tied with a profile.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/extension...
chrome/browser/extensions/extension_gcm_app_handler.cc:72:
GetGCMService()->RemoveAppHandler((*extension)->id());
Why? Extension is tied with profile.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service.cc:405: gcm::GCMService*
gcm_service =
ditto

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_service.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.cc:184:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
I do not think IOWorker can be destructed on IO thread, even given your current
logic.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.cc:428:
base::Unretained(io_worker_.get())));
base::Unretained should almost never be used in non-test code. This will almost
definitely cause the crash. Say, when GCMService is disposed, it causes
io_worker_ to be destructed because you switch to use scoped_ptr. If there is
still a pending task, we will get a crash. Have you run all GCM unittests in
memory bots?

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_service.h (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:106: //
InvalidationAuthProvider::Observer:
This seems to be app specific logic and it should not live in
GCMService/GCMProfileService. It should be put within the AppHandler for the
invalidation.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:111: virtual bool IsAlwaysEnabled() =
0;
This is kind of confusing. You should move GetGCMEnabledState to this base class
and let it return ALWAYS_ENABLED.

However, I do not think you should bypass the GCM enabling/disabling logic for
CrOS device-level invalidation. The disabling logic is very useful to deploy the
kill to GCM connection in case something goes wrong.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:116:
scoped_ptr<invalidation::InvalidationAuthProvider> auth_provider_;
This should not live here. GCMService should not have knowledge about
invalidation. It should be factored into AppHandler.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:171:
scoped_refptr<net::URLRequestContextGetter> request_context_;
This should be obtained by calling virtual method GetURLRequestContextGetter.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:173: const base::FilePath store_path_;
ditto.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:186: scoped_ptr<IOWorker> io_worker_;
Why do you change to use scoped_ptr?

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:1016: gcm::GCMService* gcm_service =
GCMProfileServiceFactory::GetForProfile returns GCMProfileService. Why bothering
to casting to GCMService?

[Yoyo Zhou, 2014-04-10 21:12:04]

extensions OWNERS rubber stamp LGTM

[bartfab (slow), 2014-04-11 16:58:52]

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/extension...
chrome/browser/extensions/api/gcm/gcm_api.cc:101: gcm::GCMService*
GcmApiFunction::GCMService() const {
On 2014/04/10 20:39:56, jianli wrote:
> Why changing it to GCMService? The gcm API is tied with a profile.

The GCM API requires a profile. But there are other consumers of this service. I
am specifically working on making TiclInvalidationService profile-free because I
need to have an instance running at the Chrome OS device level, where I do have
credentials for a GAIA robot account but that account is not tied to any
profile. Since TiclInvalidationService uses GCMProfileService, I need a
profile-free variant of it.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/extension...
File chrome/browser/extensions/extension_gcm_app_handler.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/extension...
chrome/browser/extensions/extension_gcm_app_handler.cc:72:
GetGCMService()->RemoveAppHandler((*extension)->id());
On 2014/04/10 20:39:56, jianli wrote:
> Why? Extension is tied with profile.

I had made the change here to make it explicit that this particular piece of
code does not require the profile-specific version of GCMService. If you prefer
GCMProfileService being used here, no problem - I changed it back to the way it
was in extension_gcm_app_handler.*.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/invalidat...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/invalidat...
chrome/browser/invalidation/ticl_invalidation_service.cc:405: gcm::GCMService*
gcm_service =
On 2014/04/10 20:39:56, jianli wrote:
> ditto

This is the core of my work - I am removing the Profile dependency from
TiclInvalidationService. With this CL, TiclInvalidationService still gets its
GCMService from the GCMProfileServiceFactory. The next step will be to pass it
in as a dependency, so that TiclInvalidationService really only has a GCMService
and does not access GCMProfile* anymore.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_service.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.cc:184:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
On 2014/04/10 20:39:56, jianli wrote:
> I do not think IOWorker can be destructed on IO thread, even given your
current
> logic.

Since it does work on the IO thread, it must be destroyed on that thread.
Otherwise, the work it is currently doing and destruction race against each
other.

I ran Chrome and the tests in debug, with DCHECK enabled, and everything passed.
The IOWorker is being properly torn down on the IO thread now.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.cc:428:
base::Unretained(io_worker_.get())));
On 2014/04/10 20:39:56, jianli wrote:
> base::Unretained should almost never be used in non-test code. This will
almost
> definitely cause the crash. Say, when GCMService is disposed, it causes
> io_worker_ to be destructed because you switch to use scoped_ptr. If there is
> still a pending task, we will get a crash. Have you run all GCM unittests in
> memory bots?

Used correctly, base::Unretained is both useful and necessary in production
code. It is used whenever you want to bind a pointer without passing ownership.
This is exactly what happens here. The scoped_ptr is only used to indicate
ownership. Note that the scoped_ptr will never be allowed to destroy the
IOWorker:

The owner of GCMService must call its Shutdown() method before destroying the
service. Shutdown() will explicitly release() the io_worker_ scoped_ptr and pass
ownership to DeleteSoon(), which will post a task to destroy the IOWorker on the
IO thread's message loop. The nature of message loops is that they process tasks
sequentially. This means that any tasks previously sent to the IO thread,
referencing base::Unretained(io_worker_.get()), will get to run before the
IOWorker is destroyed.

I will run all the tests on the bots of course but I am addressing review
comments first.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_service.h (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:106: //
InvalidationAuthProvider::Observer:
On 2014/04/10 20:39:56, jianli wrote:
> This seems to be app specific logic and it should not live in
> GCMService/GCMProfileService. It should be put within the AppHandler for the
> invalidation.

Before this CL, GCMProfileService was an SigninManagerBase::Observer and
received GoogleSigninSucceeded() / GoogleSignedOut() notifications.

With this CL, GCMService no longer has direct access to SigninManagerBase. The
same notifications need to be forwarded by an intermediary class now.

The intermediary that I used was originally written for invalidation, which is
why it was called InvalidationAuthProvider and invoked methods that have
"Invalidation" in their name. I see that this led to confusion. To address this,
I renamed the intermediary class to IdentityProvider (because it just provides
access to GAIA identity details and tokens - it has absolutely nothing to do
with invalidation). This renaming is CL 235273002. With that, the methods are
called OnLogin() / OnLogout() now and are the direct equivalents of
GoogleSigninSucceeded() / GoogleSignedOut() that GCMProfileService used before.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:111: virtual bool IsAlwaysEnabled() =
0;
On 2014/04/10 20:39:56, jianli wrote:
> This is kind of confusing. You should move GetGCMEnabledState to this base
class
> and let it return ALWAYS_ENABLED.
> 
> However, I do not think you should bypass the GCM enabling/disabling logic for
> CrOS device-level invalidation. The disabling logic is very useful to deploy
the
> kill to GCM connection in case something goes wrong.

The reason I implemented it like this is that for the generic base class, there
is only a distinction between always enabled and not always enabled. The more
granular distinction between positive/negative/neutral signals only comes into
play when a GCMService is tied to a profile. The device-level GCMService will
not need this level of control.

And yes, of course the device-level GCMService will have logic to disable it.
The signal will just be different - instead of a profile pref, it will be a
local state pref.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:116:
scoped_ptr<invalidation::InvalidationAuthProvider> auth_provider_;
On 2014/04/10 20:39:56, jianli wrote:
> This should not live here. GCMService should not have knowledge about
> invalidation. It should be factored into AppHandler.

As above, sorry for the confusion. This is just an interface for accessing GAIA
sign-in state. I renamed the class to IdentityProvider to make it clear that it
has absolutely nothing to do with invalidation.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:171:
scoped_refptr<net::URLRequestContextGetter> request_context_;
On 2014/04/10 20:39:56, jianli wrote:
> This should be obtained by calling virtual method GetURLRequestContextGetter.

Done.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:173: const base::FilePath store_path_;
On 2014/04/10 20:39:56, jianli wrote:
> ditto.

Done.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.h:186: scoped_ptr<IOWorker> io_worker_;
On 2014/04/10 20:39:56, jianli wrote:
> Why do you change to use scoped_ptr?

To make the ownership explicit. The previous implementation using scoped_refptr
was wrong in two ways:

1) With scoped_refptr, the IOWorker will be destroyed when the last reference to
it is released. Since references are held on different threads, this means that
it is undefined which thread the destructor will run on.

2) The above actually did lead to destruction on the wrong thread in practice:
When GCMProfileService is shutting down (on the UI thread), it happens to
release the last reference to the IOWorker. This causes ~IOWorker() to runn on
the UI thread while the object may actually still be doing something on the IO
thread, causing a race and potential for unallocated memory reads/writes.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/sync/prof...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/sync/prof...
chrome/browser/sync/profile_sync_service.cc:1016: gcm::GCMService* gcm_service =
On 2014/04/10 20:39:56, jianli wrote:
> GCMProfileServiceFactory::GetForProfile returns GCMProfileService. Why
bothering
> to casting to GCMService?

I wanted to make it explicit that in this case here, no Profile-specific
functionality of GCMService is needed. But you are right, in a class that has
"Profile" in its name, using the Profile-specific version of GCMService is no
crime. Fixed.

[jianli, 2014-04-17 00:33:18]

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_service.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.cc:428:
base::Unretained(io_worker_.get())));
On 2014/04/11 16:58:52, bartfab wrote:
> On 2014/04/10 20:39:56, jianli wrote:
> > base::Unretained should almost never be used in non-test code. This will
> almost
> > definitely cause the crash. Say, when GCMService is disposed, it causes
> > io_worker_ to be destructed because you switch to use scoped_ptr. If there
is
> > still a pending task, we will get a crash. Have you run all GCM unittests in
> > memory bots?
> 
> Used correctly, base::Unretained is both useful and necessary in production
> code. It is used whenever you want to bind a pointer without passing
ownership.
> This is exactly what happens here. The scoped_ptr is only used to indicate
> ownership. Note that the scoped_ptr will never be allowed to destroy the
> IOWorker:
> 
> The owner of GCMService must call its Shutdown() method before destroying the
> service. Shutdown() will explicitly release() the io_worker_ scoped_ptr and
pass
> ownership to DeleteSoon(), which will post a task to destroy the IOWorker on
the
> IO thread's message loop. The nature of message loops is that they process
tasks
> sequentially. This means that any tasks previously sent to the IO thread,
> referencing base::Unretained(io_worker_.get()), will get to run before the
> IOWorker is destroyed.
> 
> I will run all the tests on the bots of course but I am addressing review
> comments first.

I understand this, but it still seems to be dangerous. Calling DeleteSoon and
passing reference via base::Unretained do not cover all scenarios. For example,
what if we post a delayed task to IOWorker instance? I know currently we do not
have scenario like this, but I have concern we might get wield crash in the
future when some other people is changing the code.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/extensio...
chrome/browser/extensions/api/gcm/gcm_api.cc:101: gcm::GCMService*
GcmApiFunction::GCMService() const {
GCMProfileServiceFactory::GetForProfile returns GCMProfileService. There is no
need to convert it to GCMService. Since gcm API is tied with a profile, we might
call certain GCMProfileService's functionality from here in the future.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:28: // A specialization of
GCMService that is a KeyedService.
A specialization of GCMService that is tied with a profile.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (left):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:494:
TEST_F(GCMProfileServiceTest, RegisterUnderNeutralChannelSignal) {
I think we still need to keep this test for GCMProfileService.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:535:
TEST_F(GCMProfileServiceTest, SendUnderNeutralChannelSignal) {
ditto

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service.h (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:56: void Shutdown();
The name Shutdown is same as the one defined in GCMProfileService while they
mean different things and one of them is virtual while the other is not. We'd
better use different name.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:109: virtual bool IsAlwaysEnabled() =
0;
nit: add const modifier

Also, I think IsEnabled should be good enough since we do not have the 3rd state
here.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:111: virtual base::FilePath
GetStorePath() = 0;
nit: add const modifier

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:113: virtual
scoped_refptr<net::URLRequestContextGetter>
ditto

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service_unittest.cc (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:56: class
FakeOAuth2TokenService : public OAuth2TokenService {
Should we move this and FakeIdentityProvider to
gcm_profile_service_test_helper.h to replace FakeSigninManager?

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:475: bool wait) {
nit: better use enum to improve readability

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:882: PumpIOLoop();
Why so many PumpIOLoop? Please explain like in original code.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:1018: PumpIOLoop();
ditto

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:1116:
scoped_ptr<TestGCMServiceWrapper> wrapper_2_;
wrapper2_

[bartfab (slow), 2014-04-17 14:20:00]

Hi Yoyo,

I addressed most of your comments. I will address the remaining comments as well
of course but the 4-day Easter break is coming up and I wanted to send you as
much to re-review as I could. Please take a look at the changes that do address
your comments already.

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_service.cc (right):

https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_service.cc:428:
base::Unretained(io_worker_.get())));
On 2014/04/17 00:33:18, jianli wrote:
> On 2014/04/11 16:58:52, bartfab wrote:
> > On 2014/04/10 20:39:56, jianli wrote:
> > > base::Unretained should almost never be used in non-test code. This will
> > almost
> > > definitely cause the crash. Say, when GCMService is disposed, it causes
> > > io_worker_ to be destructed because you switch to use scoped_ptr. If there
> is
> > > still a pending task, we will get a crash. Have you run all GCM unittests
in
> > > memory bots?
> > 
> > Used correctly, base::Unretained is both useful and necessary in production
> > code. It is used whenever you want to bind a pointer without passing
> ownership.
> > This is exactly what happens here. The scoped_ptr is only used to indicate
> > ownership. Note that the scoped_ptr will never be allowed to destroy the
> > IOWorker:
> > 
> > The owner of GCMService must call its Shutdown() method before destroying
the
> > service. Shutdown() will explicitly release() the io_worker_ scoped_ptr and
> pass
> > ownership to DeleteSoon(), which will post a task to destroy the IOWorker on
> the
> > IO thread's message loop. The nature of message loops is that they process
> tasks
> > sequentially. This means that any tasks previously sent to the IO thread,
> > referencing base::Unretained(io_worker_.get()), will get to run before the
> > IOWorker is destroyed.
> > 
> > I will run all the tests on the bots of course but I am addressing review
> > comments first.
> 
> I understand this, but it still seems to be dangerous. Calling DeleteSoon and
> passing reference via base::Unretained do not cover all scenarios. For
example,
> what if we post a delayed task to IOWorker instance? I know currently we do
not
> have scenario like this, but I have concern we might get wield crash in the
> future when some other people is changing the code.

The way to ensure safe and predictable operation is to have clear ownership. Ref
counting is dangerous because it makes ownership entirely unclear. An object
will be destroyed at an unpredictable point in time, possibly even on an
unpredictable thread, whenever the last reference is dropped. You can find
multiple rants (as he likes to call them :) on chromium-dev where Will Chan
explains why ownership should form a DAG and ref counting is bad.

This code here uses a paradigm that is quite common in Chrome: You have a class
with an outward-facing interface that lives on the UI thread. Internally, the
class has a Worker that lives on IO. The outer class owns the Worker and makes
sure to post its destruction to the IO thread when it is being destroyed itself.

All interactions with the Worker go through the outer class. Raw pointers are
inherently safe because nobody can access the Worker when the parent class has
shut down (and Worker destruction has been posted).

As for delayed tasks: A delayed task should never be posted using a raw pointer.
Even if you post it to yourself, you may no longer exist when the task runs. The
solution are weak pointers. Whenever you post a task, whether to yourself or
someone else, just use a weak pointer so that the task cancels itself if the
recipient no longer exists. Of course weak pointers are not thread safe but
that's OK:

- If the outer class wants to post a delayed task, it should post it to itself
via a weak pointer that lives on the UI thread. The method invoked by this task
would then post an immediate task to the worker on the IO thread.
- If the Worker wants to post a delayed task, it should also post it to itself,
via a weak pointer that lives on the IO thread.

Posting a delayed task to another object, across thread boundaries, is bad code
that should not be patched over by muddying ownership via ref counting. There is
a safe way to use delayed tasks without ref counting. And if you truly dislike
weak pointers, you can use base::Timer, which handles the cancelation for you if
it happens to go out of scope before the delayed task has executed.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/extensio...
File chrome/browser/extensions/api/gcm/gcm_api.cc (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/extensio...
chrome/browser/extensions/api/gcm/gcm_api.cc:101: gcm::GCMService*
GcmApiFunction::GCMService() const {
On 2014/04/17 00:33:19, jianli wrote:
> GCMProfileServiceFactory::GetForProfile returns GCMProfileService. There is no
> need to convert it to GCMService. Since gcm API is tied with a profile, we
might
> call certain GCMProfileService's functionality from here in the future.

Done.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.h:28: // A specialization of
GCMService that is a KeyedService.
On 2014/04/17 00:33:19, jianli wrote:
> A specialization of GCMService that is tied with a profile.

Done.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service.h (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:56: void Shutdown();
On 2014/04/17 00:33:19, jianli wrote:
> The name Shutdown is same as the one defined in GCMProfileService while they
> mean different things and one of them is virtual while the other is not. We'd
> better use different name.

Oops, GCMService::Shutdown() should have been virtual. I fixed that now. The
Shutdown() methods in GCMService and GCMProfileService serve exactly the same
purpose. Both are meant to be called just before the object is destroyed. I
added a comment explaining this.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:109: virtual bool IsAlwaysEnabled() =
0;
On 2014/04/17 00:33:19, jianli wrote:
> nit: add const modifier
> 
> Also, I think IsEnabled should be good enough since we do not have the 3rd
state
> here.

const: Done.

Renaming: I think "IsEnabled" is not a good name because it implies the wrong
thing. If IsEnabled() == false, you would think that the service is not enabled
== disabled. However, the method only indicates whether the service should start
automatically, unconditionally. Even if the method returns false, the service
should still start up if an extension uses the GCM API. I renamed the method to
"StartAutomatically" instead. I think this is clearer.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:111: virtual base::FilePath
GetStorePath() = 0;
On 2014/04/17 00:33:19, jianli wrote:
> nit: add const modifier

Done.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:113: virtual
scoped_refptr<net::URLRequestContextGetter>
On 2014/04/17 00:33:19, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service_unittest.cc (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:56: class
FakeOAuth2TokenService : public OAuth2TokenService {
On 2014/04/17 00:33:19, jianli wrote:
> Should we move this and FakeIdentityProvider to
> gcm_profile_service_test_helper.h to replace FakeSigninManager?

Done. While at it, I took a closer look at gcm_profile_service_test_helper.h:
This file is included by two tests (gcm_service_unittest.cc and
extension_gcm_app_handler_unittest.cc) but there is only a single class that
both actually need, FakeGCMClientFactory. So I moved all the classes only needed
by one of the tests into the respective test, fixed up the includes (IWYU was
not really being followed in any of the code) and renamed
gcm_profile_service_test_helper.h to fake_gcm_client_factory.h because this is
the only class it now contains.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:1116:
scoped_ptr<TestGCMServiceWrapper> wrapper_2_;
On 2014/04/17 00:33:19, jianli wrote:
> wrapper2_

Done.

[Yoyo Zhou, 2014-04-17 16:10:53]

On Thu, Apr 17, 2014 at 7:20 AM, <bartfab@chromium.org> wrote:

> Hi Yoyo,
>
> I addressed most of your comments. I will address the remaining comments
> as well
> of course but the 4-day Easter break is coming up and I wanted to send you
> as
> much to re-review as I could. Please take a look at the changes that do
> address
> your comments already.
>
> Just to be clear, those are Jian's comments, not mine. =)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Munjal (Google), 2014-04-17 18:51:24]

This refactoring is useful. But several high level comments:
- The patch seems to be too big. Is there a way to break down work into multiple
smaller patches (if it is not too much work).
- There are cases in which GCM needs to be used without Gaia. For such cases, we
need to decouple GCMService from sign in / sign out concept. Basically we need
to remove the dependency on IdneityProvider at GCMService level.
- Basically GCMService can just do thread management and nothing else. Other
thigns can be done on top of that.

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service.h (right):

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:108: virtual void OnLogout() OVERRIDE;
We should remove dependency on login / logout concept also at this level if
possible. There are use cases to use GCM without Gaia accounts.

[jianli, 2014-04-17 19:05:58]

On Thu, Apr 17, 2014 at 11:51 AM, <munjal@chromium.org> wrote:

> This refactoring is useful. But several high level comments:
> - The patch seems to be too big. Is there a way to break down work into
> multiple
> smaller patches (if it is not too much work).
>

I agree on this. We should try to put any identity provider change in other
patch.


> - There are cases in which GCM needs to be used without Gaia. For such
> cases, we
> need to decouple GCMService from sign in / sign out concept. Basically we
> need
> to remove the dependency on IdneityProvider at GCMService level.
>

I am doing design on this to remove the signin enforcement. I rather leave
it as it is in order not to bring more complexity to the whole patch.


> - Basically GCMService can just do thread management and nothing else.
> Other
> thigns can be done on top of that.
>
>
> https://codereview.chromium.org/225403021/diff/200020/
> chrome/browser/services/gcm/gcm_service.h
> File chrome/browser/services/gcm/gcm_service.h (right):
>
> https://codereview.chromium.org/225403021/diff/200020/
> chrome/browser/services/gcm/gcm_service.h#newcode108
> chrome/browser/services/gcm/gcm_service.h:108: virtual void OnLogout()
> OVERRIDE;
> We should remove dependency on login / logout concept also at this level
> if possible. There are use cases to use GCM without Gaia accounts.
>
> https://codereview.chromium.org/225403021/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jianli, 2014-04-18 01:35:52]

On 2014/04/17 14:20:00, bartfab wrote:
> Hi Yoyo,
> 
> I addressed most of your comments. I will address the remaining comments as
well
> of course but the 4-day Easter break is coming up and I wanted to send you as
> much to re-review as I could. Please take a look at the changes that do
address
> your comments already.
> 
>
https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
> File chrome/browser/services/gcm/gcm_service.cc (right):
> 
>
https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
> chrome/browser/services/gcm/gcm_service.cc:428:
> base::Unretained(io_worker_.get())));
> On 2014/04/17 00:33:18, jianli wrote:
> > On 2014/04/11 16:58:52, bartfab wrote:
> > > On 2014/04/10 20:39:56, jianli wrote:
> > > > base::Unretained should almost never be used in non-test code. This will
> > > almost
> > > > definitely cause the crash. Say, when GCMService is disposed, it causes
> > > > io_worker_ to be destructed because you switch to use scoped_ptr. If
there
> > is
> > > > still a pending task, we will get a crash. Have you run all GCM
unittests
> in
> > > > memory bots?
> > > 
> > > Used correctly, base::Unretained is both useful and necessary in
production
> > > code. It is used whenever you want to bind a pointer without passing
> > ownership.
> > > This is exactly what happens here. The scoped_ptr is only used to indicate
> > > ownership. Note that the scoped_ptr will never be allowed to destroy the
> > > IOWorker:
> > > 
> > > The owner of GCMService must call its Shutdown() method before destroying
> the
> > > service. Shutdown() will explicitly release() the io_worker_ scoped_ptr
and
> > pass
> > > ownership to DeleteSoon(), which will post a task to destroy the IOWorker
on
> > the
> > > IO thread's message loop. The nature of message loops is that they process
> > tasks
> > > sequentially. This means that any tasks previously sent to the IO thread,
> > > referencing base::Unretained(io_worker_.get()), will get to run before the
> > > IOWorker is destroyed.
> > > 
> > > I will run all the tests on the bots of course but I am addressing review
> > > comments first.
> > 
> > I understand this, but it still seems to be dangerous. Calling DeleteSoon
and
> > passing reference via base::Unretained do not cover all scenarios. For
> example,
> > what if we post a delayed task to IOWorker instance? I know currently we do
> not
> > have scenario like this, but I have concern we might get wield crash in the
> > future when some other people is changing the code.
> 
> The way to ensure safe and predictable operation is to have clear ownership.
Ref
> counting is dangerous because it makes ownership entirely unclear. An object
> will be destroyed at an unpredictable point in time, possibly even on an
> unpredictable thread, whenever the last reference is dropped. You can find
> multiple rants (as he likes to call them :) on chromium-dev where Will Chan
> explains why ownership should form a DAG and ref counting is bad.

I agree on that we should avoid using ref pointers if possible. I just have the
concern on passing raw pointer and using DeleteSoon to do the work. The example
I gave last time was just to illustrate my concern, it might not be a well
thought solid example. My concern is that relying on DeleteSoon might not work
all the cases. Forgive me if I do not understand all the DeleteSoon logic
correctly. What if we have the following case
1) GCMService::Foo posts task to GCMService::IOWorker::Bar
2) DeleteSoon is called from GCMService::Shutdown
3) GCMService::IOWorker::Bar is executed and it posts a task to itself
Will DeleteSoon handle this case?

[jianli, 2014-04-18 01:49:43]

You have not addressed my comment that 2 tests in
gcm_profile_service_unittest.cc were removed without any replacement.

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service.cc (right):

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.cc:221: gcm_client_.reset();
I think we can now rely on the destructor to disposing gcm_client_ since the
destructor is called on IO thread. Thus we don't need to call Reset() any more.

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service.h (right):

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:58: virtual void Shutdown();
GCMProfileService is inherited from both GCMService and KeyedService and both of
them define Shutdown. I don't think it is a good practice to use the same name.
Can we rename this to something like ShutdownService?

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:111: virtual bool StartAutomatically()
const = 0;
CanStartAutomatically?

[bartfab (slow), 2014-04-22 17:51:06]

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (left):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:494:
TEST_F(GCMProfileServiceTest, RegisterUnderNeutralChannelSignal) {
On 2014/04/17 00:33:19, jianli wrote:
> I think we still need to keep this test for GCMProfileService.

Done.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:535:
TEST_F(GCMProfileServiceTest, SendUnderNeutralChannelSignal) {
On 2014/04/17 00:33:19, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service_unittest.cc (right):

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:475: bool wait) {
On 2014/04/17 00:33:19, jianli wrote:
> nit: better use enum to improve readability

Done.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:882: PumpIOLoop();
On 2014/04/17 00:33:19, jianli wrote:
> Why so many PumpIOLoop? Please explain like in original code.

I replaced this with a proper wait until a notification is received that the
operation has completed.

https://codereview.chromium.org/225403021/diff/210001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:1018: PumpIOLoop();
On 2014/04/17 00:33:19, jianli wrote:
> ditto

I replaced this with a proper wait until a notification is received that the
operation has completed.

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service.cc (right):

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.cc:221: gcm_client_.reset();
On 2014/04/18 01:49:44, jianli wrote:
> I think we can now rely on the destructor to disposing gcm_client_ since the
> destructor is called on IO thread. Thus we don't need to call Reset() any
more.

Done.

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service.h (right):

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:58: virtual void Shutdown();
On 2014/04/18 01:49:44, jianli wrote:
> GCMProfileService is inherited from both GCMService and KeyedService and both
of
> them define Shutdown. I don't think it is a good practice to use the same
name.
> Can we rename this to something like ShutdownService? 

Done.

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:108: virtual void OnLogout() OVERRIDE;
On 2014/04/17 18:51:25, Munjal (Google) wrote:
> We should remove dependency on login / logout concept also at this level if
> possible. There are use cases to use GCM without Gaia accounts.

As commented by Jian, he is working on a design that will remove the account
dependency from GCM in a follow-up step.

https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
chrome/browser/services/gcm/gcm_service.h:111: virtual bool StartAutomatically()
const = 0;
On 2014/04/18 01:49:44, jianli wrote:
> CanStartAutomatically?

GCM always can start automatically. The question is whether it should.
Therefore, I renamed the method to ShouldStartAutomatically().

[bartfab (slow), 2014-04-22 17:51:51]

On 2014/04/17 16:10:53, Yoyo Zhou wrote:
> On Thu, Apr 17, 2014 at 7:20 AM, <mailto:bartfab@chromium.org> wrote:
> 
> > Hi Yoyo,
> >
> > I addressed most of your comments. I will address the remaining comments
> > as well
> > of course but the 4-day Easter break is coming up and I wanted to send you
> > as
> > much to re-review as I could. Please take a look at the changes that do
> > address
> > your comments already.
> >
> > Just to be clear, those are Jian's comments, not mine. =)

Sorry, I meant Jian :).

> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[bartfab (slow), 2014-04-22 17:56:46]

On 2014/04/17 19:05:58, jianli wrote:
> On Thu, Apr 17, 2014 at 11:51 AM, <mailto:munjal@chromium.org> wrote:
> 
> > This refactoring is useful. But several high level comments:
> > - The patch seems to be too big. Is there a way to break down work into
> > multiple
> > smaller patches (if it is not too much work).
> >
> 
> I agree on this. We should try to put any identity provider change in other
> patch.

I can make another CL that adds the IdentityProvider but that will not reduce
the size of this CL. As you can see by looking at the per-file diffs, the size
comes from the fact most of GCMProfileService moves to a new GCMService class,
leading to -1050 lines in gcm_profile_service.[h|cc], -1150 lines in
gcm_profile_service_unittest.cc and +1320 lines in gcm_service_unittest.cc as
the implementation and tests move to their new place.

> 
> 
> > - There are cases in which GCM needs to be used without Gaia. For such
> > cases, we
> > need to decouple GCMService from sign in / sign out concept. Basically we
> > need
> > to remove the dependency on IdneityProvider at GCMService level.
> >
> 
> I am doing design on this to remove the signin enforcement. I rather leave
> it as it is in order not to bring more complexity to the whole patch.
> 
> 
> > - Basically GCMService can just do thread management and nothing else.
> > Other
> > thigns can be done on top of that.
> >
> >
> > https://codereview.chromium.org/225403021/diff/200020/
> > chrome/browser/services/gcm/gcm_service.h
> > File chrome/browser/services/gcm/gcm_service.h (right):
> >
> > https://codereview.chromium.org/225403021/diff/200020/
> > chrome/browser/services/gcm/gcm_service.h#newcode108
> > chrome/browser/services/gcm/gcm_service.h:108: virtual void OnLogout()
> > OVERRIDE;
> > We should remove dependency on login / logout concept also at this level
> > if possible. There are use cases to use GCM without Gaia accounts.
> >
> > https://codereview.chromium.org/225403021/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[bartfab (slow), 2014-04-22 17:59:45]

On 2014/04/18 01:35:52, jianli wrote:
> On 2014/04/17 14:20:00, bartfab wrote:
> > Hi Yoyo,
> > 
> > I addressed most of your comments. I will address the remaining comments as
> well
> > of course but the 4-day Easter break is coming up and I wanted to send you
as
> > much to re-review as I could. Please take a look at the changes that do
> address
> > your comments already.
> > 
> >
>
https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
> > File chrome/browser/services/gcm/gcm_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/225403021/diff/50001/chrome/browser/services/...
> > chrome/browser/services/gcm/gcm_service.cc:428:
> > base::Unretained(io_worker_.get())));
> > On 2014/04/17 00:33:18, jianli wrote:
> > > On 2014/04/11 16:58:52, bartfab wrote:
> > > > On 2014/04/10 20:39:56, jianli wrote:
> > > > > base::Unretained should almost never be used in non-test code. This
will
> > > > almost
> > > > > definitely cause the crash. Say, when GCMService is disposed, it
causes
> > > > > io_worker_ to be destructed because you switch to use scoped_ptr. If
> there
> > > is
> > > > > still a pending task, we will get a crash. Have you run all GCM
> unittests
> > in
> > > > > memory bots?
> > > > 
> > > > Used correctly, base::Unretained is both useful and necessary in
> production
> > > > code. It is used whenever you want to bind a pointer without passing
> > > ownership.
> > > > This is exactly what happens here. The scoped_ptr is only used to
indicate
> > > > ownership. Note that the scoped_ptr will never be allowed to destroy the
> > > > IOWorker:
> > > > 
> > > > The owner of GCMService must call its Shutdown() method before
destroying
> > the
> > > > service. Shutdown() will explicitly release() the io_worker_ scoped_ptr
> and
> > > pass
> > > > ownership to DeleteSoon(), which will post a task to destroy the
IOWorker
> on
> > > the
> > > > IO thread's message loop. The nature of message loops is that they
process
> > > tasks
> > > > sequentially. This means that any tasks previously sent to the IO
thread,
> > > > referencing base::Unretained(io_worker_.get()), will get to run before
the
> > > > IOWorker is destroyed.
> > > > 
> > > > I will run all the tests on the bots of course but I am addressing
review
> > > > comments first.
> > > 
> > > I understand this, but it still seems to be dangerous. Calling DeleteSoon
> and
> > > passing reference via base::Unretained do not cover all scenarios. For
> > example,
> > > what if we post a delayed task to IOWorker instance? I know currently we
do
> > not
> > > have scenario like this, but I have concern we might get wield crash in
the
> > > future when some other people is changing the code.
> > 
> > The way to ensure safe and predictable operation is to have clear ownership.
> Ref
> > counting is dangerous because it makes ownership entirely unclear. An object
> > will be destroyed at an unpredictable point in time, possibly even on an
> > unpredictable thread, whenever the last reference is dropped. You can find
> > multiple rants (as he likes to call them :) on chromium-dev where Will Chan
> > explains why ownership should form a DAG and ref counting is bad.
> 
> I agree on that we should avoid using ref pointers if possible. I just have
the
> concern on passing raw pointer and using DeleteSoon to do the work. The
example
> I gave last time was just to illustrate my concern, it might not be a well
> thought solid example. My concern is that relying on DeleteSoon might not work
> all the cases. Forgive me if I do not understand all the DeleteSoon logic
> correctly. What if we have the following case
> 1) GCMService::Foo posts task to GCMService::IOWorker::Bar
> 2) DeleteSoon is called from GCMService::Shutdown
> 3) GCMService::IOWorker::Bar is executed and it posts a task to itself
> Will DeleteSoon handle this case?

In step 3), when the IOWorker is posting a task to itself, it should *always*
use a WeakPtr to itself, not a base::Unretained() pointer. This is necessary
because the IOWorker does not control its own lifetime (GMService does) and
cannot predict whether it will still be alive when a task posted to the future
runs. As long as it does that, the code will be perfectly safe. If the code uses
base::Unretained() while posting a task to itself in the future, the code is
buggy and will crash if a DeleteSoon happens in between.

[bartfab (slow), 2014-04-22 18:00:37]

On 2014/04/18 01:49:43, jianli wrote:
> You have not addressed my comment that 2 tests in
> gcm_profile_service_unittest.cc were removed without any replacement.

Yes. As I said, I did not get all the way through your comments before the
Easter weekends. All your comments are addressed now and the two tests have been
reinstated.

> 
>
https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
> File chrome/browser/services/gcm/gcm_service.cc (right):
> 
>
https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
> chrome/browser/services/gcm/gcm_service.cc:221: gcm_client_.reset();
> I think we can now rely on the destructor to disposing gcm_client_ since the
> destructor is called on IO thread. Thus we don't need to call Reset() any
more.
> 
>
https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
> File chrome/browser/services/gcm/gcm_service.h (right):
> 
>
https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
> chrome/browser/services/gcm/gcm_service.h:58: virtual void Shutdown();
> GCMProfileService is inherited from both GCMService and KeyedService and both
of
> them define Shutdown. I don't think it is a good practice to use the same
name.
> Can we rename this to something like ShutdownService?
> 
>
https://codereview.chromium.org/225403021/diff/200020/chrome/browser/services...
> chrome/browser/services/gcm/gcm_service.h:111: virtual bool
StartAutomatically()
> const = 0;
> CanStartAutomatically?

[jianli, 2014-04-23 00:42:31]

lgtm

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:49: class
FakeSigninManager : public SigninManagerBase {
This has been defined in both extension_gcm_app_handler_unittest.cc and this
file. Could you please put it in a common place?

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service_unittest.cc (right):

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:204: enum AsyncBehavior {
nit: the name sounds more like asynchronous execution. How about something like
WaitingToFinish?

[dcheng, 2014-04-23 01:13:58]

chrome/browser/invalidation/* LGTM with one question

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/invalida...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/invalida...
chrome/browser/invalidation/ticl_invalidation_service.cc:432: if
(gcm::GCMProfileService::GetGCMEnabledState(profile_) ==
Do we need to update this as well to indirect through GCMService? Or is that
something for a future CL?

[bartfab (slow), 2014-04-23 11:33:07]

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/invalida...
File chrome/browser/invalidation/ticl_invalidation_service.cc (right):

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/invalida...
chrome/browser/invalidation/ticl_invalidation_service.cc:432: if
(gcm::GCMProfileService::GetGCMEnabledState(profile_) ==
On 2014/04/23 01:13:58, dcheng wrote:
> Do we need to update this as well to indirect through GCMService? Or is that
> something for a future CL?

Yes, this will be done in a follow-up CL that removes the Profile dependency
from TiclInvalidationService for good.

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_unittest.cc (right):

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_unittest.cc:49: class
FakeSigninManager : public SigninManagerBase {
On 2014/04/23 00:42:32, jianli wrote:
> This has been defined in both extension_gcm_app_handler_unittest.cc and this
> file. Could you please put it in a common place?

Done.

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_service_unittest.cc (right):

https://codereview.chromium.org/225403021/diff/260001/chrome/browser/services...
chrome/browser/services/gcm/gcm_service_unittest.cc:204: enum AsyncBehavior {
On 2014/04/23 00:42:32, jianli wrote:
> nit: the name sounds more like asynchronous execution. How about something
like
> WaitingToFinish?

Well, it is async execution on another thread. But WaitToFinish is even clearer.
Renamed.

[bartfab (slow), 2014-04-23 11:34:03]

Hi Drew,

You are the only remaining reviewer I need on this CL.

[Andrew T Wilson (Slow), 2014-04-24 08:35:09]

lgtm

https://codereview.chromium.org/225403021/diff/280001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/225403021/diff/280001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:32: #include
"chrome/browser/services/gcm/gcm_profile_service.h"
Do we still need this include (gcm_profile_service.h)?

[bartfab (slow), 2014-04-24 08:41:01]

https://codereview.chromium.org/225403021/diff/280001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/225403021/diff/280001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.cc:32: #include
"chrome/browser/services/gcm/gcm_profile_service.h"
On 2014/04/24 08:35:10, Andrew T Wilson wrote:
> Do we still need this include (gcm_profile_service.h)?

Yes, because GCMProfileServiceFactory::GetForProfile() returns a
GCMProfileService and the type must be fully defined to cast it to something
else (GCMService in this case).

However, I just realized that I should roll back the changes in this file anyway
- there is no need to cast from the Profile-dependent GCMProfileService to a
Profile-independent GCMService in a file that so obviously completely depends on
Profiles anyway (even the file name has "profile" in it :).

[bartfab (slow), 2014-04-24 08:41:17]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-24 08:41:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/225403021/300001

[commit-bot: I haz the power, 2014-04-24 09:29:38]

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

[commit-bot: I haz the power, 2014-04-24 09:29:39]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[bartfab (slow), 2014-04-24 09:36:34]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-24 09:36:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/225403021/320001

[commit-bot: I haz the power, 2014-04-24 11:31:10]

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

[commit-bot: I haz the power, 2014-04-24 11:31:11]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg
  tryserver.chromium on linux_chromium_clang_dbg
  tryserver.chromium on mac_chromium_compile_dbg
  tryserver.chromium on mac_chromium_rel
  tryserver.chromium on win_chromium_rel

[bartfab (slow), 2014-04-24 11:34:01]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-24 11:34:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/225403021/330025

[commit-bot: I haz the power, 2014-04-24 13:30:24]

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

[commit-bot: I haz the power, 2014-04-24 13:30:25]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[bartfab (slow), 2014-04-24 13:55:19]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-24 14:16:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/225403021/350001

[commit-bot: I haz the power, 2014-04-24 16:16:36]

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

[commit-bot: I haz the power, 2014-04-24 16:16:37]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[bartfab (slow), 2014-04-24 16:31:07]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-24 16:31:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/225403021/350001

[commit-bot: I haz the power, 2014-04-24 17:03:53]

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

[commit-bot: I haz the power, 2014-04-24 17:03:54]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[bartfab (slow), 2014-04-24 18:45:12]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:12:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/225403021/420001

[commit-bot: I haz the power, 2014-04-24 22:46:48]

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

[commit-bot: I haz the power, 2014-04-24 22:46:49]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[bartfab (slow), 2014-04-25 07:59:04]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-04-25 08:31:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/225403021/420001

[bartfab (slow), 2014-04-25 15:37:03]

Committed patchset #20 manually as r266197 (presubmit successful).