[GCM] Reland: Introduce GCMClientFactory to create GCMClient for GCMProfileService

[GCM] Reland: Introduce GCMClientFactory to create GCMClient for GCMProfileService

We used to call GCMClient::Get() to get the single instance. This did not
work when we need to pass some initialization data from browser to it.
GCMClientFactory is introduced such that GCMClient instance can be
created, initialized and used in GCMProfileService::IOWorker.

BUG=284553
TEST=existing tests

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

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

[fgorski, 2014-01-16 23:18:31]

lgtm with one comment.

https://codereview.chromium.org/135903005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/135903005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:240: gcm_client_ =
GCMClientFactory::GetClient();
will it work on the initialization list?
: service_(service),
  gcm_client_(GCMClientFactory::GetClient()) {
}

Or is that incorrect for some reason?

[Nicolas Zea, 2014-01-16 23:47:36]

https://codereview.chromium.org/135903005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_client_factory.cc (right):

https://codereview.chromium.org/135903005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_client_factory.cc:35: GCMClientImpl* client =
g_gcm_client.Pointer();
The GCM client expects to live on the IO thread. Does this enforce that?

https://codereview.chromium.org/135903005/diff/90001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/135903005/diff/90001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:130: virtual ~GCMClient() {}
explicit constructor?

[jianli, 2014-01-17 00:43:54]

https://codereview.chromium.org/135903005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_client_factory.cc (right):

https://codereview.chromium.org/135903005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_client_factory.cc:35: GCMClientImpl* client =
g_gcm_client.Pointer();
On 2014/01/16 23:47:36, Nicolas Zea wrote:
> The GCM client expects to live on the IO thread. Does this enforce that?

GCMClientFactory::GetClient is expected to be called from IO thread. Added a
DCHECK for it.

https://codereview.chromium.org/135903005/diff/90001/chrome/browser/services/...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/135903005/diff/90001/chrome/browser/services/...
chrome/browser/services/gcm/gcm_profile_service.cc:240: gcm_client_ =
GCMClientFactory::GetClient();
On 2014/01/16 23:18:31, Filip Gorski wrote:
> will it work on the initialization list?
> : service_(service),
>   gcm_client_(GCMClientFactory::GetClient()) {
> }
> 
> Or is that incorrect for some reason?

Yes, your suggestion works. However, I need to move setting gcm_client_ to
another method.

https://codereview.chromium.org/135903005/diff/90001/google_apis/gcm/gcm_clie...
File google_apis/gcm/gcm_client.h (right):

https://codereview.chromium.org/135903005/diff/90001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client.h:130: virtual ~GCMClient() {}
On 2014/01/16 23:47:36, Nicolas Zea wrote:
> explicit constructor?

Done.

[Nicolas Zea, 2014-01-17 01:09:23]

lgtm

[commit-bot: I haz the power, 2014-01-17 01:13:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/135903005/220001

[commit-bot: I haz the power, 2014-01-17 06:49:47]

Change committed as 245447

[fgorski, 2014-01-21 22:51:53]

lgtm

[Nicolas Zea, 2014-01-21 23:04:44]

https://codereview.chromium.org/135903005/diff/550001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/135903005/diff/550001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:515:
io_worker_->Initialize();
I think it's cleaner to have the GCMProfileService do:
PostTask(IO, ..., base::Bind(..IOWoker::Initialize, io_worker_))

and make replace Initialize's implementation with InitializeGCMClient. That way
the IOWorker truly is an IO worker, including the initialize code.

[jianli, 2014-01-21 23:16:16]

https://codereview.chromium.org/135903005/diff/550001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/135903005/diff/550001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:515:
io_worker_->Initialize();
On 2014/01/21 23:04:45, Nicolas Zea wrote:
> I think it's cleaner to have the GCMProfileService do:
> PostTask(IO, ..., base::Bind(..IOWoker::Initialize, io_worker_))
> 
> and make replace Initialize's implementation with InitializeGCMClient. That
way
> the IOWorker truly is an IO worker, including the initialize code.

Good idea. Also moved the logic defined in CheckGCMClientLoading to
IOWorker::Initialize and remove the following task posting.

[Nicolas Zea, 2014-01-21 23:30:30]

lgtm

https://codereview.chromium.org/135903005/diff/660001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/135903005/diff/660001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service.cc:503: // This initializes
GCMClient and also do the check to find out if GCMClient
nit: also do -> also does

[commit-bot: I haz the power, 2014-01-21 23:35:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/135903005/730001

[commit-bot: I haz the power, 2014-01-22 01:38:11]

Change committed as 246198