Create services/gcm directory and add some initial files

Create services/gcm directory and add some initial files

1) Create a gcm subdir under browser/services and add some initial files.
2) Add GCMProfileService that is profile based and acts a bridge between
GCM API and GCM client. This does not contain the real implementation yet.
3) Add GCMProfileServiceFactory that owns and creates all GCMProfileService
and associate them with profiles

BUG=314924
TEST=none due to no real implementation has been added yet
R=fgorski@chromium.org, zea@chromium.org

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

[fgorski, 2013-11-05 19:40:18]

lgtm with a question for your consideration.

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
File chrome/browser/services/gcm/gcm_profile_service.h (right):

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service.h:22: static bool
IsGCMEnabled();
What are the advantages/disadvantages of having that as a static on the service
class as opposed to having it on the service factory (perhaps as a static)?

[jianli, 2013-11-05 19:47:36]

Normally we do not put Is*Enabled() in the factory class.


On Tue, Nov 5, 2013 at 11:40 AM, <fgorski@chromium.org> wrote:

> lgtm with a question for your consideration.
>
>
> https://codereview.chromium.org/60673002/diff/30001/
> chrome/browser/services/gcm/gcm_profile_service.h
> File chrome/browser/services/gcm/gcm_profile_service.h (right):
>
> https://codereview.chromium.org/60673002/diff/30001/
> chrome/browser/services/gcm/gcm_profile_service.h#newcode22
> chrome/browser/services/gcm/gcm_profile_service.h:22: static bool
> IsGCMEnabled();
> What are the advantages/disadvantages of having that as a static on the
> service class as opposed to having it on the service factory (perhaps as
> a static)?
>
> https://codereview.chromium.org/60673002/
>

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

[Nicolas Zea, 2013-11-05 20:17:33]

lgtm with nits

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service.cc:11: bool
GCMProfileService::IsGCMEnabled() {
nit: keep definitions this in the order from the header file

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:7: #include
"chrome/browser/browser_process.h"
is this used?

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:9: #include
"chrome/browser/profiles/profile.h"
you never actually do anything with the profile, so you can probably leave it
forward declared

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:12: #include
"content/public/browser/browser_thread.h"
don't think you use this?

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:14: using
content::BrowserThread;
this too

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:19:
GCMProfileService* GCMProfileServiceFactory::GetForProfile(Profile* profile) {
same comment about having ordering consistent with header file

[jianli, 2013-11-05 20:42:31]

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
File chrome/browser/services/gcm/gcm_profile_service.cc (right):

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service.cc:11: bool
GCMProfileService::IsGCMEnabled() {
On 2013/11/05 20:17:33, Nicolas Zea wrote:
> nit: keep definitions this in the order from the header file

Done.

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:7: #include
"chrome/browser/browser_process.h"
On 2013/11/05 20:17:33, Nicolas Zea wrote:
> is this used?

Done.

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:9: #include
"chrome/browser/profiles/profile.h"
On 2013/11/05 20:17:33, Nicolas Zea wrote:
> you never actually do anything with the profile, so you can probably leave it
> forward declared

It is needed because we need to cast from BrowserContext to Profile in
BuildServiceInstanceFor.

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:12: #include
"content/public/browser/browser_thread.h"
On 2013/11/05 20:17:33, Nicolas Zea wrote:
> don't think you use this?

Done.

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:14: using
content::BrowserThread;
On 2013/11/05 20:17:33, Nicolas Zea wrote:
> this too

Done.

https://codereview.chromium.org/60673002/diff/30001/chrome/browser/services/g...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:19:
GCMProfileService* GCMProfileServiceFactory::GetForProfile(Profile* profile) {
On 2013/11/05 20:17:33, Nicolas Zea wrote:
> same comment about having ordering consistent with header file

For this file, indeed it is consistent with the ordering in header file.

[commit-bot: I haz the power, 2013-11-05 21:26:58]

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

[commit-bot: I haz the power, 2013-11-05 22:54:27]

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

[jianli, 2013-11-06 00:51:37]

Committed patchset #2 manually as r233163 (presubmit successful).