Componentize GCM Part 1: create GCM component and move some files over

Componentize GCM Part 1: create GCM component and move some files over

BUG=356716
TEST=existing tests

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

R=blundell@chromium.org, fgorski@chromium.org, jhawkins@chromium.org, kalman@chromium.org, pavely@chromium.org, zea@chromium.org

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

[blundell, 2014-05-07 09:19:02]

Looks good overall, thanks!

https://codereview.chromium.org/261853012/diff/40001/components/gcm.gypi
File components/gcm.gypi (right):

https://codereview.chromium.org/261853012/diff/40001/components/gcm.gypi#newc...
components/gcm.gypi:13: '../net/net.gyp:net',
This should depend on os_crypt.

https://codereview.chromium.org/261853012/diff/40001/components/gcm.gypi#newc...
components/gcm.gypi:41: 'sources': [
I don't think it works to have a static_library with no code in it.

https://codereview.chromium.org/261853012/diff/40001/components/gcm/DEPS
File components/gcm/DEPS (right):

https://codereview.chromium.org/261853012/diff/40001/components/gcm/DEPS#newc...
components/gcm/DEPS:2: "-content/public/common",
You can remove this...I landed a CL yesterday changing the top-level
//components DEPS file to not have an allowance for content/public/common.

https://codereview.chromium.org/261853012/diff/40001/components/gcm/OWNERS
File components/gcm/OWNERS (left):

https://codereview.chromium.org/261853012/diff/40001/components/gcm/OWNERS#ol...
components/gcm/OWNERS:1: dimich@chromium.org
There should be per-file OWNERS for gcm.* in //components/OWNERS.

https://codereview.chromium.org/261853012/diff/40001/google_apis/gcm/base/enc...
File google_apis/gcm/base/encryptor.h (left):

https://codereview.chromium.org/261853012/diff/40001/google_apis/gcm/base/enc...
google_apis/gcm/base/encryptor.h:5: #ifndef SYNC_UTIL_ENCRYPTOR_H_
Hmm, do we have two copies of this header in the codebase now? That seems
undesirable. I'm not close enough to this code to know what the issues here are
in depth...

[fgorski, 2014-05-07 18:27:54]

LGTM,

Nice idea to handle the Encryptor dependency. Is there a way to inject it
through Internals builder?

https://codereview.chromium.org/261853012/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/fake_gcm_client_factory.cc (right):

https://codereview.chromium.org/261853012/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/fake_gcm_client_factory.cc:23: return
scoped_ptr<GCMClient>(new GCMClientMock(
nit: did you split this line on purpose?

[jianli, 2014-05-07 19:12:29]

https://codereview.chromium.org/261853012/diff/40001/components/gcm.gypi
File components/gcm.gypi (right):

https://codereview.chromium.org/261853012/diff/40001/components/gcm.gypi#newc...
components/gcm.gypi:13: '../net/net.gyp:net',
On 2014/05/07 09:19:02, blundell wrote:
> This should depend on os_crypt.

Done.

https://codereview.chromium.org/261853012/diff/40001/components/gcm.gypi#newc...
components/gcm.gypi:41: 'sources': [
On 2014/05/07 09:19:02, blundell wrote:
> I don't think it works to have a static_library with no code in it.

Removed.

https://codereview.chromium.org/261853012/diff/40001/components/gcm/DEPS
File components/gcm/DEPS (right):

https://codereview.chromium.org/261853012/diff/40001/components/gcm/DEPS#newc...
components/gcm/DEPS:2: "-content/public/common",
On 2014/05/07 09:19:02, blundell wrote:
> You can remove this...I landed a CL yesterday changing the top-level
> //components DEPS file to not have an allowance for content/public/common.

Done.

https://codereview.chromium.org/261853012/diff/40001/components/gcm/OWNERS
File components/gcm/OWNERS (left):

https://codereview.chromium.org/261853012/diff/40001/components/gcm/OWNERS#ol...
components/gcm/OWNERS:1: dimich@chromium.org
On 2014/05/07 09:19:02, blundell wrote:
> There should be per-file OWNERS for gcm.* in //components/OWNERS.

Done.

https://codereview.chromium.org/261853012/diff/40001/google_apis/gcm/base/enc...
File google_apis/gcm/base/encryptor.h (left):

https://codereview.chromium.org/261853012/diff/40001/google_apis/gcm/base/enc...
google_apis/gcm/base/encryptor.h:5: #ifndef SYNC_UTIL_ENCRYPTOR_H_
On 2014/05/07 09:19:02, blundell wrote:
> Hmm, do we have two copies of this header in the codebase now? That seems
> undesirable. I'm not close enough to this code to know what the issues here
are
> in depth...

Yes. Since both sync and gcm needs some os_crypt functionality. I don't want to
introduce dependency between gcm and sync. We should try to either figure out
how to move os_crypt out of components or find a place to place some common
files.

https://codereview.chromium.org/261853012/diff/60001/chrome/browser/services/...
File chrome/browser/services/gcm/fake_gcm_client_factory.cc (right):

https://codereview.chromium.org/261853012/diff/60001/chrome/browser/services/...
chrome/browser/services/gcm/fake_gcm_client_factory.cc:23: return
scoped_ptr<GCMClient>(new GCMClientMock(
On 2014/05/07 18:27:55, fgorski wrote:
> nit: did you split this line on purpose?

Reverted. This is a leftover from undoing previous change.

[Nicolas Zea, 2014-05-07 20:06:59]

mostly LG

https://codereview.chromium.org/261853012/diff/90001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/261853012/diff/90001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:39: '../components/components.gyp:gcm',
shouldn't we be calling this gcm_driver?

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

https://codereview.chromium.org/261853012/diff/90001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.h:78:
GCMClientImpl(scoped_ptr<GCMInternalsBuilder> internals_builder);
this should still be explicit

[jianli, 2014-05-07 20:12:15]

https://codereview.chromium.org/261853012/diff/90001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/261853012/diff/90001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:39: '../components/components.gyp:gcm',
On 2014/05/07 20:07:00, Nicolas Zea wrote:
> shouldn't we be calling this gcm_driver?

I prefer to simplify it as gcm in order to be consistent gcm directory name used
in other places. Also, it will allow us to rename/refactor files without
updating the directory name in the future.

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

https://codereview.chromium.org/261853012/diff/90001/google_apis/gcm/gcm_clie...
google_apis/gcm/gcm_client_impl.h:78:
GCMClientImpl(scoped_ptr<GCMInternalsBuilder> internals_builder);
On 2014/05/07 20:07:00, Nicolas Zea wrote:
> this should still be explicit

Done.

[jianli, 2014-05-08 21:06:11]

zea and blundell, could you please take another look? thanks.

[Nicolas Zea, 2014-05-08 21:21:49]

https://codereview.chromium.org/261853012/diff/170001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/261853012/diff/170001/components/components.g...
components/components.gyp:25: 'gcm.gypi',
This should probably be within the android_webview_build == 0 case below.

For that matter, do we even want to build it on android at all?

[jianli, 2014-05-08 23:08:00]

https://codereview.chromium.org/261853012/diff/170001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/261853012/diff/170001/components/components.g...
components/components.gyp:25: 'gcm.gypi',
On 2014/05/08 21:21:49, Nicolas Zea wrote:
> This should probably be within the android_webview_build == 0 case below.
> 
> For that matter, do we even want to build it on android at all?

Done.

[tfarina, 2014-05-09 03:49:13]

https://codereview.chromium.org/261853012/diff/190001/components/gcm.gypi
File components/gcm.gypi (right):

https://codereview.chromium.org/261853012/diff/190001/components/gcm.gypi#new...
components/gcm.gypi:11: 'components.gyp:os_crypt',
move this below net, also, just os_crypt, no need of components.gyp

[blundell, 2014-05-09 08:23:35]

LGTM

I'm OK with gcm instead of gcm_driver as the component name. (Aside: I think we
could consider renaming //components/sync_driver to //components/sync).

https://codereview.chromium.org/261853012/diff/190001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/261853012/diff/190001/components/components.g...
components/components.gyp:71: # it once we remove the depdency on
google_apis/gcm for Android.
Should there be a TODO and bug reference here?

[Nicolas Zea, 2014-05-09 17:55:13]

lgtm

https://codereview.chromium.org/261853012/diff/190001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/261853012/diff/190001/components/components.g...
components/components.gyp:71: # it once we remove the depdency on
google_apis/gcm for Android.
nit: depdency -> dependency

[jianli, 2014-05-09 21:29:37]

https://codereview.chromium.org/261853012/diff/190001/components/components.gyp
File components/components.gyp (right):

https://codereview.chromium.org/261853012/diff/190001/components/components.g...
components/components.gyp:71: # it once we remove the depdency on
google_apis/gcm for Android.
On 2014/05/09 17:55:14, Nicolas Zea wrote:
> nit: depdency -> dependency

Done.

https://codereview.chromium.org/261853012/diff/190001/components/components.g...
components/components.gyp:71: # it once we remove the depdency on
google_apis/gcm for Android.
On 2014/05/09 08:23:35, blundell wrote:
> Should there be a TODO and bug reference here?

This is part of the GCM componentization effort. Same bug is used to track this
issue.

https://codereview.chromium.org/261853012/diff/190001/components/gcm.gypi
File components/gcm.gypi (right):

https://codereview.chromium.org/261853012/diff/190001/components/gcm.gypi#new...
components/gcm.gypi:11: 'components.gyp:os_crypt',
On 2014/05/09 03:49:13, tfarina wrote:
> move this below net, also, just os_crypt, no need of components.gyp

Done.

[jianli, 2014-05-09 21:33:38]

yoz, could you please review and approve changes to chrome/browser/extensions?

pavely, could you please review and approve changes to
chrome/browser/invalidations?

[jianli, 2014-05-12 18:35:00]

Kalman, could you please approve chrome/browser/extension changes? Thanks.

[not at google - send to devlin, 2014-05-12 18:36:31]

lgtm but just tbr include changes

[pavely, 2014-05-12 18:43:07]

LGTM c/b/invalidation

[jianli, 2014-05-12 18:56:29]

jhawkins, could you please approve chrome/browser/DEPS? thanks.

[blundell, 2014-05-12 19:07:18]

You can TBR jam or darin for the DEPS change.

[jianli, 2014-05-12 19:11:31]

Ok. Thanks.


On Mon, May 12, 2014 at 12:07 PM, <blundell@chromium.org> wrote:

> You can TBR jam or darin for the DEPS change.
>
> https://codereview.chromium.org/261853012/
>

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

[James Hawkins, 2014-05-12 19:35:35]

deps lgtm

[jianli, 2014-05-13 19:25:59]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-13 19:27:35]

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

[commit-bot: I haz the power, 2014-05-13 22:25:27]

Change committed as 270226

[jianli, 2014-05-14 00:55:52]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-14 00:56:29]

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

[commit-bot: I haz the power, 2014-05-14 05:51:54]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[commit-bot: I haz the power, 2014-05-14 05:55:46]

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

[commit-bot: I haz the power, 2014-05-14 05:55:46]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[jianli, 2014-05-14 07:21:41]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2014-05-14 07:22:18]

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

[commit-bot: I haz the power, 2014-05-14 07:26:59]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[commit-bot: I haz the power, 2014-05-14 07:31:01]

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

[commit-bot: I haz the power, 2014-05-14 07:31:02]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[jianli, 2014-05-14 20:48:09]

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