Chrome To Mobile Service refactoring and polish, etc.

These cleanup changes were pulled out from:
http://codereview.chromium.org/10834203

Use the cloud print device displayName instead of name.
(these are editable names shown on the cloud print dash)
Call NOTREACHED on any task posting failure.
Extract UpdateCommandState utility function.
Rename & refactor RefreshAccessToken->RequestAccessToken.
Rename RequestSearch->RequestDeviceSearch.
Make HasMobiles const; consolidate access_token_ DCHECKs.
Misc comment updates.

BUG=102709
TEST=No Chrome To Mobile regressions, device display names reflected.

Review URL: https://chromiumcodereview.appspot.com/10828353

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@151995 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/chrome_to_mobile_service.cc

@@ -140,10 +140,12 @@ typedef base::Callback<void(const FilePath& path, bool success)>
 // Create a temp file and post the callback on the UI thread with the results.
 // Call this as a BlockingPoolTask to avoid the FILE thread.
 void CreateSnapshotFile(CreateSnapshotFileCallback callback) {
-  FilePath snapshot;
-  bool success = file_util::CreateTemporaryFile(&snapshot);
-  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
-                                   base::Bind(callback, snapshot, success));
+  FilePath file;
+  bool success = file_util::CreateTemporaryFile(&file);
+  if (!content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                        base::Bind(callback, file, success))) {
+    NOTREACHED();
+  }
 }
 
 // Delete the snapshot file; DCHECK, but really ignore the result of the delete.
@@ -187,14 +189,14 @@ ChromeToMobileService::ChromeToMobileService(Profile* profile)
       profile_(profile),
       cloud_print_url_(new CloudPrintURL(profile)),
       cloud_print_accessible_(false) {
-  // Skip initialization if constructed without a profile.
+  // TODO(msw): Fix GMock tests, which lack profiles (http://crbug.com/122183).
   if (profile_) {
     // Get an access token as soon as the Gaia login refresh token is available.
     TokenService* service = TokenServiceFactory::GetForProfile(profile_);
     registrar_.Add(this, chrome::NOTIFICATION_TOKEN_AVAILABLE,
                    content::Source<TokenService>(service));
     if (service->HasOAuthLoginToken())
-      RefreshAccessToken();
+      RequestAccessToken();
   }
 }
 
@@ -203,7 +205,7 @@ ChromeToMobileService::~ChromeToMobileService() {
     DeleteSnapshot(*snapshots_.begin());
 }
 
-bool ChromeToMobileService::HasMobiles() {
+bool ChromeToMobileService::HasMobiles() const {
   return !GetMobiles()->empty();
 }
 
@@ -213,9 +215,9 @@ const base::ListValue* ChromeToMobileService::GetMobiles() const {
 
 void ChromeToMobileService::RequestMobileListUpdate() {
   if (access_token_.empty())
-    RefreshAccessToken();
+    RequestAccessToken();
   else if (cloud_print_accessible_)
-    RequestSearch();
+    RequestDeviceSearch();
 }
 
 void ChromeToMobileService::GenerateSnapshot(Browser* browser,
@@ -226,8 +228,10 @@ void ChromeToMobileService::GenerateSnapshot(Browser* browser,
                  weak_ptr_factory_.GetWeakPtr(), observer,
                  browser->session_id().id());
   // Create a temporary file via the blocking pool for snapshot storage.
-  content::BrowserThread::PostBlockingPoolTask(FROM_HERE,
-      base::Bind(&CreateSnapshotFile, callback));
+  if (!content::BrowserThread::PostBlockingPoolTask(FROM_HERE,
+          base::Bind(&CreateSnapshotFile, callback))) {
+    NOTREACHED();
+  }
 }
 
 void ChromeToMobileService::SendToMobile(const base::DictionaryValue& mobile,
@@ -236,7 +240,6 @@ void ChromeToMobileService::SendToMobile(const base::DictionaryValue& mobile,
                                          base::WeakPtr<Observer> observer) {
   LogMetric(SENDING_URL);
 
-  DCHECK(!access_token_.empty());
   JobData data;
   std::string mobile_os;
   if (!mobile.GetString("type", &mobile_os))
@@ -262,20 +265,26 @@ void ChromeToMobileService::SendToMobile(const base::DictionaryValue& mobile,
     data.type = SNAPSHOT;
     net::URLFetcher* submit_snapshot = CreateRequest(data);
     request_observer_map_[submit_snapshot] = observer;
-    content::BrowserThread::PostBlockingPoolSequencedTask(
-        data.snapshot.AsUTF8Unsafe(), FROM_HERE,
-        base::Bind(&ChromeToMobileService::SendRequest,
-                   weak_ptr_factory_.GetWeakPtr(), submit_snapshot, data));
+    if (!content::BrowserThread::PostBlockingPoolSequencedTask(
+            data.snapshot.AsUTF8Unsafe(), FROM_HERE,
+            base::Bind(&ChromeToMobileService::SendRequest,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       submit_snapshot, data))) {
+      NOTREACHED();
+    }
   }
 }
 
 void ChromeToMobileService::DeleteSnapshot(const FilePath& snapshot) {
   DCHECK(snapshot.empty() || snapshots_.find(snapshot) != snapshots_.end());
   if (snapshots_.find(snapshot) != snapshots_.end()) {
-    if (!snapshot.empty())
-      content::BrowserThread::PostBlockingPoolSequencedTask(
-          snapshot.AsUTF8Unsafe(), FROM_HERE,
-          base::Bind(&DeleteSnapshotFile, snapshot));
+    if (!snapshot.empty()) {
+      if (!content::BrowserThread::PostBlockingPoolSequencedTask(
+              snapshot.AsUTF8Unsafe(), FROM_HERE,
+              base::Bind(&DeleteSnapshotFile, snapshot))) {
+        NOTREACHED();
+      }
+    }
     snapshots_.erase(snapshot);
   }
 }
@@ -310,7 +319,7 @@ void ChromeToMobileService::Observe(
   TokenService::TokenAvailableDetails* token_details =
       content::Details<TokenService::TokenAvailableDetails>(details).ptr();
   if (token_details->service() == GaiaConstants::kGaiaOAuth2LoginRefreshToken)
-    RefreshAccessToken();
+    RequestAccessToken();
 }
 
 void ChromeToMobileService::OnGetTokenSuccess(
@@ -329,7 +338,19 @@ void ChromeToMobileService::OnGetTokenFailure(
   auth_retry_timer_.Stop();
   auth_retry_timer_.Start(
       FROM_HERE, base::TimeDelta::FromHours(kAuthRetryDelayHours),
-      this, &ChromeToMobileService::RefreshAccessToken);
+      this, &ChromeToMobileService::RequestAccessToken);
+}
+
+void ChromeToMobileService::UpdateCommandState() const {
+  // Ensure the feature is not disabled by commandline options.
+  DCHECK(IsChromeToMobileEnabled());
+  const bool has_mobiles = HasMobiles();
+  for (BrowserList::const_iterator i = BrowserList::begin();
+       i != BrowserList::end(); ++i) {
+    Browser* browser = *i;
+    if (browser->profile() == profile_)
+      browser->command_controller()->SendToMobileStateChanged(has_mobiles);
+  }
 }
 
 void ChromeToMobileService::SnapshotFileCreated(
@@ -363,6 +384,7 @@ net::URLFetcher* ChromeToMobileService::CreateRequest(const JobData& data) {
 void ChromeToMobileService::InitRequest(net::URLFetcher* request) {
   request->SetRequestContext(profile_->GetRequestContext());
   request->SetMaxRetries(kMaxRetries);
+  DCHECK(!access_token_.empty());
   request->SetExtraRequestHeaders("Authorization: OAuth " +
       access_token_ + "\r\n" + cloud_print::kChromeCloudPrintProxyHeader);
   request->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
@@ -412,22 +434,20 @@ void ChromeToMobileService::SendRequest(net::URLFetcher* request,
   request->Start();
 }
 
-void ChromeToMobileService::RefreshAccessToken() {
-  if (access_token_fetcher_.get())
-    return;
-
-  std::string token = TokenServiceFactory::GetForProfile(profile_)->
-                          GetOAuth2LoginRefreshToken();
-  if (token.empty())
+void ChromeToMobileService::RequestAccessToken() {
+  // Deny concurrent requests and bail without a valid Gaia login refresh token.
+  TokenService* token_service = TokenServiceFactory::GetForProfile(profile_);
+  if (access_token_fetcher_.get() || !token_service->HasOAuthLoginToken())
     return;
 
   auth_retry_timer_.Stop();
   access_token_fetcher_.reset(
       new OAuth2AccessTokenFetcher(this, profile_->GetRequestContext()));
-  std::vector<std::string> scopes(1, kCloudPrintAuth);
   GaiaUrls* gaia_urls = GaiaUrls::GetInstance();
   access_token_fetcher_->Start(gaia_urls->oauth2_chrome_client_id(),
-      gaia_urls->oauth2_chrome_client_secret(), token, scopes);
+                               gaia_urls->oauth2_chrome_client_secret(),
+                               token_service->GetOAuth2LoginRefreshToken(),
+                               std::vector<std::string>(1, kCloudPrintAuth));
 }
 
 void ChromeToMobileService::RequestAccountInfo() {
@@ -458,9 +478,7 @@ void ChromeToMobileService::RequestAccountInfo() {
   account_info_request_->Start();
 }
 
-void ChromeToMobileService::RequestSearch() {
-  DCHECK(!access_token_.empty());
-
+void ChromeToMobileService::RequestDeviceSearch() {
   // Deny requests if cloud print is inaccessible, and deny concurrent requests.
   if (!cloud_print_accessible_ || search_request_.get())
     return;
@@ -524,7 +542,7 @@ void ChromeToMobileService::HandleSearchResponse() {
           printer->GetString("type", &type) &&
           (type.compare(kTypeAndroid) == 0 || type.compare(kTypeIOS) == 0)) {
         // Copy just the requisite values from the full |printer| definition.
-        if (printer->GetString("name", &name) &&
+        if (printer->GetString("displayName", &name) &&
             printer->GetString("id", &id)) {
           mobile = new DictionaryValue();
           mobile->SetString("type", type);
@@ -543,16 +561,9 @@ void ChromeToMobileService::HandleSearchResponse() {
     prefs->SetInt64(prefs::kChromeToMobileTimestamp,
                     base::TimeTicks::Now().ToInternalValue());
 
-    const bool has_mobiles = HasMobiles();
-    if (has_mobiles)
+    if (HasMobiles())
       LogMetric(DEVICES_AVAILABLE);
-
-    for (BrowserList::const_iterator i = BrowserList::begin();
-         i != BrowserList::end(); ++i) {
-      Browser* browser = *i;
-      if (browser->profile() == profile_)
-        browser->command_controller()->SendToMobileStateChanged(has_mobiles);
-    }
+    UpdateCommandState();
   }
 }
 

chrome/browser/chrome_to_mobile_service.h

@@ -11,7 +11,6 @@
 #include <vector>
 
 #include "base/file_path.h"
-#include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "base/string16.h"
 #include "base/timer.h"
@@ -105,7 +104,7 @@ class ChromeToMobileService : public ProfileKeyedService,
   virtual ~ChromeToMobileService();
 
   // Returns true if the service has found any registered mobile devices.
-  bool HasMobiles();
+  bool HasMobiles() const;
 
   // Get the non-NULL ListValue of mobile devices from the cloud print service.
   // The list is owned by PrefService, which outlives ChromeToMobileService.
@@ -156,6 +155,10 @@ class ChromeToMobileService : public ProfileKeyedService,
  private:
   friend class MockChromeToMobileService;
 
+  // Enable or disable Chrome To Mobile with the browsers' command controllers.
+  // The feature state is automatically derived from internal conditions.
+  void UpdateCommandState() const;
+
   // Handle the attempted creation of a temporary file for snapshot generation.
   // Alert the observer of failure or generate MHTML with an observer callback.
   void SnapshotFileCreated(base::WeakPtr<Observer> observer,
@@ -166,7 +169,7 @@ class ChromeToMobileService : public ProfileKeyedService,
   // Create a cloud print job submission request for a URL or snapshot.
   net::URLFetcher* CreateRequest(const JobData& data);
 
-  // Initialize URLFetcher requests (search and jobs submit).
+  // Initialize cloud print URLFetcher requests.
   void InitRequest(net::URLFetcher* request);
 
   // Submit a cloud print job request with the requisite data.
@@ -174,13 +177,13 @@ class ChromeToMobileService : public ProfileKeyedService,
 
   // Send the OAuth2AccessTokenFetcher request.
   // Virtual for unit test mocking.
-  virtual void RefreshAccessToken();
+  virtual void RequestAccessToken();
 
   // Request account information to limit cloud print access to existing users.
   void RequestAccountInfo();
 
-  // Send the cloud print URLFetcher search request.
-  void RequestSearch();
+  // Send the cloud print URLFetcher device search request.
+  void RequestDeviceSearch();
 
   void HandleAccountInfoResponse();
   void HandleSearchResponse();

chrome/browser/chrome_to_mobile_service_unittest.cc

@@ -17,13 +17,13 @@ const char kDummyString[] = "dummy";
 
 class DummyNotificationSource {};
 
-// A mock ChromeToMobileService with a mocked out RefreshAccessToken method.
+// A mock ChromeToMobileService with a mocked out RequestAccessToken method.
 class MockChromeToMobileService : public ChromeToMobileService {
  public:
   MockChromeToMobileService();
   ~MockChromeToMobileService();
 
-  MOCK_METHOD0(RefreshAccessToken, void());
+  MOCK_METHOD0(RequestAccessToken, void());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockChromeToMobileService);
@@ -50,9 +50,9 @@ ChromeToMobileServiceTest::ChromeToMobileServiceTest() {}
 
 ChromeToMobileServiceTest::~ChromeToMobileServiceTest() {}
 
-// Ensure that RefreshAccessToken is not called for irrelevant notifications.
+// Ensure that RequestAccessToken is not called for irrelevant notifications.
 TEST_F(ChromeToMobileServiceTest, IgnoreIrrelevantNotifications) {
-  EXPECT_CALL(service_, RefreshAccessToken()).Times(0);
+  EXPECT_CALL(service_, RequestAccessToken()).Times(0);
 
   // Send dummy service/token details (should not refresh token).
   DummyNotificationSource dummy_source;
@@ -62,9 +62,9 @@ TEST_F(ChromeToMobileServiceTest, IgnoreIrrelevantNotifications) {
       content::Details<TokenService::TokenAvailableDetails>(&dummy_details));
 }
 
-// Ensure that RefreshAccessToken is called on the proper notification.
+// Ensure that RequestAccessToken is called on the proper notification.
 TEST_F(ChromeToMobileServiceTest, AuthenticateOnTokenAvailable) {
-  EXPECT_CALL(service_, RefreshAccessToken()).Times(1);
+  EXPECT_CALL(service_, RequestAccessToken()).Times(1);
 
   // Send a Gaia OAuth2 Login service dummy token (should refresh token).
   DummyNotificationSource dummy_source;