Integrate invalidation API into ChromeToMobileService.

chrome/browser/chrome_to_mobile_service.cc

Index: chrome/browser/chrome_to_mobile_service.cc
diff --git a/chrome/browser/chrome_to_mobile_service.cc b/chrome/browser/chrome_to_mobile_service.cc
index fb93a50c9b8e128205084163f81448a51080144a..a08fd81a1644e45163c90c9451293a3ed350ddda 100644
--- a/chrome/browser/chrome_to_mobile_service.cc
+++ b/chrome/browser/chrome_to_mobile_service.cc
@@ -20,6 +20,8 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/token_service.h"
 #include "chrome/browser/signin/token_service_factory.h"
+#include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_command_controller.h"
 #include "chrome/browser/ui/browser_finder.h"
@@ -38,6 +40,8 @@
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
+#include "google/cacheinvalidation/include/types.h"
+#include "google/cacheinvalidation/types.pb.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_fetcher.h"
@@ -59,6 +63,11 @@ const size_t kAuthRetryDelayHours = 6;
 // Note that this limitation does not hold across application restarts.
 const int kSearchRequestDelayHours = 24;
 
+// TODO(msw): Push back for enum values, instead of these string contstants?
+// The invalidation object ID for the User space, Chrome to Mobile, Mobile LiST.
+// ("U" == "User", "CM" == "Chrome to Mobile", "MLST" == "Mobile LiST").
+const char kMobileListObjectId[] = "UCMMLST";

[akalin, 2012/08/08 23:06:27]

i think object id names have to be strings

[msw, 2012/08/10 22:05:05]

I've voiced my dissent, but will go along :)
"It won't be my problem down the road..." - msw

[akalin, 2012/08/10 23:03:22]

I understand you don't like strings, but I don't know of any alternatives.  The
object ID names are part of the ticl protocol, right?

[nyquist, 2012/08/10 23:46:39]

The object ID name must match whatever the source sends, in this case Cloud
Print. There is server-side validation for this, and it is alphanumeric-based.

We should probably have some way of making sure we don't reuse object ID name
space, such as here, "CM" which is the chrome component project ID.

+
 // The cloud print OAuth2 scope and 'printer' type of compatible mobile devices.
 const char kCloudPrintAuth[] = "https://www.googleapis.com/auth/cloudprint";
 const char kTypeAndroid[] = "ANDROID_CHROME_SNAPSHOT";
@@ -178,43 +187,67 @@ bool ChromeToMobileService::IsChromeToMobileEnabled() {
 void ChromeToMobileService::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterListPref(prefs::kChromeToMobileDeviceList,
                           PrefService::UNSYNCABLE_PREF);
-  prefs->RegisterInt64Pref(prefs::kChromeToMobileTimestamp, 0,
-                           PrefService::UNSYNCABLE_PREF);
+  // TODO(msw): Okay to change a pref from Int64 to string?
+  // TODO(msw): Consider renaming timestamp to version? Which is it?
+  prefs->RegisterStringPref(prefs::kChromeToMobileTimestamp, "",

[akalin, 2012/08/08 23:06:27]

just to be clear, this is different from the invalidation timestamp/version?

[msw, 2012/08/10 22:05:05]

Removed. I was changing it to match the last received invalidation
timestamp/version to ensure that we'd only update the device list when truly
needed, but Tommy explained offline that this shouldn't be necessary.

As I understand, OnIncomingNotification should correctly trigger updates from
cloud print without Chrome To Mobile needing to store or check a
timestamp/version. Please let me know if I'm wrong!

+                            PrefService::UNSYNCABLE_PREF);
 }
 
 ChromeToMobileService::ChromeToMobileService(Profile* profile)
     : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
       profile_(profile),
+      mobile_list_id_(ipc::invalidation::ObjectSource::CHROME_COMPONENTS,
+                      kMobileListObjectId),
+      sync_invalidation_enabled_(false),
       cloud_print_url_(new CloudPrintURL(profile)),
       cloud_print_accessible_(false) {
   // Skip initialization if constructed without a profile.
   if (profile_) {

[dcheng, 2012/08/08 23:11:10]

Why do you need to check profile_ here and not other places (line 244). Maybe
just DCHECK(profile) and remove the checks. Same comment on other places that
use profile_.

[msw, 2012/08/10 22:05:05]

It's for the GMock unit tests, which provide a NULL profile to this ctor and
mock out other profile-dependent locations. This isn't ideal, I clarified the
comment as a TODO pointing to http://crbug.com/122183 (for now, will follow up).

     // Get an access token as soon as the Gaia login refresh token is available.
-    TokenService* service = TokenServiceFactory::GetForProfile(profile_);
+    TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
     registrar_.Add(this, chrome::NOTIFICATION_TOKEN_AVAILABLE,
-                   content::Source<TokenService>(service));
-    if (service->HasOAuthLoginToken())
+                   content::Source<TokenService>(token_service));
+    if (token_service->HasOAuthLoginToken())
       RefreshAccessToken();
+
+    ProfileSyncService* profile_sync_service =
+        ProfileSyncServiceFactory::GetForProfile(profile_);
+    // TODO(msw): Update with https://chromiumcodereview.appspot.com/10824161/
+    //profile_sync_service->AddHandler(this);
+    sync_objects_.insert(mobile_list_id_);
+    profile_sync_service->UpdateRegisteredInvalidationIds(this, sync_objects_);
+    // TODO(msw): Check if sync_invalidation_enabled_ directly here?

[akalin, 2012/08/08 23:06:27]

Hmm, I guess you need some way to check if notifications are enabled.

[msw, 2012/08/10 22:05:05]

How about profile_sync_service->IsSyncEnabled()? Not sure if you meant I should
use ShouldPushChanges() for this in http://crbug.com/137086 ?

+    // TODO(msw): Or are we always pinged via OnNotificationsEnabled?
   }
 }
 
 ChromeToMobileService::~ChromeToMobileService() {
   while (!snapshots_.empty())
     DeleteSnapshot(*snapshots_.begin());
+  if (profile_) {
+    // TODO(msw): Update with https://chromiumcodereview.appspot.com/10824161/
+    //ProfileSyncService* profile_sync_service =
+    //    ProfileSyncServiceFactory::GetForProfile(profile_);
+    //profile_sync_service->RemoveHandler(this);
+  }
 }
 
 bool ChromeToMobileService::HasMobiles() {
-  return !GetMobiles()->empty();
+  const base::ListValue* mobiles = GetMobiles();
+  return mobiles && !mobiles->empty();
 }
 
 const base::ListValue* ChromeToMobileService::GetMobiles() const {
+  if (!sync_invalidation_enabled_)
+    return NULL;
+
   return profile_->GetPrefs()->GetList(prefs::kChromeToMobileDeviceList);
 }
 
 void ChromeToMobileService::RequestMobileListUpdate() {
   if (access_token_.empty())
     RefreshAccessToken();
-  else if (cloud_print_accessible_)
+  else if (sync_invalidation_enabled_ && cloud_print_accessible_)
     RequestSearch();
 }
 
@@ -332,6 +365,39 @@ void ChromeToMobileService::OnGetTokenFailure(
       this, &ChromeToMobileService::RefreshAccessToken);
 }
 
+void ChromeToMobileService::OnNotificationsEnabled() {
+  // Only enable Chrome To Mobile if Sync Invalidation Notification are enabled.
+  // Otherwise, the device list may be out of date and result in send failures.
+  // TODO(msw): Load the cached mobile list now (instead of sync_invalidation_enabled_)?
+  sync_invalidation_enabled_ = true;
+}
+
+void ChromeToMobileService::OnNotificationsDisabled(
+    syncer::NotificationsDisabledReason reason) {
+  // Only enable Chrome To Mobile if Sync Invalidation Notification are enabled.
+  // Otherwise, the device list may be out of date and result in send failures.
+  // TODO(msw): Clear the cached mobile list now (instead of sync_invalidation_enabled_)?
+  sync_invalidation_enabled_ = false;
+}
+
+void ChromeToMobileService::OnIncomingNotification(
+    const syncer::ObjectIdPayloadMap& id_payloads,
+    syncer::IncomingNotificationSource source) {
+  DCHECK(sync_invalidation_enabled_);
+  DCHECK_EQ(id_payloads.size(), 1U);
+  DCHECK_EQ(id_payloads.count(mobile_list_id_), 1U);
+  std::string new_timestamp = id_payloads.find(mobile_list_id_)->second;
+
+  PrefService* prefs = profile_->GetPrefs();
+  std::string timestamp = prefs->GetString(prefs::kChromeToMobileTimestamp);
+  // TODO(msw): Check if the timestamp is newer? Or just different?
+  if (!new_timestamp.empty() && timestamp.compare(new_timestamp) != 0) {
+    // TODO(msw): Update the cached timestamp now, or only on successful update?
+    prefs->SetString(prefs::kChromeToMobileTimestamp, new_timestamp);
+    RequestMobileListUpdate();
+  }
+}
+
 void ChromeToMobileService::SnapshotFileCreated(
     base::WeakPtr<Observer> observer,
     SessionID::id_type browser_id,
@@ -445,7 +511,7 @@ void ChromeToMobileService::RequestAccountInfo() {
       CookieSettings::Factory::GetForProfile(profile_);
   if (cookie_settings && !cookie_settings->IsReadingCookieAllowed(url, url)) {
     cloud_print_accessible_ = true;
-    RequestMobileListUpdate();
+    // TODO(msw): Don't poll now... RequestMobileListUpdate();
     return;
   }
 
@@ -460,21 +526,12 @@ void ChromeToMobileService::RequestAccountInfo() {
 
 void ChromeToMobileService::RequestSearch() {
   DCHECK(!access_token_.empty());
+  DCHECK(sync_invalidation_enabled_);
 
   // Deny requests if cloud print is inaccessible, and deny concurrent requests.
   if (!cloud_print_accessible_ || search_request_.get())
     return;
 
-  PrefService* prefs = profile_->GetPrefs();
-  base::TimeTicks previous_search_time = base::TimeTicks::FromInternalValue(
-      prefs->GetInt64(prefs::kChromeToMobileTimestamp));
-
-  // Deny requests before the delay period has passed since the last request.
-  base::TimeDelta elapsed_time = base::TimeTicks::Now() - previous_search_time;
-  if (!previous_search_time.is_null() &&
-      elapsed_time.InHours() < kSearchRequestDelayHours)
-    return;
-
   LogMetric(DEVICES_REQUESTED);
 
   const GURL service_url(cloud_print_url_->GetCloudPrintServiceURL());
@@ -499,7 +556,7 @@ void ChromeToMobileService::HandleAccountInfoResponse() {
       dictionary->GetList(kAccountServicesKey, &services) && services &&
       services->Find(cloud_print_service) != services->end()) {
     cloud_print_accessible_ = true;
-    RequestMobileListUpdate();
+    // TODO(msw): Don't poll now... RequestMobileListUpdate();
   }
 }
 
@@ -537,11 +594,9 @@ void ChromeToMobileService::HandleSearchResponse() {
       }
     }
 
-    // Update the mobile list and timestamp in prefs.
-    PrefService* prefs = profile_->GetPrefs();
-    prefs->Set(prefs::kChromeToMobileDeviceList, mobiles);
-    prefs->SetInt64(prefs::kChromeToMobileTimestamp,
-                    base::TimeTicks::Now().ToInternalValue());
+    // Update the mobile list in prefs.
+    // TODO(msw): Update the timestamp now (only on successful list updates?)
+    profile_->GetPrefs()->Set(prefs::kChromeToMobileDeviceList, mobiles);
 
     const bool has_mobiles = HasMobiles();
     if (has_mobiles)