Add browser-global GCMDriver

Add browser-global GCMDriver

This CL adds a browser-global GCMDriver instance. This GCM connection can
be used even if no session is in progress and no user is signed in.

Since GCMDriverDesktop was originally meant to work for signed-in users
only, it still depends on an IdentityProvider. The CL introduces a
temporary DummyIdentityProvider to satisfy the dependency. This dummy
class will be deleted when GCMDriver's dependency on IdentityProvider is
removed.

BUG=376746
TEST=Covered by GCMDriverTest.* unit tests

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

[bartfab (slow), 2014-05-30 12:46:51]

This is an alternative to CL 296953006 that makes a browser-global GCMDriver
available on all platforms since there are consumers interested in it outside of
Chrome OS, as pointed out by Jian Li.

Jochen, could you take a look at:
chrome/browser/browser_process*
chrome/common/chrome_paths.[cc|h]

Hi Roger, could you take a look at:
google_apis/*

Jian, FYI

[Roger Tawa OOO till Jul 10th, 2014-05-30 15:07:20]

google_apis lgtm

[johnme, 2014-05-30 15:30:42]

Ah, interesting - in http://crrev.com/278493002 (currently on the CQ) I'm
splitting GCMDriver into GCMDriverDesktop and GCMDriverAndroid. Once that lands,
BrowserProcessImpl::CreateGCMDriver would need to have an ifdef and construct
the GCMDriver differently depending on whether it's on Android. At the time,
GCMDriver was only ever constructed by GCMProfileService, but if you're
introducing a new place that constructs GCMDriver, perhaps we should consider
having a GCMDriverFactory to hide the platform-specific details (and other
duplicated code, like getting the blocking_task_runner and message loop
proxies)?

[bartfab (slow), 2014-05-30 15:37:19]

On 2014/05/30 15:30:42, johnme wrote:
> Ah, interesting - in http://crrev.com/278493002 (currently on the CQ) I'm
> splitting GCMDriver into GCMDriverDesktop and GCMDriverAndroid. Once that
lands,
> BrowserProcessImpl::CreateGCMDriver would need to have an ifdef and construct
> the GCMDriver differently depending on whether it's on Android. At the time,
> GCMDriver was only ever constructed by GCMProfileService, but if you're
> introducing a new place that constructs GCMDriver, perhaps we should consider
> having a GCMDriverFactory to hide the platform-specific details (and other
> duplicated code, like getting the blocking_task_runner and message loop
> proxies)?

Thanks for commenting here to make me aware of your work. One of the reasons for
splitting off GCMDriver in the first place was to create a global,
Profile-indenpendent instance. You are right that since the per-Profile and
global instances share a lot of setup code, having a builder/factory method
encapsulate most of the setup is a good idea. Plus, it could be used to hide the
differences between Android and other platforms as you also suggest. Do you want
to introduce this method or should I? I think it would be best to land it as a
separate CL so that your CL can land in peace right now, the factory method can
be added next and then my CL can be landed.

[jianli, 2014-06-02 22:34:40]

https://codereview.chromium.org/301973009/diff/1/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/301973009/diff/1/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:1026: gcm_driver_.reset(new
gcm::GCMDriver(
You need to sync to pick up johnme's change. I don't think we need to create
GCMDriver global instance for Android.

It seems that the creation logic here is very similar to the one used in
GCMProfileService constructor. Could you please create a helper function to
share the code? Perhaps you could define something like CreateGCMDriver in
gcm_utils.h:
  scoped_ptr<GCMDriver> CreateGCMDriverDesktop(
    scoped_ptr<GCMClientFactory> gcm_client_factory,
    scoped_ptr<IdentityProvider> identity_provider,
    const base::FilePath& store_path,
    const scoped_refptr<net::URLRequestContextGetter>& request_context);

https://codereview.chromium.org/301973009/diff/1/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/301973009/diff/1/chrome/common/chrome_paths.c...
chrome/common/chrome_paths.cc:583: cur = cur.Append("gcm_store");
Can we use kGCMStoreDirname that is defined in chrome_constants?

[bartfab (slow), 2014-06-03 13:36:49]

https://codereview.chromium.org/301973009/diff/1/chrome/browser/browser_proce...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/301973009/diff/1/chrome/browser/browser_proce...
chrome/browser/browser_process_impl.cc:1026: gcm_driver_.reset(new
gcm::GCMDriver(
On 2014/06/02 22:34:40, jianli wrote:
> You need to sync to pick up johnme's change. I don't think we need to create
> GCMDriver global instance for Android.
> 
> It seems that the creation logic here is very similar to the one used in
> GCMProfileService constructor. Could you please create a helper function to
> share the code? Perhaps you could define something like CreateGCMDriver in
> gcm_utils.h:
>   scoped_ptr<GCMDriver> CreateGCMDriverDesktop(
>     scoped_ptr<GCMClientFactory> gcm_client_factory,
>     scoped_ptr<IdentityProvider> identity_provider,
>     const base::FilePath& store_path,
>     const scoped_refptr<net::URLRequestContextGetter>& request_context);

Done. For consistency, I made the code also create a global GCMDriver instance
on Android. I feel that if we add gcm_driver() to the cross-platform
BrowserProcessImpl class, it should return something valid on every platform,
including Android. If nobody ever requests a GCMDriver on Android, none will be
created, so no resources will be wasted needlessly.

https://codereview.chromium.org/301973009/diff/1/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (right):

https://codereview.chromium.org/301973009/diff/1/chrome/common/chrome_paths.c...
chrome/common/chrome_paths.cc:583: cur = cur.Append("gcm_store");
On 2014/06/02 22:34:40, jianli wrote:
> Can we use kGCMStoreDirname that is defined in chrome_constants?

Done.

[bartfab (slow), 2014-06-03 13:40:11]

Hi Jochen,

Friendly review ping.

[jianli, 2014-06-03 17:18:42]

https://codereview.chromium.org/301973009/diff/20001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/301973009/diff/20001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:1023: base::FilePath store_path;
One more thought to simplify the code here. We could consider adding Profile*
argument to CreateGCMDriverDesktop helper function that will figure out the
right path and IdentityProvider based on whether profile is null or not.

https://codereview.chromium.org/301973009/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/301973009/diff/20001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:3227: 'browser/services/gcm/gcm_utils.cc',
Excluding these 2 files from Android build seems to be confusing since gcm_utils
could contain other helper functions in the future. We should either wrap
CreateGCMDriverDesktop under !OS_ANDROID or rename gcm_utils to
gcm_desktop_utils.

[bartfab (slow), 2014-06-04 11:00:48]

https://codereview.chromium.org/301973009/diff/20001/chrome/browser/browser_p...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/301973009/diff/20001/chrome/browser/browser_p...
chrome/browser/browser_process_impl.cc:1023: base::FilePath store_path;
On 2014/06/03 17:18:42, jianli wrote:
> One more thought to simplify the code here. We could consider adding Profile*
> argument to CreateGCMDriverDesktop helper function that will figure out the
> right path and IdentityProvider based on whether profile is null or not.

I think it would be be better to not introduce a |Profile| argument if
CreateGCMDriverDesktop() can work without a |Profile| as well. Also, the
identity provider requirement is going away soon anyway, so all of the code that
deals with it will be removed soon anyway.

https://codereview.chromium.org/301973009/diff/20001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/301973009/diff/20001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:3227: 'browser/services/gcm/gcm_utils.cc',
On 2014/06/03 17:18:42, jianli wrote:
> Excluding these 2 files from Android build seems to be confusing since
gcm_utils
> could contain other helper functions in the future. We should either wrap
> CreateGCMDriverDesktop under !OS_ANDROID or rename gcm_utils to
> gcm_desktop_utils.

Renamed to gcm_desktop_utils. If a utility function is added in the future that
would be useful on Android, we can rename the file to gcm_utils again.

[jianli, 2014-06-05 05:57:00]

lgtm

[jochen (gone - plz use gerrit), 2014-06-05 06:53:04]

can you clarify which test covers this?

[bartfab (slow), 2014-06-05 08:33:02]

On 2014/06/05 06:53:04, jochen wrote:
> can you clarify which test covers this?

I clarified it in the CL description: The GCMDriverTest.* unit tests cover the
entire GCMDriverDesktop class. This CL just creates another instance of the same
class.

[bartfab (slow), 2014-06-05 08:33:03]

On 2014/06/05 06:53:04, jochen wrote:
> can you clarify which test covers this?

I clarified it in the CL description: The GCMDriverTest.* unit tests cover the
entire GCMDriverDesktop class. This CL just creates another instance of the same
class.

[bartfab (slow), 2014-06-05 08:33:04]

On 2014/06/05 06:53:04, jochen wrote:
> can you clarify which test covers this?

I clarified it in the CL description: The GCMDriverTest.* unit tests cover the
entire GCMDriverDesktop class. This CL just creates another instance of the same
class.

[jochen (gone - plz use gerrit), 2014-06-05 08:52:06]

lgtm

[bartfab (slow), 2014-06-05 09:02:09]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-06-05 09:04:24]

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

[bartfab (slow), 2014-06-05 09:27:59]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-06-05 09:29:16]

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

[bartfab (slow), 2014-06-05 09:58:25]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-06-05 09:59:26]

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

[commit-bot: I haz the power, 2014-06-05 13:31:30]

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

[commit-bot: I haz the power, 2014-06-05 15:25:45]

Change committed as 275134