[GCM] Make sure GCM checkout logic is invoked when the profile is signed out

chrome/browser/services/gcm/gcm_profile_service.cc

Index: chrome/browser/services/gcm/gcm_profile_service.cc
diff --git a/chrome/browser/services/gcm/gcm_profile_service.cc b/chrome/browser/services/gcm/gcm_profile_service.cc
index b58f7e64029eb3a789dc86920aba86ba043de21c..9f9945955215b8cb34a888f8f71caae13fa83455 100644
--- a/chrome/browser/services/gcm/gcm_profile_service.cc
+++ b/chrome/browser/services/gcm/gcm_profile_service.cc
@@ -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 CheckIn();
   void CheckOut();
   void Register(const std::string& app_id,
                 const std::vector<std::string>& sender_ids,
@@ -269,6 +270,9 @@ 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();
@@ -288,7 +292,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) {
@@ -317,9 +321,7 @@ void GCMProfileService::IOWorker::Initialize(
   content::BrowserThread::PostTask(
       content::BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&GCMProfileService::FinishInitializationOnUI,
-                 service_,
-                 gcm_client_->IsReady()));
+      base::Bind(&GCMProfileService::FinishInitializationOnUI, service_));
 }
 
 void GCMProfileService::IOWorker::Reset() {
@@ -417,11 +419,16 @@ void GCMProfileService::IOWorker::OnGCMReady() {
                  service_));
 }
 
+void GCMProfileService::IOWorker::CheckIn() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+
+  gcm_client_->CheckIn();
+}
+
 void GCMProfileService::IOWorker::CheckOut() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
 
   gcm_client_->CheckOut();
-  gcm_client_.reset();
 }
 
 void GCMProfileService::IOWorker::Register(
@@ -505,15 +512,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 +526,19 @@ 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

[Nicolas Zea, 2014/02/21 18:43:33]

Initializing the client will result in connecting to MCS if checkin credentials
were already there. This means the kill switch won't have an effect anymore.

I think we can't perform initialization unless GCMChannel enabled returns true
or it's unset and an app attempts to perform an action.

[jianli, 2014/02/21 20:24:06]

 I have moved the GCMStore loading logic out of GCMClientImpl::Initialize. Now
when GCMClient is initialized, we only create GCMStore and MCSClient. We do not
trigger store loading at this time.

+  // GCM check-in.
+  io_worker_ = new IOWorker(weak_ptr_factory_.GetWeakPtr());
+  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));
 }
 
 void GCMProfileService::Register(const std::string& app_id,
@@ -545,6 +548,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.
+  EnsureCheckedIn();
+
   // If the profile was not signed in, bail out.
   if (username_.empty()) {
     callback.Run(std::string(), GCMClient::NOT_SIGNED_IN);
@@ -630,6 +636,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.
+  EnsureCheckedIn();
+
   // If the profile was not signed in, bail out.
   if (username_.empty()) {
     callback.Run(std::string(), GCMClient::NOT_SIGNED_IN);
@@ -674,6 +683,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 +694,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 (IsGCMChannelEnabled())
+        EnsureCheckedIn();
       break;
     }
     case chrome::NOTIFICATION_GOOGLE_SIGNED_OUT:
@@ -694,44 +704,66 @@ 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());
+bool GCMProfileService::IsGCMChannelEnabled() const {
+  const base::Value* gcm_enabled_value =
+      profile_->GetPrefs()->GetUserPrefValue(prefs::kGCMChannelEnabled);
+  bool gcm_enabled = false;
+  return gcm_enabled_value &&
+         gcm_enabled_value->GetAsBoolean(&gcm_enabled) &&
+         gcm_enabled;
+}
+
+void GCMProfileService::EnsureCheckedIn() {
+  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();
+  // Initiate the GCM check-in.
   content::BrowserThread::PostTask(
       content::BrowserThread::IO,
       FROM_HERE,
-      base::Bind(&GCMProfileService::IOWorker::Initialize,
-                 io_worker_,
-                 gcm_client_factory_.get(),
-                 profile_->GetPath().Append(chrome::kGCMStoreDirname),
-                 url_request_context_getter));
+      base::Bind(&GCMProfileService::IOWorker::CheckIn, io_worker_));
 }
 
 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 persisted data from app's state store.
@@ -742,7 +774,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,12 +919,12 @@ void GCMProfileService::MessageSendError(const std::string& app_id,
   GetEventRouter(app_id)->OnSendError(app_id, message_id, result);
 }
 
-void GCMProfileService::FinishInitializationOnUI(bool ready) {
+void GCMProfileService::FinishInitializationOnUI() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  gcm_client_ready_ = ready;
-  if (gcm_client_ready_)
-    delayed_task_controller_->SetGCMReady();
+  // Initiates the check-in if the rollout signal indicates yes.
+  if (IsGCMChannelEnabled())
+    EnsureCheckedIn();
 }
 
 void GCMProfileService::GCMClientReady() {