diff options
author | jianli@chromium.org <jianli@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-03-06 01:11:13 +0000 |
---|---|---|
committer | jianli@chromium.org <jianli@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-03-06 01:11:13 +0000 |
commit | a0d2a385a9e3cbfd0ed144a8b8aa22438729baa4 (patch) | |
tree | 29e0b6daa55f513ffa08fb1e6782f519e342c283 | |
parent | cbb771143981edc5c11846add040268cfd013184 (diff) | |
download | chromium_src-a0d2a385a9e3cbfd0ed144a8b8aa22438729baa4.zip chromium_src-a0d2a385a9e3cbfd0ed144a8b8aa22438729baa4.tar.gz chromium_src-a0d2a385a9e3cbfd0ed144a8b8aa22438729baa4.tar.bz2 |
Merge 253787 "[GCM] Make sure GCM checkout logic is invoked when..."
> [GCM] Make sure GCM checkout logic is invoked when the profile is signed out
>
> We now always create GCMProfileService that listens to profile
> notifications such that GCM checkout logic could be invoked when the
> profile is signed out.
>
> We now move the GCM check-in logic from GCMClient::Initialize to GCMClient::CheckIn.
>
> BUG=344031
> TEST=new tests added and existing tests updated
>
> Review URL: https://codereview.chromium.org/165993005
TBR=jianli@chromium.org
Review URL: https://codereview.chromium.org/184273011
git-svn-id: svn://svn.chromium.org/chrome/branches/1847/src@255193 0039d316-1c4b-4281-b951-d872f2087c98
19 files changed, 420 insertions, 228 deletions
diff --git a/chrome/browser/extensions/api/gcm/gcm_api.cc b/chrome/browser/extensions/api/gcm/gcm_api.cc index a8a5be2..ca57f8a 100644 --- a/chrome/browser/extensions/api/gcm/gcm_api.cc +++ b/chrome/browser/extensions/api/gcm/gcm_api.cc @@ -97,8 +97,14 @@ bool GcmApiFunction::RunImpl() { } bool GcmApiFunction::IsGcmApiEnabled() const { - return gcm::GCMProfileService::IsGCMEnabled( - Profile::FromBrowserContext(context())); + Profile* profile = Profile::FromBrowserContext(context()); + + // GCM is not supported in incognito mode. + if (profile->IsOffTheRecord()) + return false; + + return gcm::GCMProfileService::GetGCMEnabledState(profile) != + gcm::GCMProfileService::ALWAYS_DISABLED; } gcm::GCMProfileService* GcmApiFunction::GCMProfileService() const { diff --git a/chrome/browser/extensions/api/gcm/gcm_apitest.cc b/chrome/browser/extensions/api/gcm/gcm_apitest.cc index 41372a1..c463ed9 100644 --- a/chrome/browser/extensions/api/gcm/gcm_apitest.cc +++ b/chrome/browser/extensions/api/gcm/gcm_apitest.cc @@ -9,6 +9,7 @@ #include "chrome/browser/services/gcm/fake_gcm_profile_service.h" #include "chrome/browser/services/gcm/gcm_client_factory.h" #include "chrome/browser/services/gcm/gcm_profile_service_factory.h" +#include "chrome/common/chrome_switches.h" #include "chrome/common/extensions/features/feature_channel.h" #include "chrome/test/base/ui_test_utils.h" @@ -25,6 +26,7 @@ class GcmApiTest : public ExtensionApiTest { GcmApiTest() : fake_gcm_profile_service_(NULL) {} protected: + virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE; virtual void SetUpOnMainThread() OVERRIDE; void StartCollecting(); @@ -38,13 +40,24 @@ class GcmApiTest : public ExtensionApiTest { gcm::FakeGCMProfileService* fake_gcm_profile_service_; }; +void GcmApiTest::SetUpCommandLine(CommandLine* command_line) { + // We now always create the GCMProfileService instance in + // ProfileSyncServiceFactory that is called when a profile is being + // initialized. In order to prevent it from being created, we add the switch + // to disable the sync logic. + command_line->AppendSwitch(switches::kDisableSync); + + ExtensionApiTest::SetUpCommandLine(command_line); +} + void GcmApiTest::SetUpOnMainThread() { gcm::GCMProfileServiceFactory::GetInstance()->SetTestingFactory( browser()->profile(), &gcm::FakeGCMProfileService::Build); fake_gcm_profile_service_ = static_cast<gcm::FakeGCMProfileService*>( gcm::GCMProfileServiceFactory::GetInstance()->GetForProfile( browser()->profile())); - gcm::FakeGCMProfileService::EnableGCMForTesting(); + + ExtensionApiTest::SetUpOnMainThread(); } void GcmApiTest::StartCollecting() { diff --git a/chrome/browser/services/gcm/fake_gcm_profile_service.cc b/chrome/browser/services/gcm/fake_gcm_profile_service.cc index dad6bff..c124f4f 100644 --- a/chrome/browser/services/gcm/fake_gcm_profile_service.cc +++ b/chrome/browser/services/gcm/fake_gcm_profile_service.cc @@ -19,11 +19,6 @@ BrowserContextKeyedService* FakeGCMProfileService::Build( return new FakeGCMProfileService(profile); } -// static -void FakeGCMProfileService::EnableGCMForTesting() { - GCMProfileService::enable_gcm_for_testing_ = true; -} - FakeGCMProfileService::FakeGCMProfileService(Profile* profile) : GCMProfileService(profile), collect_(false) {} diff --git a/chrome/browser/services/gcm/fake_gcm_profile_service.h b/chrome/browser/services/gcm/fake_gcm_profile_service.h index 65757fe..1848c0b 100644 --- a/chrome/browser/services/gcm/fake_gcm_profile_service.h +++ b/chrome/browser/services/gcm/fake_gcm_profile_service.h @@ -20,8 +20,6 @@ class FakeGCMProfileService : public GCMProfileService { // BrowserContextKeyedService::SetTestingFactory(). static BrowserContextKeyedService* Build(content::BrowserContext* context); - static void EnableGCMForTesting(); - explicit FakeGCMProfileService(Profile* profile); virtual ~FakeGCMProfileService(); diff --git a/chrome/browser/services/gcm/gcm_client_mock.cc b/chrome/browser/services/gcm/gcm_client_mock.cc index 45f4349..e701013 100644 --- a/chrome/browser/services/gcm/gcm_client_mock.cc +++ b/chrome/browser/services/gcm/gcm_client_mock.cc @@ -13,10 +13,13 @@ namespace gcm { -GCMClientMock::GCMClientMock(Status status, ErrorSimulation error_simulation) +GCMClientMock::GCMClientMock(LoadingDelay loading_delay, + ErrorSimulation error_simulation) : delegate_(NULL), - status_(status), - error_simulation_(error_simulation) { + status_(UNINITIALIZED), + loading_delay_(loading_delay), + error_simulation_(error_simulation), + weak_ptr_factory_(this) { } GCMClientMock::~GCMClientMock() { @@ -32,8 +35,26 @@ void GCMClientMock::Initialize( delegate_ = delegate; } +void GCMClientMock::Load() { + DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); + DCHECK_NE(LOADED, status_); + + if (loading_delay_ == DELAY_LOADING) + return; + DoLoading(); +} + +void GCMClientMock::DoLoading() { + status_ = LOADED; + base::MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&GCMClientMock::CheckinFinished, + weak_ptr_factory_.GetWeakPtr())); +} + void GCMClientMock::CheckOut() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); + status_ = CHECKED_OUT; } void GCMClientMock::Register(const std::string& app_id, @@ -48,7 +69,7 @@ void GCMClientMock::Register(const std::string& app_id, base::MessageLoop::current()->PostTask( FROM_HERE, base::Bind(&GCMClientMock::RegisterFinished, - base::Unretained(this), + weak_ptr_factory_.GetWeakPtr(), app_id, registration_id)); } @@ -64,13 +85,18 @@ void GCMClientMock::Send(const std::string& app_id, base::MessageLoop::current()->PostTask( FROM_HERE, base::Bind(&GCMClientMock::SendFinished, - base::Unretained(this), + weak_ptr_factory_.GetWeakPtr(), app_id, message.id)); } -bool GCMClientMock::IsReady() const { - return status_ == READY; +void GCMClientMock::PerformDelayedLoading() { + DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); + + content::BrowserThread::PostTask( + content::BrowserThread::IO, + FROM_HERE, + base::Bind(&GCMClientMock::DoLoading, weak_ptr_factory_.GetWeakPtr())); } void GCMClientMock::ReceiveMessage(const std::string& app_id, @@ -81,7 +107,7 @@ void GCMClientMock::ReceiveMessage(const std::string& app_id, content::BrowserThread::IO, FROM_HERE, base::Bind(&GCMClientMock::MessageReceived, - base::Unretained(this), + weak_ptr_factory_.GetWeakPtr(), app_id, message)); } @@ -93,22 +119,10 @@ void GCMClientMock::DeleteMessages(const std::string& app_id) { content::BrowserThread::IO, FROM_HERE, base::Bind(&GCMClientMock::MessagesDeleted, - base::Unretained(this), + weak_ptr_factory_.GetWeakPtr(), app_id)); } -void GCMClientMock::SetReady() { - DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - DCHECK_EQ(status_, NOT_READY); - - status_ = READY; - content::BrowserThread::PostTask( - content::BrowserThread::IO, - FROM_HERE, - base::Bind(&GCMClientMock::SetReadyOnIO, - base::Unretained(this))); -} - // static std::string GCMClientMock::GetRegistrationIdFromSenderIds( const std::vector<std::string>& sender_ids) { @@ -131,6 +145,10 @@ std::string GCMClientMock::GetRegistrationIdFromSenderIds( return registration_id; } +void GCMClientMock::CheckinFinished() { + delegate_->OnGCMReady(); +} + void GCMClientMock::RegisterFinished(const std::string& app_id, const std::string& registrion_id) { delegate_->OnRegisterFinished( @@ -146,7 +164,7 @@ void GCMClientMock::SendFinished(const std::string& app_id, base::MessageLoop::current()->PostDelayedTask( FROM_HERE, base::Bind(&GCMClientMock::MessageSendError, - base::Unretained(this), + weak_ptr_factory_.GetWeakPtr(), app_id, message_id), base::TimeDelta::FromMilliseconds(200)); @@ -170,8 +188,4 @@ void GCMClientMock::MessageSendError(const std::string& app_id, delegate_->OnMessageSendError(app_id, message_id, NETWORK_ERROR); } -void GCMClientMock::SetReadyOnIO() { - delegate_->OnGCMReady(); -} - } // namespace gcm diff --git a/chrome/browser/services/gcm/gcm_client_mock.h b/chrome/browser/services/gcm/gcm_client_mock.h index 485a6a8..a10b483 100644 --- a/chrome/browser/services/gcm/gcm_client_mock.h +++ b/chrome/browser/services/gcm/gcm_client_mock.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_SERVICES_GCM_GCM_CLIENT_MOCK_H_ #include "base/compiler_specific.h" +#include "base/memory/weak_ptr.h" #include "google_apis/gcm/gcm_client.h" namespace gcm { @@ -13,8 +14,14 @@ namespace gcm { class GCMClientMock : public GCMClient { public: enum Status { - NOT_READY, - READY + UNINITIALIZED, + LOADED, + CHECKED_OUT + }; + + enum LoadingDelay { + NO_DELAY_LOADING, + DELAY_LOADING, }; enum ErrorSimulation { @@ -22,9 +29,9 @@ class GCMClientMock : public GCMClient { FORCE_ERROR }; - // |status| denotes if the fake client is ready or not at the beginning. + // |loading_delay| denotes if the check-in should be delayed. // |error_simulation| denotes if we should simulate server error. - GCMClientMock(Status status, ErrorSimulation error_simulation); + GCMClientMock(LoadingDelay loading_delay, ErrorSimulation error_simulation); virtual ~GCMClientMock(); // Overridden from GCMClient: @@ -36,6 +43,7 @@ class GCMClientMock : public GCMClient { const scoped_refptr<net::URLRequestContextGetter>& url_request_context_getter, Delegate* delegate) OVERRIDE; + virtual void Load() OVERRIDE; virtual void CheckOut() OVERRIDE; virtual void Register(const std::string& app_id, const std::string& cert, @@ -44,7 +52,10 @@ class GCMClientMock : public GCMClient { virtual void Send(const std::string& app_id, const std::string& receiver_id, const OutgoingMessage& message) OVERRIDE; - virtual bool IsReady() const OVERRIDE; + + // Initiate the loading that has been delayed. + // Called on UI thread. + void PerformDelayedLoading(); // Simulate receiving something from the server. // Called on UI thread. @@ -52,14 +63,15 @@ class GCMClientMock : public GCMClient { const IncomingMessage& message); void DeleteMessages(const std::string& app_id); - // Can only transition from non-ready to ready. - void SetReady(); - static std::string GetRegistrationIdFromSenderIds( const std::vector<std::string>& sender_ids); + Status status() const { return status_; } + private: // Called on IO thread. + void DoLoading(); + void CheckinFinished(); void RegisterFinished(const std::string& app_id, const std::string& registrion_id); void SendFinished(const std::string& app_id, const std::string& message_id); @@ -68,11 +80,12 @@ class GCMClientMock : public GCMClient { void MessagesDeleted(const std::string& app_id); void MessageSendError(const std::string& app_id, const std::string& message_id); - void SetReadyOnIO(); Delegate* delegate_; Status status_; + LoadingDelay loading_delay_; ErrorSimulation error_simulation_; + base::WeakPtrFactory<GCMClientMock> weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN(GCMClientMock); }; diff --git a/chrome/browser/services/gcm/gcm_profile_service.cc b/chrome/browser/services/gcm/gcm_profile_service.cc index b58f7e6..8bb4627 100644 --- a/chrome/browser/services/gcm/gcm_profile_service.cc +++ b/chrome/browser/services/gcm/gcm_profile_service.cc @@ -232,7 +232,7 @@ class GCMProfileService::IOWorker public base::RefCountedThreadSafe<GCMProfileService::IOWorker>{ public: // Called on UI thread. - explicit IOWorker(const base::WeakPtr<GCMProfileService>& service); + IOWorker(); // Overridden from GCMClient::Delegate: // Called on IO thread. @@ -255,11 +255,12 @@ class GCMProfileService::IOWorker // Called on IO thread. void Initialize( - GCMClientFactory* gcm_client_factory, + scoped_ptr<GCMClientFactory> gcm_client_factory, const base::FilePath& store_path, const scoped_refptr<net::URLRequestContextGetter>& url_request_context_getter); void Reset(); + void Load(const base::WeakPtr<GCMProfileService>& service); void CheckOut(); void Register(const std::string& app_id, const std::vector<std::string>& sender_ids, @@ -269,18 +270,19 @@ class GCMProfileService::IOWorker const std::string& receiver_id, const GCMClient::OutgoingMessage& message); + // For testing purpose. Can be called from UI thread. Use with care. + GCMClient* gcm_client_for_testing() const { return gcm_client_.get(); } + private: friend class base::RefCountedThreadSafe<IOWorker>; virtual ~IOWorker(); - const base::WeakPtr<GCMProfileService> service_; + base::WeakPtr<GCMProfileService> service_; scoped_ptr<GCMClient> gcm_client_; }; -GCMProfileService::IOWorker::IOWorker( - const base::WeakPtr<GCMProfileService>& service) - : service_(service) { +GCMProfileService::IOWorker::IOWorker() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); } @@ -288,7 +290,7 @@ GCMProfileService::IOWorker::~IOWorker() { } void GCMProfileService::IOWorker::Initialize( - GCMClientFactory* gcm_client_factory, + scoped_ptr<GCMClientFactory> gcm_client_factory, const base::FilePath& store_path, const scoped_refptr<net::URLRequestContextGetter>& url_request_context_getter) { @@ -313,13 +315,6 @@ void GCMProfileService::IOWorker::Initialize( blocking_task_runner, url_request_context_getter, this); - - content::BrowserThread::PostTask( - content::BrowserThread::UI, - FROM_HERE, - base::Bind(&GCMProfileService::FinishInitializationOnUI, - service_, - gcm_client_->IsReady())); } void GCMProfileService::IOWorker::Reset() { @@ -417,11 +412,21 @@ void GCMProfileService::IOWorker::OnGCMReady() { service_)); } +void GCMProfileService::IOWorker::Load( + const base::WeakPtr<GCMProfileService>& service) { + DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); + + service_ = service; + gcm_client_->Load(); +} + void GCMProfileService::IOWorker::CheckOut() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); gcm_client_->CheckOut(); - gcm_client_.reset(); + + // Note that we still need to keep GCMClient instance alive since the profile + // might be signed in again. } void GCMProfileService::IOWorker::Register( @@ -458,18 +463,19 @@ bool GCMProfileService::RegistrationInfo::IsValid() const { return !sender_ids.empty() && !registration_id.empty(); } -bool GCMProfileService::enable_gcm_for_testing_ = false; - // static -bool GCMProfileService::IsGCMEnabled(Profile* profile) { - // GCM is not enabled in incognito mode. - if (profile->IsOffTheRecord()) - return false; +GCMProfileService::GCMEnabledState GCMProfileService::GetGCMEnabledState( + Profile* profile) { + const base::Value* gcm_enabled_value = + profile->GetPrefs()->GetUserPrefValue(prefs::kGCMChannelEnabled); + if (!gcm_enabled_value) + return ENABLED_FOR_APPS; - if (enable_gcm_for_testing_) - return true; + bool gcm_enabled = false; + if (!gcm_enabled_value->GetAsBoolean(&gcm_enabled)) + return ENABLED_FOR_APPS; - return profile->GetPrefs()->GetBoolean(prefs::kGCMChannelEnabled); + return gcm_enabled ? ALWAYS_ENABLED : ALWAYS_DISABLED; } // static @@ -505,15 +511,6 @@ GCMProfileService::~GCMProfileService() { void GCMProfileService::Initialize( scoped_ptr<GCMClientFactory> gcm_client_factory) { - gcm_client_factory_ = gcm_client_factory.Pass(); - - // This has to be done first since CheckIn depends on it. - io_worker_ = new IOWorker(weak_ptr_factory_.GetWeakPtr()); - -#if !defined(OS_ANDROID) - js_event_router_.reset(new extensions::GcmJsEventRouter(profile_)); -#endif - registrar_.Add(this, chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL, content::Source<Profile>(profile_)); @@ -528,14 +525,24 @@ void GCMProfileService::Initialize( chrome:: NOTIFICATION_EXTENSION_UNINSTALLED, content::Source<Profile>(profile_)); - // In case that the profile has been signed in before GCMProfileService is - // created. - SigninManagerBase* manager = SigninManagerFactory::GetForProfile(profile_); - if (manager) { - std::string username = manager->GetAuthenticatedUsername(); - if (!username.empty()) - CheckIn(username); - } + // Create and initialize the GCMClient. Note that this does not initiate the + // GCM check-in. + io_worker_ = new IOWorker(); + scoped_refptr<net::URLRequestContextGetter> url_request_context_getter = + profile_->GetRequestContext(); + content::BrowserThread::PostTask( + content::BrowserThread::IO, + FROM_HERE, + base::Bind(&GCMProfileService::IOWorker::Initialize, + io_worker_, + base::Passed(&gcm_client_factory), + profile_->GetPath().Append(chrome::kGCMStoreDirname), + url_request_context_getter)); + + // Load from the GCM store and initiate the GCM check-in if the rollout signal + // indicates yes. + if (GetGCMEnabledState(profile_) == ALWAYS_ENABLED) + EnsureLoaded(); } void GCMProfileService::Register(const std::string& app_id, @@ -545,6 +552,9 @@ void GCMProfileService::Register(const std::string& app_id, DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); DCHECK(!app_id.empty() && !sender_ids.empty() && !callback.is_null()); + // Ensure that check-in has been done. + EnsureLoaded(); + // If the profile was not signed in, bail out. if (username_.empty()) { callback.Run(std::string(), GCMClient::NOT_SIGNED_IN); @@ -630,6 +640,9 @@ void GCMProfileService::Send(const std::string& app_id, DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); DCHECK(!app_id.empty() && !receiver_id.empty() && !callback.is_null()); + // Ensure that check-in has been done. + EnsureLoaded(); + // If the profile was not signed in, bail out. if (username_.empty()) { callback.Run(std::string(), GCMClient::NOT_SIGNED_IN); @@ -674,6 +687,10 @@ void GCMProfileService::DoSend(const std::string& app_id, message)); } +GCMClient* GCMProfileService::GetGCMClientForTesting() const { + return io_worker_ ? io_worker_->gcm_client_for_testing() : NULL; +} + void GCMProfileService::Observe(int type, const content::NotificationSource& source, const content::NotificationDetails& details) { @@ -681,11 +698,8 @@ void GCMProfileService::Observe(int type, switch (type) { case chrome::NOTIFICATION_GOOGLE_SIGNIN_SUCCESSFUL: { - const GoogleServiceSigninSuccessDetails* signin_details = - content::Details<GoogleServiceSigninSuccessDetails>(details).ptr(); - // This could be called multiple times when the password changed. - if (username_ != signin_details->username) - CheckIn(signin_details->username); + if (GetGCMEnabledState(profile_) == ALWAYS_ENABLED) + EnsureLoaded(); break; } case chrome::NOTIFICATION_GOOGLE_SIGNED_OUT: @@ -694,46 +708,68 @@ void GCMProfileService::Observe(int type, case chrome::NOTIFICATION_PROFILE_DESTROYED: ResetGCMClient(); break; - case chrome:: NOTIFICATION_EXTENSION_UNINSTALLED: { - extensions::Extension* extension = - content::Details<extensions::Extension>(details).ptr(); - Unregister(extension->id()); + case chrome:: NOTIFICATION_EXTENSION_UNINSTALLED: + if (!username_.empty()) { + extensions::Extension* extension = + content::Details<extensions::Extension>(details).ptr(); + Unregister(extension->id()); + } break; - } default: NOTREACHED(); } } -void GCMProfileService::CheckIn(const std::string& username) { - DCHECK(!username.empty() && username_.empty()); +void GCMProfileService::EnsureLoaded() { + SigninManagerBase* manager = SigninManagerFactory::GetForProfile(profile_); + if (!manager) + return; + std::string username = manager->GetAuthenticatedUsername(); + if (username.empty()) + return; + + // CheckIn could be called more than once when: + // 1) The password changes. + // 2) Register/send function calls it to ensure CheckIn is done. + if (username_ == username) + return; username_ = username; +#if !defined(OS_ANDROID) + if (!js_event_router_) + js_event_router_.reset(new extensions::GcmJsEventRouter(profile_)); +#endif + DCHECK(!delayed_task_controller_); delayed_task_controller_.reset(new DelayedTaskController); - // Load all register apps. + // Load all the registered apps. ReadRegisteredAppIDs(); - // Let the IO thread create and initialize GCMClient. - scoped_refptr<net::URLRequestContextGetter> url_request_context_getter = - profile_->GetRequestContext(); + // This will load the data from the gcm store and trigger the check-in if + // the persisted check-in info is not found. + // Note that we need to pass weak pointer again since the existing weak + // pointer in IOWorker might have been invalidated when check-out occurs. content::BrowserThread::PostTask( content::BrowserThread::IO, FROM_HERE, - base::Bind(&GCMProfileService::IOWorker::Initialize, + base::Bind(&GCMProfileService::IOWorker::Load, io_worker_, - gcm_client_factory_.get(), - profile_->GetPath().Append(chrome::kGCMStoreDirname), - url_request_context_getter)); + weak_ptr_factory_.GetWeakPtr())); } void GCMProfileService::CheckOut() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - DCHECK(!username_.empty()); + // We still proceed with the check-out logic even if the check-in is not + // initiated in the current session. This will make sure that all the + // persisted data written previously will get purged. username_.clear(); + // Remove all the queued tasks since they no longer make sense after + // check-out. + weak_ptr_factory_.InvalidateWeakPtrs(); + // Remove persisted data from app's state store. for (RegistrationInfoMap::const_iterator iter = registration_info_map_.begin(); @@ -742,7 +778,6 @@ void GCMProfileService::CheckOut() { } // Remove persisted data from prefs store. - profile_->GetPrefs()->ClearPref(prefs::kGCMChannelEnabled); profile_->GetPrefs()->ClearPref(prefs::kGCMRegisteredAppIDs); gcm_client_ready_ = false; @@ -888,14 +923,6 @@ void GCMProfileService::MessageSendError(const std::string& app_id, GetEventRouter(app_id)->OnSendError(app_id, message_id, result); } -void GCMProfileService::FinishInitializationOnUI(bool ready) { - DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); - - gcm_client_ready_ = ready; - if (gcm_client_ready_) - delayed_task_controller_->SetGCMReady(); -} - void GCMProfileService::GCMClientReady() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); diff --git a/chrome/browser/services/gcm/gcm_profile_service.h b/chrome/browser/services/gcm/gcm_profile_service.h index 633a6e8c..a516d71 100644 --- a/chrome/browser/services/gcm/gcm_profile_service.h +++ b/chrome/browser/services/gcm/gcm_profile_service.h @@ -47,14 +47,24 @@ class GCMProfileService : public BrowserContextKeyedService, typedef base::Callback<void(const std::string& message_id, GCMClient::Result result)> SendCallback; + enum GCMEnabledState { + // GCM is always enabled. GCMClient will always load and connect with GCM. + ALWAYS_ENABLED, + // GCM is only enabled for apps. GCMClient will start to load and connect + // with GCM only when GCM API is used. + ENABLED_FOR_APPS, + // GCM is always disabled. GCMClient will never load and connect with GCM. + ALWAYS_DISABLED + }; + // For testing purpose. class TestingDelegate { public: virtual GCMEventRouter* GetEventRouter() const = 0; }; - // Returns true if the GCM support is enabled. - static bool IsGCMEnabled(Profile* profile); + // Returns the GCM enabled state. + static GCMEnabledState GetGCMEnabledState(Profile* profile); // Register profile-specific prefs for GCM. static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry); @@ -88,15 +98,12 @@ class GCMProfileService : public BrowserContextKeyedService, SendCallback callback); // For testing purpose. + GCMClient* GetGCMClientForTesting() const; + void set_testing_delegate(TestingDelegate* testing_delegate) { testing_delegate_ = testing_delegate; } - protected: - // Flag that could be set by the testing code to enable GCM. Otherwise, - // tests from official build will fail. - static bool enable_gcm_for_testing_; - private: friend class GCMProfileServiceTestConsumer; @@ -117,9 +124,9 @@ class GCMProfileService : public BrowserContextKeyedService, const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; - // Checks in with GCM by creating and initializing GCMClient when the profile - // has been signed in. - void CheckIn(const std::string& username); + // Ensures that the GCMClient is loaded and the GCM check-in is done when + // the profile was signed in. + void EnsureLoaded(); // Checks out of GCM when the profile has been signed out. This will erase // all the cached and persisted data. @@ -155,7 +162,6 @@ class GCMProfileService : public BrowserContextKeyedService, void MessageSendError(const std::string& app_id, const std::string& message_id, GCMClient::Result result); - void FinishInitializationOnUI(bool ready); void GCMClientReady(); // Returns the event router to fire the event for the given app. @@ -181,9 +187,6 @@ class GCMProfileService : public BrowserContextKeyedService, // The profile which owns this object. Profile* profile_; - // Used to creat the GCMClient instance. - scoped_ptr<GCMClientFactory> gcm_client_factory_; - // Flag to indicate if GCMClient is ready. bool gcm_client_ready_; diff --git a/chrome/browser/services/gcm/gcm_profile_service_factory.cc b/chrome/browser/services/gcm/gcm_profile_service_factory.cc index aff75d8..c5b2469 100644 --- a/chrome/browser/services/gcm/gcm_profile_service_factory.cc +++ b/chrome/browser/services/gcm/gcm_profile_service_factory.cc @@ -14,7 +14,8 @@ namespace gcm { // static GCMProfileService* GCMProfileServiceFactory::GetForProfile(Profile* profile) { - if (!gcm::GCMProfileService::IsGCMEnabled(profile)) + // GCM is not supported in incognito mode. + if (profile->IsOffTheRecord()) return NULL; return static_cast<GCMProfileService*>( @@ -38,8 +39,6 @@ GCMProfileServiceFactory::~GCMProfileServiceFactory() { BrowserContextKeyedService* GCMProfileServiceFactory::BuildServiceInstanceFor( content::BrowserContext* context) const { Profile* profile = static_cast<Profile*>(context); - if (!gcm::GCMProfileService::IsGCMEnabled(profile)) - return NULL; GCMProfileService* service = new GCMProfileService(profile); scoped_ptr<GCMClientFactory> gcm_client_factory(new GCMClientFactory); service->Initialize(gcm_client_factory.Pass()); diff --git a/chrome/browser/services/gcm/gcm_profile_service_unittest.cc b/chrome/browser/services/gcm/gcm_profile_service_unittest.cc index 395fa14..99a8281 100644 --- a/chrome/browser/services/gcm/gcm_profile_service_unittest.cc +++ b/chrome/browser/services/gcm/gcm_profile_service_unittest.cc @@ -98,7 +98,14 @@ class Waiter { void OnIOLoopPump() { DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); - base::MessageLoop::current()->RunUntilIdle(); + content::BrowserThread::PostTask( + content::BrowserThread::IO, + FROM_HERE, + base::Bind(&Waiter::OnIOLoopPumpCompleted, base::Unretained(this))); + } + + void OnIOLoopPumpCompleted() { + DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO)); content::BrowserThread::PostTask( content::BrowserThread::UI, @@ -214,9 +221,9 @@ class FakeGCMEventRouter : public GCMEventRouter { class FakeGCMClientFactory : public GCMClientFactory { public: FakeGCMClientFactory( - GCMClientMock::Status gcm_client_initial_status, + GCMClientMock::LoadingDelay gcm_client_loading_delay, GCMClientMock::ErrorSimulation gcm_client_error_simulation) - : gcm_client_initial_status_(gcm_client_initial_status), + : gcm_client_loading_delay_(gcm_client_loading_delay), gcm_client_error_simulation_(gcm_client_error_simulation), gcm_client_(NULL) { } @@ -225,7 +232,7 @@ class FakeGCMClientFactory : public GCMClientFactory { } virtual scoped_ptr<GCMClient> BuildInstance() OVERRIDE { - gcm_client_ = new GCMClientMock(gcm_client_initial_status_, + gcm_client_ = new GCMClientMock(gcm_client_loading_delay_, gcm_client_error_simulation_); return scoped_ptr<GCMClient>(gcm_client_); } @@ -233,7 +240,7 @@ class FakeGCMClientFactory : public GCMClientFactory { GCMClientMock* gcm_client() const { return gcm_client_; } private: - GCMClientMock::Status gcm_client_initial_status_; + GCMClientMock::LoadingDelay gcm_client_loading_delay_; GCMClientMock::ErrorSimulation gcm_client_error_simulation_; GCMClientMock* gcm_client_; @@ -256,7 +263,7 @@ class GCMProfileServiceTestConsumer : public GCMProfileService::TestingDelegate{ : waiter_(waiter), extension_service_(NULL), signin_manager_(NULL), - gcm_client_initial_status_(GCMClientMock::READY), + gcm_client_loading_delay_(GCMClientMock::NO_DELAY_LOADING), gcm_client_error_simulation_(GCMClientMock::ALWAYS_SUCCEED), registration_result_(GCMClient::SUCCESS), has_persisted_registration_info_(false), @@ -278,7 +285,7 @@ class GCMProfileServiceTestConsumer : public GCMProfileService::TestingDelegate{ extension_service_ = extension_system->Get(profile())->extension_service(); // Enable GCM such that tests could be run on all channels. - GCMProfileService::enable_gcm_for_testing_ = true; + profile()->GetPrefs()->SetBoolean(prefs::kGCMChannelEnabled, true); // Mock a GCMEventRouter. gcm_event_router_.reset(new FakeGCMEventRouter(waiter_)); @@ -334,7 +341,7 @@ class GCMProfileServiceTestConsumer : public GCMProfileService::TestingDelegate{ profile(), &GCMProfileServiceTestConsumer::BuildGCMProfileService)); gcm_profile_service->set_testing_delegate(this); scoped_ptr<GCMClientFactory> gcm_client_factory( - new FakeGCMClientFactory(gcm_client_initial_status_, + new FakeGCMClientFactory(gcm_client_loading_delay_, gcm_client_error_simulation_)); gcm_profile_service->Initialize(gcm_client_factory.Pass()); waiter_->PumpIOLoop(); @@ -411,8 +418,8 @@ class GCMProfileServiceTestConsumer : public GCMProfileService::TestingDelegate{ } GCMClientMock* GetGCMClient() const { - return static_cast<FakeGCMClientFactory*>( - GetGCMProfileService()->gcm_client_factory_.get())->gcm_client(); + return static_cast<GCMClientMock*>( + GetGCMProfileService()->GetGCMClientForTesting()); } const std::string& GetUsername() const { @@ -432,8 +439,8 @@ class GCMProfileServiceTestConsumer : public GCMProfileService::TestingDelegate{ return gcm_event_router_.get(); } - void set_gcm_client_initial_status(GCMClientMock::Status status) { - gcm_client_initial_status_ = status; + void set_gcm_client_loading_delay(GCMClientMock::LoadingDelay delay) { + gcm_client_loading_delay_ = delay; } void set_gcm_client_error_simulation(GCMClientMock::ErrorSimulation error) { gcm_client_error_simulation_ = error; @@ -465,7 +472,7 @@ class GCMProfileServiceTestConsumer : public GCMProfileService::TestingDelegate{ FakeSigninManager* signin_manager_; // Not owned. scoped_ptr<FakeGCMEventRouter> gcm_event_router_; - GCMClientMock::Status gcm_client_initial_status_; + GCMClientMock::LoadingDelay gcm_client_loading_delay_; GCMClientMock::ErrorSimulation gcm_client_error_simulation_; std::string registration_id_; @@ -584,7 +591,67 @@ TEST_F(GCMProfileServiceTest, CreateGCMProfileServiceAfterProfileSignIn) { EXPECT_FALSE(consumer()->GetUsername().empty()); } -TEST_F(GCMProfileServiceTest, RegsiterWhenNotSignedIn) { +TEST_F(GCMProfileServiceTest, SignInAndSignOutUnderPositiveChannelSignal) { + // Positive channel signal is provided in SetUp. + consumer()->CreateGCMProfileServiceInstance(); + consumer()->SignIn(kTestingUsername); + + // GCMClient should be loaded. + EXPECT_TRUE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::LOADED, consumer()->GetGCMClient()->status()); + + consumer()->SignOut(); + + // GCMClient should be checked out. + EXPECT_FALSE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::CHECKED_OUT, consumer()->GetGCMClient()->status()); +} + +TEST_F(GCMProfileServiceTest, SignInAndSignOutUnderNegativeChannelSignal) { + // Negative channel signal will prevent GCMClient from checking in when the + // profile is signed in. + profile()->GetPrefs()->SetBoolean(prefs::kGCMChannelEnabled, false); + + consumer()->CreateGCMProfileServiceInstance(); + consumer()->SignIn(kTestingUsername); + + // GCMClient should not be loaded. + EXPECT_FALSE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::UNINITIALIZED, consumer()->GetGCMClient()->status()); + + consumer()->SignOut(); + + // Check-out should still be performed. + EXPECT_FALSE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::CHECKED_OUT, consumer()->GetGCMClient()->status()); +} + +TEST_F(GCMProfileServiceTest, SignOutAndThenSignIn) { + // Positive channel signal is provided in SetUp. + consumer()->CreateGCMProfileServiceInstance(); + consumer()->SignIn(kTestingUsername); + + // GCMClient should be loaded. + EXPECT_TRUE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::LOADED, consumer()->GetGCMClient()->status()); + + consumer()->SignOut(); + + // GCMClient should be checked out. + EXPECT_FALSE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::CHECKED_OUT, consumer()->GetGCMClient()->status()); + + // Sign-in with a different username. + consumer()->SignIn(kTestingUsername2); + + // GCMClient should be loaded again. + EXPECT_TRUE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::LOADED, consumer()->GetGCMClient()->status()); +} + +TEST_F(GCMProfileServiceTest, RegisterWhenNotSignedIn) { + consumer()->CreateGCMProfileServiceInstance(); + std::vector<std::string> sender_ids; sender_ids.push_back("sender1"); consumer()->Register(kTestingAppId, sender_ids); @@ -593,7 +660,38 @@ TEST_F(GCMProfileServiceTest, RegsiterWhenNotSignedIn) { EXPECT_EQ(GCMClient::NOT_SIGNED_IN, consumer()->registration_result()); } +TEST_F(GCMProfileServiceTest, RegisterUnderNeutralChannelSignal) { + // Neutral channel signal will prevent GCMClient from checking in when the + // profile is signed in. + profile()->GetPrefs()->ClearPref(prefs::kGCMChannelEnabled); + + consumer()->CreateGCMProfileServiceInstance(); + consumer()->SignIn(kTestingUsername); + + // GCMClient should not be checked in. + EXPECT_FALSE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::UNINITIALIZED, consumer()->GetGCMClient()->status()); + + // Invoking register will make GCMClient checked in. + std::vector<std::string> sender_ids; + sender_ids.push_back("sender1"); + consumer()->Register(kTestingAppId, sender_ids); + WaitUntilCompleted(); + + // GCMClient should be checked in. + EXPECT_TRUE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::LOADED, consumer()->GetGCMClient()->status()); + + // Registration should succeed. + std::string expected_registration_id = + GCMClientMock::GetRegistrationIdFromSenderIds(sender_ids); + EXPECT_EQ(expected_registration_id, consumer()->registration_id()); + EXPECT_EQ(GCMClient::SUCCESS, consumer()->registration_result()); +} + TEST_F(GCMProfileServiceTest, SendWhenNotSignedIn) { + consumer()->CreateGCMProfileServiceInstance(); + GCMClient::OutgoingMessage message; message.id = "1"; message.data["key1"] = "value1"; @@ -603,6 +701,34 @@ TEST_F(GCMProfileServiceTest, SendWhenNotSignedIn) { EXPECT_EQ(GCMClient::NOT_SIGNED_IN, consumer()->send_result()); } +TEST_F(GCMProfileServiceTest, SendUnderNeutralChannelSignal) { + // Neutral channel signal will prevent GCMClient from checking in when the + // profile is signed in. + profile()->GetPrefs()->ClearPref(prefs::kGCMChannelEnabled); + + consumer()->CreateGCMProfileServiceInstance(); + consumer()->SignIn(kTestingUsername); + + // GCMClient should not be checked in. + EXPECT_FALSE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::UNINITIALIZED, consumer()->GetGCMClient()->status()); + + // Invoking send will make GCMClient checked in. + GCMClient::OutgoingMessage message; + message.id = "1"; + message.data["key1"] = "value1"; + consumer()->Send(kTestingAppId, kUserId, message); + WaitUntilCompleted(); + + // GCMClient should be checked in. + EXPECT_TRUE(consumer()->IsGCMClientReady()); + EXPECT_EQ(GCMClientMock::LOADED, consumer()->GetGCMClient()->status()); + + // Sending should succeed. + EXPECT_EQ(consumer()->send_message_id(), message.id); + EXPECT_EQ(GCMClient::SUCCESS, consumer()->send_result()); +} + // Tests single-profile. class GCMProfileServiceSingleProfileTest : public GCMProfileServiceTest { public: @@ -620,29 +746,6 @@ class GCMProfileServiceSingleProfileTest : public GCMProfileServiceTest { } }; -TEST_F(GCMProfileServiceSingleProfileTest, SignOut) { - EXPECT_TRUE(consumer()->IsGCMClientReady()); - - // This will trigger check-out. - consumer()->SignOut(); - - EXPECT_FALSE(consumer()->IsGCMClientReady()); -} - -TEST_F(GCMProfileServiceSingleProfileTest, SignOutAndThenSignIn) { - EXPECT_TRUE(consumer()->IsGCMClientReady()); - - // This will trigger check-out. - consumer()->SignOut(); - - EXPECT_FALSE(consumer()->IsGCMClientReady()); - - // Sign-in with a different username. - consumer()->SignIn(kTestingUsername2); - - EXPECT_TRUE(consumer()->IsGCMClientReady()); -} - TEST_F(GCMProfileServiceSingleProfileTest, Register) { std::vector<std::string> sender_ids; sender_ids.push_back("sender1"); @@ -766,6 +869,7 @@ TEST_F(GCMProfileServiceSingleProfileTest, ReadRegistrationFromStateStore) { // This should read the registration info from the extension's state store. consumer()->Register(extension->id(), sender_ids); + PumpIOLoop(); PumpUILoop(); EXPECT_EQ(old_registration_id, consumer()->registration_id()); EXPECT_EQ(GCMClient::SUCCESS, consumer()->registration_result()); @@ -788,10 +892,8 @@ TEST_F(GCMProfileServiceSingleProfileTest, // preparation to call register 2nd time. consumer()->clear_registration_result(); - // Needs to create a GCMClient instance that is not ready initiallly. - consumer()->set_gcm_client_initial_status(GCMClientMock::NOT_READY); - // Simulate start-up by recreating GCMProfileService. + consumer()->set_gcm_client_loading_delay(GCMClientMock::DELAY_LOADING); consumer()->CreateGCMProfileServiceInstance(); // Simulate start-up by reloading extension. @@ -805,7 +907,7 @@ TEST_F(GCMProfileServiceSingleProfileTest, EXPECT_EQ(GCMClient::UNKNOWN_ERROR, consumer()->registration_result()); // Register operation will be invoked after GCMClient becomes ready. - consumer()->GetGCMClient()->SetReady(); + consumer()->GetGCMClient()->PerformDelayedLoading(); WaitUntilCompleted(); EXPECT_EQ(old_registration_id, consumer()->registration_id()); EXPECT_EQ(GCMClient::SUCCESS, consumer()->registration_result()); diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index 09090fd..cfe8726 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -1082,6 +1082,8 @@ void ProfileSyncService::OnExperimentsChanged( experiments.gcm_channel_state == syncer::Experiments::ENABLED); gcm::GCMProfileServiceFactory::GetForProfile(profile()); + } else { + profile()->GetPrefs()->ClearPref(prefs::kGCMChannelEnabled); } if (experiments.enhanced_bookmarks_enabled) { diff --git a/chrome/browser/sync/profile_sync_service_factory.cc b/chrome/browser/sync/profile_sync_service_factory.cc index 6e2f869..421fa01 100644 --- a/chrome/browser/sync/profile_sync_service_factory.cc +++ b/chrome/browser/sync/profile_sync_service_factory.cc @@ -95,16 +95,10 @@ BrowserContextKeyedService* ProfileSyncServiceFactory::BuildServiceInstanceFor( SigninManagerBase* signin = SigninManagerFactory::GetForProfile(profile); - // Automatically load the GCMProfileService if the enabled state has been - // explicitly set. - const base::Value* gcm_enabled_value = - profile->GetPrefs()->GetUserPrefValue(prefs::kGCMChannelEnabled); - bool gcm_enabled = false; - if (gcm_enabled_value && - gcm_enabled_value->GetAsBoolean(&gcm_enabled) && - gcm_enabled) { - gcm::GCMProfileServiceFactory::GetForProfile(profile); - } + // Always create the GCMProfileService instance such that we can listen to + // the profile notifications and purge the GCM store when the profile is + // being signed out. + gcm::GCMProfileServiceFactory::GetForProfile(profile); // TODO(atwilson): Change AboutSigninInternalsFactory to load on startup // once http://crbug.com/171406 has been fixed. diff --git a/google_apis/gcm/engine/mcs_client.cc b/google_apis/gcm/engine/mcs_client.cc index adae939..dbfa7ff 100644 --- a/google_apis/gcm/engine/mcs_client.cc +++ b/google_apis/gcm/engine/mcs_client.cc @@ -335,11 +335,6 @@ void MCSClient::SendMessage(const MCSMessage& message) { MaybeSendMessage(); } -void MCSClient::Destroy() { - gcm_store_->Destroy(base::Bind(&MCSClient::OnGCMUpdateFinished, - weak_ptr_factory_.GetWeakPtr())); -} - void MCSClient::ResetStateAndBuildLoginRequest( mcs_proto::LoginRequest* request) { DCHECK(android_id_); diff --git a/google_apis/gcm/engine/mcs_client.h b/google_apis/gcm/engine/mcs_client.h index a81643b..f254566 100644 --- a/google_apis/gcm/engine/mcs_client.h +++ b/google_apis/gcm/engine/mcs_client.h @@ -122,13 +122,6 @@ class GCM_EXPORT MCSClient { // |message_sent_callback_| is invoked with a TTL expiration error. virtual void SendMessage(const MCSMessage& message); - // Disconnects the client and permanently destroys the persistent GCM store. - // WARNING: This is permanent, and the client must be recreated with new - // credentials afterwards. - // TODO(jianli): destroying the persistent GCM store should be moved to - // GCMClient. - void Destroy(); - // Returns the current state of the client. State state() const { return state_; } diff --git a/google_apis/gcm/gcm_client.h b/google_apis/gcm/gcm_client.h index 8bb616d..962486f 100644 --- a/google_apis/gcm/gcm_client.h +++ b/google_apis/gcm/gcm_client.h @@ -132,7 +132,8 @@ class GCM_EXPORT GCMClient { GCMClient(); virtual ~GCMClient(); - // Begins initialization of the GCM Client. + // Begins initialization of the GCM Client. This will not trigger a + // connection. // |chrome_build_proto|: chrome info, i.e., version, channel and etc. // |store_path|: path to the GCM store. // |blocking_task_runner|: for running blocking file tasks. @@ -147,6 +148,10 @@ class GCM_EXPORT GCMClient { url_request_context_getter, Delegate* delegate) = 0; + // Loads the data from the persistent store. This will automatically kick off + // the check-in if the check-in info is not found in the store. + virtual void Load() = 0; + // Checks out of the GCM service. This will erase all the cached and persisted // data. virtual void CheckOut() = 0; @@ -176,9 +181,6 @@ class GCM_EXPORT GCMClient { virtual void Send(const std::string& app_id, const std::string& receiver_id, const OutgoingMessage& message) = 0; - - // Returns true if GCM becomes ready. - virtual bool IsReady() const = 0; }; } // namespace gcm diff --git a/google_apis/gcm/gcm_client_impl.cc b/google_apis/gcm/gcm_client_impl.cc index 491599c..68e5c1f 100644 --- a/google_apis/gcm/gcm_client_impl.cc +++ b/google_apis/gcm/gcm_client_impl.cc @@ -5,10 +5,12 @@ #include "google_apis/gcm/gcm_client_impl.h" #include "base/bind.h" +#include "base/command_line.h" #include "base/files/file_path.h" #include "base/logging.h" #include "base/memory/scoped_ptr.h" #include "base/message_loop/message_loop.h" +#include "base/metrics/histogram.h" #include "base/sequenced_task_runner.h" #include "base/time/default_clock.h" #include "google_apis/gcm/base/mcs_message.h" @@ -137,31 +139,26 @@ void GCMClientImpl::Initialize( chrome_build_proto_.CopyFrom(chrome_build_proto); url_request_context_getter_ = url_request_context_getter; - gcm_store_.reset(new GCMStoreImpl(false, path, blocking_task_runner)); - gcm_store_->Load(base::Bind(&GCMClientImpl::OnLoadCompleted, - weak_ptr_factory_.GetWeakPtr())); + bool use_mock_keychain = false; +#if defined(OS_MACOSX) + if (CommandLine::ForCurrentProcess()->HasSwitch("use-mock-keychain")) + use_mock_keychain = true; +#endif + + gcm_store_.reset( + new GCMStoreImpl(use_mock_keychain, path, blocking_task_runner)); delegate_ = delegate; - // |mcs_client_| might already be set for testing at this point. No need to - // create a |connection_factory_|. - if (!mcs_client_.get()) { - const net::HttpNetworkSession::Params* network_session_params = - url_request_context_getter->GetURLRequestContext()-> - GetNetworkSessionParams(); - DCHECK(network_session_params); - network_session_ = new net::HttpNetworkSession(*network_session_params); - connection_factory_.reset(new ConnectionFactoryImpl( - GURL(kMCSEndpoint), - kDefaultBackoffPolicy, - network_session_, - net_log_.net_log())); - mcs_client_.reset(new MCSClient(chrome_build_proto.chrome_version(), - clock_.get(), - connection_factory_.get(), - gcm_store_.get())); - } + state_ = INITIALIZED; +} +void GCMClientImpl::Load() { + DCHECK_EQ(INITIALIZED, state_); + + // Once the loading is completed, the check-in will be initiated. + gcm_store_->Load(base::Bind(&GCMClientImpl::OnLoadCompleted, + weak_ptr_factory_.GetWeakPtr())); state_ = LOADING; } @@ -188,6 +185,25 @@ void GCMClientImpl::OnLoadCompleted(scoped_ptr<GCMStore::LoadResult> result) { void GCMClientImpl::InitializeMCSClient( scoped_ptr<GCMStore::LoadResult> result) { + // |mcs_client_| might already be set for testing at this point. No need to + // create a |connection_factory_|. + if (!mcs_client_.get()) { + const net::HttpNetworkSession::Params* network_session_params = + url_request_context_getter_->GetURLRequestContext()-> + GetNetworkSessionParams(); + DCHECK(network_session_params); + network_session_ = new net::HttpNetworkSession(*network_session_params); + connection_factory_.reset(new ConnectionFactoryImpl( + GURL(kMCSEndpoint), + kDefaultBackoffPolicy, + network_session_, + net_log_.net_log())); + mcs_client_.reset(new MCSClient(chrome_build_proto_.chrome_version(), + clock_.get(), + connection_factory_.get(), + gcm_store_.get())); + } + mcs_client_->Initialize( base::Bind(&GCMClientImpl::OnMCSError, weak_ptr_factory_.GetWeakPtr()), base::Bind(&GCMClientImpl::OnMessageReceivedFromMCS, @@ -276,12 +292,13 @@ void GCMClientImpl::SetDeviceCredentialsCallback(bool success) { } void GCMClientImpl::CheckOut() { - delegate_ = NULL; device_checkin_info_.Reset(); - mcs_client_->Destroy(); // This will also destroy GCM store. mcs_client_.reset(); + gcm_store_->Destroy(base::Bind(&GCMClientImpl::OnGCMStoreDestroyed, + weak_ptr_factory_.GetWeakPtr())); checkin_request_.reset(); pending_registrations_.clear(); + state_ = INITIALIZED; } void GCMClientImpl::Register(const std::string& app_id, @@ -368,6 +385,11 @@ void GCMClientImpl::OnUnregisterCompleted(const std::string& app_id, pending_unregistrations_.erase(iter); } +void GCMClientImpl::OnGCMStoreDestroyed(bool success) { + DLOG_IF(ERROR, !success) << "GCM store failed to be destroyed!"; + UMA_HISTOGRAM_BOOLEAN("GCM.StoreDestroySucceeded", success); +} + void GCMClientImpl::Send(const std::string& app_id, const std::string& receiver_id, const OutgoingMessage& message) { @@ -395,10 +417,6 @@ void GCMClientImpl::Send(const std::string& app_id, mcs_client_->SendMessage(mcs_message); } -bool GCMClientImpl::IsReady() const { - return state_ == READY; -} - void GCMClientImpl::OnMessageReceivedFromMCS(const gcm::MCSMessage& message) { switch (message.tag()) { case kLoginResponseTag: diff --git a/google_apis/gcm/gcm_client_impl.h b/google_apis/gcm/gcm_client_impl.h index 40b90f2..6feedbf 100644 --- a/google_apis/gcm/gcm_client_impl.h +++ b/google_apis/gcm/gcm_client_impl.h @@ -54,6 +54,7 @@ class GCM_EXPORT GCMClientImpl : public GCMClient { const scoped_refptr<net::URLRequestContextGetter>& url_request_context_getter, Delegate* delegate) OVERRIDE; + virtual void Load() OVERRIDE; virtual void CheckOut() OVERRIDE; virtual void Register(const std::string& app_id, const std::string& cert, @@ -62,13 +63,14 @@ class GCM_EXPORT GCMClientImpl : public GCMClient { virtual void Send(const std::string& app_id, const std::string& receiver_id, const OutgoingMessage& message) OVERRIDE; - virtual bool IsReady() const OVERRIDE; private: // State representation of the GCMClient. enum State { // Uninitialized. UNINITIALIZED, + // Initialized, + INITIALIZED, // GCM store loading is in progress. LOADING, // Initial device checkin is in progress. @@ -149,6 +151,9 @@ class GCM_EXPORT GCMClientImpl : public GCMClient { // Completes the unregistration request. void OnUnregisterCompleted(const std::string& app_id, bool status); + // Completes the GCM store destroy request. + void OnGCMStoreDestroyed(bool success); + // Handles incoming data message and dispatches it the a relevant user // delegate. void HandleIncomingMessage(const gcm::MCSMessage& message); diff --git a/google_apis/gcm/gcm_client_impl_unittest.cc b/google_apis/gcm/gcm_client_impl_unittest.cc index f2364f4..a78e1a6 100644 --- a/google_apis/gcm/gcm_client_impl_unittest.cc +++ b/google_apis/gcm/gcm_client_impl_unittest.cc @@ -283,10 +283,15 @@ void GCMClientImplTest::InitializeGCMClient() { message_loop_.message_loop_proxy(), url_request_context_getter_, this); + #if defined(OS_MACOSX) // On OSX, prevent the Keychain permissions popup during unit tests. Encryptor::UseMockKeychain(true); // Must be after Initialize. #endif + + // Starting loading and check-in. + gcm_client_->Load(); + // Ensuring that mcs_client is using the same gcm_store as gcm_client. mcs_client()->set_gcm_store(gcm_client_->gcm_store_.get()); PumpLoopUntilIdle(); diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 5b98b10..3b3047c 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -5588,6 +5588,14 @@ other types of suffix sets. </summary> </histogram> +<histogram name="GCM.StoreDestroySucceeded" enum="BooleanSuccess"> + <summary> + Success indicates successfully destroying the GCM persistent store. Failure + indicates a failure destroying the persistence store. GCM store will be + destroyed when the profile has been signed out. + </summary> +</histogram> + <histogram name="GCM.StoreLoadSucceeded" enum="BooleanSuccess"> <summary> Success indicates successfully loading an initialized persistent GCM store |