Componentizing GcmProfileService.

Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
TBR=rockot,stevenjb,rkc,fgorski,dcheng,skuhne,isherman,bauerb

Committed: https://crrev.com/7554194aec4024a5409d2de575d8b59e4d9679c2
Cr-Commit-Position: refs/heads/master@{#357609}

[Jitu( very slow this week), 2015-10-27 11:31:18]

jitendra.ks@samsung.com changed reviewers:
+ droger@chromium.org

[Jitu( very slow this week), 2015-10-27 11:31:18]

PTAL ...

It failed in some build bots for UT.
Can you please help me figure out the issue here.

Backtrace:
	base::ObserverListBase<ui_zoom::ZoomObserver>::AddObserver [0x0148FAA8+88]
	ProfileIdentityProvider::ProfileIdentityProvider [0x03ED20E1+65]
	gcm::GCMProfileService::IdentityObserver::IdentityObserver [0x03E730AD+93]
	gcm::GCMProfileService::SetDriverForTesting [0x03E733C9+73]
	gcm::FakeGCMProfileService::Build [0x00FAB9DA+90]
	KeyedServiceFactory::GetServiceForContext [0x03B998CC+332]
	gcm::GCMProfileServiceFactory::GetForProfile [0x013AF2B8+40]
	extensions::GcmApiTest::SetUpOnMainThread [0x00D322BA+74]
	InProcessBrowserTest::RunTestOnMainThreadLoop [0x015AF85A+234]

[Jitu( very slow this week), 2015-10-27 12:08:17]

Description was changed from

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
==========

to

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
==========

[droger, 2015-10-27 12:23:44]

Looks good.

I see the failures, it seems it is crashing because signin_manager_ is null.
I understand why it would be the case:
BuildFakeGCMProfileService uses the default constructor for GCMProfileService,
which uses a null signin manager.

I don't understand why it was working before though. It seems to me that the
existing code would use a null profile, and should crash the same way.

Maybe for some reason SigninManagerFactory::GetForProfile(nullptr) was returning
something non-null and was preventing the crash? If so, that means the bug was
definitely already there.

If this is the explanation, then one way to fix it would be to pass a
FakeSigninManagerBase somehow. Maybe by changing the empty constructor and add a
SigninManagerBase parameter to it.

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.cc (right):

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:168: : signin_manager_(NULL),
use nullptr instead of NULL.

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:170: request_context_() {}
request_context_(nullptr)

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.h:15: #include
"components/signin/core/browser/profile_oauth2_token_service.h"
Can we forward declare
ProfileOAuth2TokenService
instead of including this?

Same for:
SigninManagerBase
URLRequestContextGetter

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.h:21: class Profile;
Remove Profile, here and in comments below (lines 36 and 39).

[droger, 2015-10-27 12:36:43]

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.cc (left):

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:201: if (identity_observer_)
As pointed out by blundell: your removed that if.
This probably explains the failures. Putting the if back should fix.

[Jitu( very slow this week), 2015-10-27 16:23:31]

Patchset #3 (id:40001) has been deleted

[Jitu( very slow this week), 2015-10-28 05:11:36]

PTAL ...

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.cc (left):

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:201: if (identity_observer_)
On 2015/10/27 12:36:43, droger wrote:
> As pointed out by blundell: your removed that if.
> This probably explains the failures. Putting the if back should fix.

Oh
This was removed mistakenly.

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.cc (right):

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:168: : signin_manager_(NULL),
On 2015/10/27 12:23:44, droger wrote:
> use nullptr instead of NULL.

Done.

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:168: : signin_manager_(NULL),
On 2015/10/27 12:23:44, droger wrote:
> use nullptr instead of NULL.

Done.

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:170: request_context_() {}
On 2015/10/27 12:23:44, droger wrote:
> request_context_(nullptr)

Done.

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.h:15: #include
"components/signin/core/browser/profile_oauth2_token_service.h"
On 2015/10/27 12:23:44, droger wrote:
> Can we forward declare
> ProfileOAuth2TokenService
> instead of including this?
> 
> Same for:
> SigninManagerBase
> URLRequestContextGetter

Done.

https://codereview.chromium.org/1425783002/diff/20001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.h:21: class Profile;
On 2015/10/27 12:23:44, droger wrote:
> Remove Profile, here and in comments below (lines 36 and 39).

Done.

[droger, 2015-10-28 08:30:58]

lgtm

https://codereview.chromium.org/1425783002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.cc (right):

https://codereview.chromium.org/1425783002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:188: if (identity_observer_)
Use curly braces {} fot the if
(the style is to use them if the body takes multiple lines).

https://codereview.chromium.org/1425783002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.h:10: #include "base/callback.h"
base/callback_forward.h
may be enough

[droger, 2015-10-28 08:31:19]

droger@chromium.org changed reviewers:
+ zea@chromium.org

[droger, 2015-10-28 08:31:19]

+zea as owner of GCM

[Jitu( very slow this week), 2015-10-28 13:35:14]

Fixed the comments...
PTAL

https://codereview.chromium.org/1425783002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.cc (right):

https://codereview.chromium.org/1425783002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:188: if (identity_observer_)
On 2015/10/28 08:30:57, droger wrote:
> Use curly braces {} fot the if
> (the style is to use them if the body takes multiple lines).

Done.

https://codereview.chromium.org/1425783002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.h:10: #include "base/callback.h"
On 2015/10/28 08:30:57, droger wrote:
> base/callback_forward.h
> may be enough

Done.

[johnme, 2015-10-28 14:34:56]

johnme@chromium.org changed reviewers:
+ johnme@chromium.org

[johnme, 2015-10-28 14:34:57]

Drive-by review: generally looks good, left two comments where things could
perhaps be simpler.

https://codereview.chromium.org/1425783002/diff/80001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/80001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:60:
base::SequencedWorkerPool* worker_pool =
It seems a shame to duplicate this code with
extension_gcm_app_handler_unittest.cc and gcm_profile_service_unittest.cc.

Perhaps you could make a private method:

GCMProfileServiceFactory::BuildGCMProfileService(
    content::BrowserContext* context,
    scoped_ptr<GCMClientFactory> client_factory)

and have extension_gcm_app_handler_unittest.cc and
gcm_profile_service_unittest.cc declared as friend classes/methods of
GCMProfileServiceFactory so they can call it?

https://codereview.chromium.org/1425783002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.cc (right):

https://codereview.chromium.org/1425783002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:141:
GCMProfileService::GCMProfileService(
It seems this constructor could be simplified to:

GCMProfileService::GCMProfileService(
    scoped_ptr<GCMDriver> driver,
    scoped_ptr<ProfileIdentityProvider> identity_provider,
    net::URLRequestContextGetter* request_context)
    : driver_(driver),
      identity_provider_(identity_provider),
      request_context_(request_context) {
  identity_observer_.reset(new IdentityObserver(
      identity_provider_.get(), request_context_, driver_.get());
}

if you change the IdentityObserver constructor signature to:

IdentityObserver::IdentityObserver(ProfileIdentityProvider* identity_provider,
net::URLRequestContextGetter* request_context, GCMDriver* driver)

Or perhaps, to avoid duplicating all the pointers, you could even change that
constructor to just:

IdentityObserver::IdentityObserver(GCMProfileService* service);

Similarly for OS_ANDROID, except you'd just need:

GCMProfileService::GCMProfileService(scoped_ptr<GCMDriver> driver)
    : driver_(driver) {}

[droger, 2015-10-28 14:41:08]

https://codereview.chromium.org/1425783002/diff/80001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/80001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:60:
base::SequencedWorkerPool* worker_pool =
On 2015/10/28 14:34:56, johnme wrote:
> It seems a shame to duplicate this code with
> extension_gcm_app_handler_unittest.cc and gcm_profile_service_unittest.cc.
> 
> Perhaps you could make a private method:
> 
> GCMProfileServiceFactory::BuildGCMProfileService(
>     content::BrowserContext* context,
>     scoped_ptr<GCMClientFactory> client_factory)
> 
> and have extension_gcm_app_handler_unittest.cc and
> gcm_profile_service_unittest.cc declared as friend classes/methods of
> GCMProfileServiceFactory so they can call it?

gcm_profile_service_unittest should be componentized too, and thus should not
use BrowserContext or GCMProfileServiceFactory.

extension_gcm_app_handler_unittest.cc may be fine though.

[Jitu( very slow this week), 2015-10-29 14:11:25]

PTAL

https://codereview.chromium.org/1425783002/diff/80001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/80001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:60:
base::SequencedWorkerPool* worker_pool =
On 2015/10/28 14:41:08, droger wrote:
> On 2015/10/28 14:34:56, johnme wrote:
> > It seems a shame to duplicate this code with
> > extension_gcm_app_handler_unittest.cc and gcm_profile_service_unittest.cc.
> > 
> > Perhaps you could make a private method:
> > 
> > GCMProfileServiceFactory::BuildGCMProfileService(
> >     content::BrowserContext* context,
> >     scoped_ptr<GCMClientFactory> client_factory)
> > 
> > and have extension_gcm_app_handler_unittest.cc and
> > gcm_profile_service_unittest.cc declared as friend classes/methods of
> > GCMProfileServiceFactory so they can call it?
> 
> gcm_profile_service_unittest should be componentized too, and thus should not
> use BrowserContext or GCMProfileServiceFactory.
> 
> extension_gcm_app_handler_unittest.cc may be fine though.

I am planning to take componentizing of gcm_profile_service_unittest in next
subsequent patch.
WDYT?

https://codereview.chromium.org/1425783002/diff/80001/components/gcm_driver/g...
File components/gcm_driver/gcm_profile_service.cc (right):

https://codereview.chromium.org/1425783002/diff/80001/components/gcm_driver/g...
components/gcm_driver/gcm_profile_service.cc:141:
GCMProfileService::GCMProfileService(
On 2015/10/28 14:34:56, johnme wrote:
> It seems this constructor could be simplified to:
> 
> GCMProfileService::GCMProfileService(
>     scoped_ptr<GCMDriver> driver,
>     scoped_ptr<ProfileIdentityProvider> identity_provider,
>     net::URLRequestContextGetter* request_context)
>     : driver_(driver),
>       identity_provider_(identity_provider),
>       request_context_(request_context) {
>   identity_observer_.reset(new IdentityObserver(
>       identity_provider_.get(), request_context_, driver_.get());
> }
> 
> if you change the IdentityObserver constructor signature to:
> 
> IdentityObserver::IdentityObserver(ProfileIdentityProvider* identity_provider,
> net::URLRequestContextGetter* request_context, GCMDriver* driver)
> 
> Or perhaps, to avoid duplicating all the pointers, you could even change that
> constructor to just:
> 
> IdentityObserver::IdentityObserver(GCMProfileService* service);
> 
> Similarly for OS_ANDROID, except you'd just need:
> 
> GCMProfileService::GCMProfileService(scoped_ptr<GCMDriver> driver)
>     : driver_(driver) {}

Done.

[droger, 2015-10-29 14:18:06]

Still LGTM

https://codereview.chromium.org/1425783002/diff/80001/chrome/browser/services...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/80001/chrome/browser/services...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:60:
base::SequencedWorkerPool* worker_pool =
On 2015/10/29 14:11:25, jitu (slow this week) wrote:
> On 2015/10/28 14:41:08, droger wrote:
> > On 2015/10/28 14:34:56, johnme wrote:
> > > It seems a shame to duplicate this code with
> > > extension_gcm_app_handler_unittest.cc and gcm_profile_service_unittest.cc.
> > > 
> > > Perhaps you could make a private method:
> > > 
> > > GCMProfileServiceFactory::BuildGCMProfileService(
> > >     content::BrowserContext* context,
> > >     scoped_ptr<GCMClientFactory> client_factory)
> > > 
> > > and have extension_gcm_app_handler_unittest.cc and
> > > gcm_profile_service_unittest.cc declared as friend classes/methods of
> > > GCMProfileServiceFactory so they can call it?
> > 
> > gcm_profile_service_unittest should be componentized too, and thus should
not
> > use BrowserContext or GCMProfileServiceFactory.
> > 
> > extension_gcm_app_handler_unittest.cc may be fine though.
> 
> I am planning to take componentizing of gcm_profile_service_unittest in next
> subsequent patch.
> WDYT?

That's fine for me.

[Nicolas Zea, 2015-10-29 17:27:12]

gcm LGTM, but please consider splitting up GCMProfileService into an Android
implementation and a non-Android one as a follow up.

https://codereview.chromium.org/1425783002/diff/100001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:51: #if defined(OS_ANDROID)
I wonder if it would be better to have a separate GCMProfileServiceAndroid from
GCMProfileServiceCommon or something like that. That doesn't need to happen in
this patch, but seems a cleaner abstraction. Maybe file a followup bug for that?

Having all this #ifdef'd code makes it difficult to follow what is actually
happening and what isn't.

https://codereview.chromium.org/1425783002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:53: base::FilePath path,
nit: pass by const ref? (in the non-android version too)

[johnme, 2015-10-29 17:49:40]

lgtm, thanks

[Jitu( very slow this week), 2015-10-30 04:31:30]

jitendra.ks@samsung.com changed reviewers:
+ rockot@chromium.org

[Jitu( very slow this week), 2015-10-30 04:31:31]

@Rockot: Please do an OWNERS review of
         -- chrome/browser/extensions/*

@Sky: Please do an OWNER review of
      -- chrome/browser/ui/webui/*

https://codereview.chromium.org/1425783002/diff/100001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:53: base::FilePath path,
On 2015/10/29 17:27:12, Nicolas Zea wrote:
> nit: pass by const ref? (in the non-android version too)

Done.

[Jitu( very slow this week), 2015-10-30 04:32:12]

jitendra.ks@samsung.com changed reviewers:
+ sky@chromium.org

[blundell, 2015-10-30 08:56:48]

blundell@chromium.org changed reviewers:
+ blundell@chromium.org

[blundell, 2015-10-30 08:56:49]

lgtm

I looked over the //chrome/browser/extensions and //chrome/browser/ui/webui
changes, and they're acceptable to TBR to each of their respective owners, as
they're changes in API callsites where the API change was LGTM'd by the owner of
the API.

https://codereview.chromium.org/1425783002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/1425783002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:214:
content::BrowserThread::GetMessageLoopProxyForThread(
nit: pull ui_thread and io_thread into local variables to eliminate the code
duplication.

[Jitu( very slow this week), 2015-10-30 10:13:51]

Description was changed from

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
==========

to

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
TBR=rockot,sky
==========

[Jitu( very slow this week), 2015-10-30 10:14:34]

PTAL ...

I have added other OWNERS for TBR.

https://codereview.chromium.org/1425783002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/1425783002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:214:
content::BrowserThread::GetMessageLoopProxyForThread(
On 2015/10/30 08:56:49, blundell wrote:
> nit: pull ui_thread and io_thread into local variables to eliminate the code
> duplication.

Done.

[blundell, 2015-10-30 10:15:58]

lgtm

Make sure to send an explicit message about who you're TBR'ing for what (e.g.,
"TBR=sky@ for //chrome/browser/ui/webui")

[Jitu( very slow this week), 2015-10-30 11:26:35]

Description was changed from

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
TBR=rockot,sky
==========

to

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
TBR=rockot,stevenjb,rkc,fgorski,dcheng,skuhne,isherman,bauerb
==========

[Jitu( very slow this week), 2015-10-30 11:26:35]

Description was changed from

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
TBR=rockot,sky
==========

to

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
TBR=rockot,stevenjb,rkc,fgorski,dcheng,skuhne,isherman,bauerb
==========

[Jitu( very slow this week), 2015-10-30 11:26:35]

Description was changed from

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
TBR=rockot,sky
==========

to

==========
Componentizing GcmProfileService.

This class is shared by shared by the iOS port, and thus needs to decouple
from //chrome dependencies for clean integration on iOS.

This CL:
-- Pass PrefService, FilePath, net::URLRequestContextGetter,SigninManagerBase,
   ProfileOAuth2TokenService, base::Closure etc instead of profile.
-- Added a new file for constants.

BUG=519585
TBR=rockot,stevenjb,rkc,fgorski,dcheng,skuhne,isherman,bauerb
==========

[Jitu( very slow this week), 2015-10-30 11:26:35]

jitendra.ks@samsung.com changed reviewers:
+ bauerb@chromium.org, dcheng@chromium.org, fgorski@chromium.org,
isherman@chromium.org, rkc@chromium.org, skuhne@chromium.org,
stevenjb@chromium.org
- sky@chromium.org

[Jitu( very slow this week), 2015-10-30 11:26:35]

jitendra.ks@samsung.com changed reviewers:
+ bauerb@chromium.org, dcheng@chromium.org, fgorski@chromium.org,
isherman@chromium.org, rkc@chromium.org, skuhne@chromium.org,
stevenjb@chromium.org
- sky@chromium.org

[Jitu( very slow this week), 2015-10-30 11:26:35]

jitendra.ks@samsung.com changed reviewers:
+ bauerb@chromium.org, dcheng@chromium.org, fgorski@chromium.org,
isherman@chromium.org, rkc@chromium.org, skuhne@chromium.org,
stevenjb@chromium.org
- sky@chromium.org

[Jitu( very slow this week), 2015-10-30 11:30:20]

Remaining changes are mechanical
TBR'ing

@stevenjb for //chrome/browser/chromeos/net/*
@rkc for chrome/browser/extensions/api/copresence/*
@fgorski for chrome/browser/extensions/api/gcm/*
            chrome/browser/extensions/api/instance_id/*
@rockot for chrome/browser/extensions/*				
@dcheng for chrome/browser/invalidation/*
@skuhne  for chrome/browser/profile/*
@isherman  for chrome/browser/signin/*
@bauerb for chrome/browser/ui/webui/

[fgorski, 2015-10-30 13:19:06]

api/gcm and api/instance_id lgtm

I do have a comment to the gcm_profile_service_factory thou

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:59:
CHECK(!profile->IsOffTheRecord());
Where did this come from?
GCM is enabled in incognito. Was there a line someplace else you moved here, or
did you put it in by mistake?

[blundell, 2015-10-30 13:29:08]

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:59:
CHECK(!profile->IsOffTheRecord());
On 2015/10/30 13:19:05, fgorski wrote:
> Where did this come from?
> GCM is enabled in incognito. Was there a line someplace else you moved here,
or
> did you put it in by mistake?

Hmm, that's very strange. It used to be in the GCMProfileService constructor, so
the change made here is correct AFAICT. I don't know how to square that fact
with line 88 of this file below.

[droger, 2015-10-30 13:38:30]

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:59:
CHECK(!profile->IsOffTheRecord());
On 2015/10/30 13:29:07, blundell wrote:
> On 2015/10/30 13:19:05, fgorski wrote:
> > Where did this come from?
> > GCM is enabled in incognito. Was there a line someplace else you moved here,
> or
> > did you put it in by mistake?
> 
> Hmm, that's very strange. It used to be in the GCMProfileService constructor,
so
> the change made here is correct AFAICT. I don't know how to square that fact
> with line 88 of this file below.

I agree that the CL is fine in the sense that it does not change the existing
logic.

However, that logic indeed seems inconsistent and my hunch is that GCM is not
actually used in incognito.
We should either remove the DCHECK or remove the GetBrowserContextToUse()
method.
Let us know if you have a strong opinion.

In doubt, removing the method is better, but we may also consider this to be out
of scope of the CL.

[Bernhard Bauer, 2015-10-30 13:38:46]

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:59:
CHECK(!profile->IsOffTheRecord());
On 2015/10/30 13:29:07, blundell wrote:
> On 2015/10/30 13:19:05, fgorski wrote:
> > Where did this come from?
> > GCM is enabled in incognito. Was there a line someplace else you moved here,
> or
> > did you put it in by mistake?
> 
> Hmm, that's very strange. It used to be in the GCMProfileService constructor,
so
> the change made here is correct AFAICT. I don't know how to square that fact
> with line 88 of this file below.

Peter added this to investigate https://crbug.com/437827.

Also, this is now a CHECK instead of a DCHECK.

[fgorski, 2015-10-30 13:40:35]

fgorski@chromium.org changed reviewers:
+ jianli@chromium.org

[fgorski, 2015-10-30 13:40:36]

Jian Li,

could you please take a look at the CHECK/DCHECK(!is off the record). To my
knowledge nothing like that should be in the code since we enabled the code in
incognito.

thanks.
filip

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:59:
CHECK(!profile->IsOffTheRecord());
On 2015/10/30 13:29:07, blundell wrote:
> On 2015/10/30 13:19:05, fgorski wrote:
> > Where did this come from?
> > GCM is enabled in incognito. Was there a line someplace else you moved here,
> or
> > did you put it in by mistake?
> 
> Hmm, that's very strange. It used to be in the GCMProfileService constructor,
so
> the change made here is correct AFAICT. I don't know how to square that fact
> with line 88 of this file below.

Well in that case, thanks for bringing that to my attention... looks like we
cannot run GCM on debug builds and nobody cares...

Note, Original says DCHECK, not CHECK, which was recently removed from Android.

And I'll ask jianli@ to comment and perhaps remove the line altogether.

[blundell, 2015-10-30 13:44:49]

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:59:
CHECK(!profile->IsOffTheRecord());
On 2015/10/30 13:40:36, fgorski wrote:
> On 2015/10/30 13:29:07, blundell wrote:
> > On 2015/10/30 13:19:05, fgorski wrote:
> > > Where did this come from?
> > > GCM is enabled in incognito. Was there a line someplace else you moved
here,
> > or
> > > did you put it in by mistake?
> > 
> > Hmm, that's very strange. It used to be in the GCMProfileService
constructor,
> so
> > the change made here is correct AFAICT. I don't know how to square that fact
> > with line 88 of this file below.
> 
> Well in that case, thanks for bringing that to my attention... looks like we
> cannot run GCM on debug builds and nobody cares...
> 
> Note, Original says DCHECK, not CHECK, which was recently removed from
Android.
> 
> And I'll ask jianli@ to comment and perhaps remove the line altogether.

In any case, this should definitely be a DCHECK to match the old code -- thanks
for the catch. jitu@, can you make that fix? I think we can then land this CL
with the DCHECK in and leave the discussion of what to do with that DCHECK
(i.e., the status of the bug that Bernhard referenced) as a separate issue.

[Jitu( very slow this week), 2015-10-30 13:52:53]

PTAL :)

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
File chrome/browser/services/gcm/gcm_profile_service_factory.cc (right):

https://codereview.chromium.org/1425783002/diff/140001/chrome/browser/service...
chrome/browser/services/gcm/gcm_profile_service_factory.cc:59:
CHECK(!profile->IsOffTheRecord());
On 2015/10/30 13:44:48, blundell wrote:
> On 2015/10/30 13:40:36, fgorski wrote:
> > On 2015/10/30 13:29:07, blundell wrote:
> > > On 2015/10/30 13:19:05, fgorski wrote:
> > > > Where did this come from?
> > > > GCM is enabled in incognito. Was there a line someplace else you moved
> here,
> > > or
> > > > did you put it in by mistake?
> > > 
> > > Hmm, that's very strange. It used to be in the GCMProfileService
> constructor,
> > so
> > > the change made here is correct AFAICT. I don't know how to square that
fact
> > > with line 88 of this file below.
> > 
> > Well in that case, thanks for bringing that to my attention... looks like we
> > cannot run GCM on debug builds and nobody cares...
> > 
> > Note, Original says DCHECK, not CHECK, which was recently removed from
> Android.
> > 
> > And I'll ask jianli@ to comment and perhaps remove the line altogether.
> 
> In any case, this should definitely be a DCHECK to match the old code --
thanks
> for the catch. jitu@, can you make that fix? I think we can then land this CL
> with the DCHECK in and leave the discussion of what to do with that DCHECK
> (i.e., the status of the bug that Bernhard referenced) as a separate issue.

Nice catch
Done

[blundell, 2015-10-30 14:09:46]

lgtm

[Mr4D (OOO till 08-26), 2015-10-30 14:18:08]

profile change lgtm

[sky, 2015-10-30 15:36:50]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-10-30 15:36:51]

LGTM

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_constants.h (right):

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_driver_constants.h:5: // A handful of resource-like
constants related to the GCM Driver
nit: end sentences with '.'.

Also, no where could I find what GCM stands for, please document it.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_driver_constants.h:13: extern const
base::FilePath::CharType kGCMStoreDirname[];
Please add comments.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:5: #ifndef
CHROME_BROWSER_SERVICES_GCM_GCM_PROFILE_SERVICE_H_
fix guards.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:49: static bool
IsGCMEnabled(PrefService* prefs);
constructor/destructor before all other functions.

[dcheng, 2015-10-30 15:44:14]

chrome/browser/invalidation lgtm

https://codereview.chromium.org/1425783002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/1425783002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:219:
scoped_ptr<gcm::GCMClientFactory>(
Nit: make_scoped_ptr to take advantage of type deduction.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:52: explicit GCMProfileService(
Nit: no explicit, since it takes multiple arguments.

[dcheng, 2015-10-30 15:44:14]

chrome/browser/invalidation lgtm

https://codereview.chromium.org/1425783002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/1425783002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:219:
scoped_ptr<gcm::GCMClientFactory>(
Nit: make_scoped_ptr to take advantage of type deduction.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:52: explicit GCMProfileService(
Nit: no explicit, since it takes multiple arguments.

[rkc, 2015-10-30 15:47:34]

copresenece lgtm

[Jitu( very slow this week), 2015-11-02 06:50:55]

PTAL

https://codereview.chromium.org/1425783002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):

https://codereview.chromium.org/1425783002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:219:
scoped_ptr<gcm::GCMClientFactory>(
On 2015/10/30 15:44:14, dcheng wrote:
> Nit: make_scoped_ptr to take advantage of type deduction.

Done.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_driver_constants.h (right):

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_driver_constants.h:5: // A handful of resource-like
constants related to the GCM Driver
On 2015/10/30 15:36:51, sky wrote:
> nit: end sentences with '.'.
> 
> Also, no where could I find what GCM stands for, please document it.

Done.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_driver_constants.h:13: extern const
base::FilePath::CharType kGCMStoreDirname[];
On 2015/10/30 15:36:51, sky wrote:
> Please add comments.

Done.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.h (right):

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:5: #ifndef
CHROME_BROWSER_SERVICES_GCM_GCM_PROFILE_SERVICE_H_
On 2015/10/30 15:36:51, sky wrote:
> fix guards.

Done.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:49: static bool
IsGCMEnabled(PrefService* prefs);
On 2015/10/30 15:36:51, sky wrote:
> constructor/destructor before all other functions.

Done.

https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.h:52: explicit GCMProfileService(
On 2015/10/30 15:44:14, dcheng wrote:
> Nit: no explicit, since it takes multiple arguments.

Done.

[blundell, 2015-11-02 08:29:14]

I don't think you actually need anyone to take another look, since everyone's
nits already had LGTMs attached.

On 2015/11/02 06:50:55, jitu (slow this week) wrote:
> PTAL
> 
>
https://codereview.chromium.org/1425783002/diff/160001/chrome/browser/extensi...
> File chrome/browser/extensions/extension_gcm_app_handler_unittest.cc (right):
> 
>
https://codereview.chromium.org/1425783002/diff/160001/chrome/browser/extensi...
> chrome/browser/extensions/extension_gcm_app_handler_unittest.cc:219:
> scoped_ptr<gcm::GCMClientFactory>(
> On 2015/10/30 15:44:14, dcheng wrote:
> > Nit: make_scoped_ptr to take advantage of type deduction.
> 
> Done.
> 
>
https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
> File components/gcm_driver/gcm_driver_constants.h (right):
> 
>
https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
> components/gcm_driver/gcm_driver_constants.h:5: // A handful of resource-like
> constants related to the GCM Driver
> On 2015/10/30 15:36:51, sky wrote:
> > nit: end sentences with '.'.
> > 
> > Also, no where could I find what GCM stands for, please document it.
> 
> Done.
> 
>
https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
> components/gcm_driver/gcm_driver_constants.h:13: extern const
> base::FilePath::CharType kGCMStoreDirname[];
> On 2015/10/30 15:36:51, sky wrote:
> > Please add comments.
> 
> Done.
> 
>
https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
> File components/gcm_driver/gcm_profile_service.h (right):
> 
>
https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
> components/gcm_driver/gcm_profile_service.h:5: #ifndef
> CHROME_BROWSER_SERVICES_GCM_GCM_PROFILE_SERVICE_H_
> On 2015/10/30 15:36:51, sky wrote:
> > fix guards.
> 
> Done.
> 
>
https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
> components/gcm_driver/gcm_profile_service.h:49: static bool
> IsGCMEnabled(PrefService* prefs);
> On 2015/10/30 15:36:51, sky wrote:
> > constructor/destructor before all other functions.
> 
> Done.
> 
>
https://codereview.chromium.org/1425783002/diff/160001/components/gcm_driver/...
> components/gcm_driver/gcm_profile_service.h:52: explicit GCMProfileService(
> On 2015/10/30 15:44:14, dcheng wrote:
> > Nit: no explicit, since it takes multiple arguments.
> 
> Done.

[Jitu( very slow this week), 2015-11-02 08:46:36]

The CQ bit was checked by jitendra.ks@samsung.com

[Jitu( very slow this week), 2015-11-02 08:46:37]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
johnme@chromium.org, zea@chromium.org, fgorski@chromium.org, sky@chromium.org,
rkc@chromium.org, skuhne@chromium.org, dcheng@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1425783002/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2015-11-02 08:46:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425783002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425783002/180001

[commit-bot: I haz the power, 2015-11-02 08:48:48]

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

[commit-bot: I haz the power, 2015-11-02 08:48:50]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[blundell, 2015-11-02 15:22:46]

https://codereview.chromium.org/1425783002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_profile_service.cc (right):

https://codereview.chromium.org/1425783002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.cc:54: scoped_ptr<IdentityProvider>
identity_provider_;
This needs to be an IdentityProvider* now since this class no longer owns the
provider.

https://codereview.chromium.org/1425783002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.cc:147:
profile_identity_provider_(identity_provider.get()) {
This needs to be identity_provider.Pass() to properly transfer ownership.

https://codereview.chromium.org/1425783002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_profile_service.cc:154: identity_provider.get(),
request_context_, driver_.get()));
This needs to be identity_provider_.get() as identity_provider.get() will return
nullptr after the Pass() call mentioned above.

[Jitu( very slow this week), 2015-11-03 07:50:35]

The CQ bit was checked by jitendra.ks@samsung.com

[Jitu( very slow this week), 2015-11-03 07:50:35]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
johnme@chromium.org, sky@chromium.org, fgorski@chromium.org, rkc@chromium.org,
skuhne@chromium.org, dcheng@chromium.org, zea@chromium.org,
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1425783002/#ps240001
(title: "Fix UT errors as per comments")

[commit-bot: I haz the power, 2015-11-03 07:50:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425783002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425783002/240001

[commit-bot: I haz the power, 2015-11-03 10:06:29]

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

[commit-bot: I haz the power, 2015-11-03 10:06:31]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jitu( very slow this week), 2015-11-03 12:52:21]

On 2015/11/03 10:06:31, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

@Blundell
This is failing mac_chromium_release_ng, i am not sure why it is failing.
Please help me.

[blundell, 2015-11-03 12:55:06]

On 2015/11/03 12:52:21, jitu (slow this week) wrote:
> On 2015/11/03 10:06:31, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> @Blundell
> This is failing mac_chromium_release_ng, i am not sure why it is failing.
> Please help me.

That failure is flake that doesn't have anything to do with your CL.

[Jitu( very slow this week), 2015-11-03 13:24:28]

On 2015/11/03 12:55:06, blundell wrote:
> On 2015/11/03 12:52:21, jitu (slow this week) wrote:
> > On 2015/11/03 10:06:31, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> > 
> > @Blundell
> > This is failing mac_chromium_release_ng, i am not sure why it is failing.
> > Please help me.
> 
> That failure is flake that doesn't have anything to do with your CL.

Due to that fail this CL failed in commit Bot.
So how to proceed.

[Jitu( very slow this week), 2015-11-03 13:37:42]

The CQ bit was checked by jitendra.ks@samsung.com

[commit-bot: I haz the power, 2015-11-03 13:38:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425783002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425783002/240001

[blundell, 2015-11-03 13:51:15]

On 2015/11/03 13:24:28, jitu (slow this week) wrote:
> On 2015/11/03 12:55:06, blundell wrote:
> > On 2015/11/03 12:52:21, jitu (slow this week) wrote:
> > > On 2015/11/03 10:06:31, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> > > 
> > > @Blundell
> > > This is failing mac_chromium_release_ng, i am not sure why it is failing.
> > > Please help me.
> > 
> > That failure is flake that doesn't have anything to do with your CL.
> 
> Due to that fail this CL failed in commit Bot.
> So how to proceed.

You just need to retry. Note that right now the tree is closed in any case due
to infra issues.

[commit-bot: I haz the power, 2015-11-03 15:55:08]

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

[commit-bot: I haz the power, 2015-11-03 15:55:10]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Jitu( very slow this week), 2015-11-03 18:36:57]

The CQ bit was checked by jitendra.ks@samsung.com

[commit-bot: I haz the power, 2015-11-03 18:37:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425783002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425783002/240001

[commit-bot: I haz the power, 2015-11-03 20:18:00]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2015-11-03 20:19:28]

Patchset 12 (id:??) landed as
https://crrev.com/7554194aec4024a5409d2de575d8b59e4d9679c2
Cr-Commit-Position: refs/heads/master@{#357609}

[sdefresne, 2015-11-03 21:43:48]

On 2015/11/03 at 20:19:28, commit-bot wrote:
> Patchset 12 (id:??) landed as
https://crrev.com/7554194aec4024a5409d2de575d8b59e4d9679c2
> Cr-Commit-Position: refs/heads/master@{#357609}

Yeah!