Add DriveApiUrlGenerator.

This CL is preparation for server path injection, especially for testing.
It Extracts URL constructing logic from GetURL methods of the classes in
drive_api_operations.{h,cc}, and puts them into a new class
DriveApiUrlGenerator.
DriveApiUrlGenerator is given via operation classes' constructors from their
client (DriveApiService).
The concept of this CL is very similar to the following CLs:
https://codereview.chromium.org/11411086
https://codereview.chromium.org/11418084
https://codereview.chromium.org/11417108
https://codereview.chromium.org/11417109

BUG=162155
TEST=ran unit_tests and chromeos_unittests


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

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

chrome/browser/chromeos/drive/drive_api_service.cc

@@ -218,6 +218,7 @@ void DriveAPIService::GetFilelist(
       new google_apis::GetFilelistOperation(
           operation_registry(),
           url_request_context_getter_,
+          url_generator_,
           url,
           search_query,
           base::Bind(&ParseResourceListAndRun, callback)));
@@ -234,6 +235,7 @@ void DriveAPIService::GetChangelist(
       new google_apis::GetChangelistOperation(
           operation_registry(),
           url_request_context_getter_,
+          url_generator_,
           url,
           start_changestamp,
           base::Bind(&ParseResourceListAndRun, callback)));
@@ -248,6 +250,7 @@ void DriveAPIService::GetResourceEntry(
   runner_->StartOperationWithRetry(new google_apis::GetFileOperation(
       operation_registry(),
       url_request_context_getter_,
+      url_generator_,
       resource_id,
       base::Bind(&ParseResourceEntryAndRun, callback)));
 }
@@ -261,6 +264,7 @@ void DriveAPIService::GetAccountMetadata(
       new google_apis::GetAboutOperation(
           operation_registry(),
           url_request_context_getter_,
+          url_generator_,
           base::Bind(&ParseAccounetMetadataAndRun, callback)));
 }
 
@@ -272,6 +276,7 @@ void DriveAPIService::GetApplicationInfo(
   runner_->StartOperationWithRetry(
       new google_apis::GetApplistOperation(operation_registry(),
                                            url_request_context_getter_,
+                                           url_generator_,
                                            callback));
 }
 

chrome/browser/chromeos/drive/drive_api_service.h

@@ -11,6 +11,7 @@
 #include "base/observer_list.h"
 #include "chrome/browser/google_apis/auth_service.h"
 #include "chrome/browser/google_apis/auth_service_observer.h"
+#include "chrome/browser/google_apis/drive_api_url_generator.h"
 #include "chrome/browser/google_apis/drive_service_interface.h"
 
 class FilePath;
@@ -154,6 +155,7 @@ class DriveAPIService : public google_apis::DriveServiceInterface,
   Profile* profile_;
   scoped_ptr<google_apis::OperationRunner> runner_;
   ObserverList<google_apis::DriveServiceObserver> observers_;
+  google_apis::DriveApiUrlGenerator url_generator_;
   const std::string custom_user_agent_;
 
   DISALLOW_COPY_AND_ASSIGN(DriveAPIService);

chrome/browser/google_apis/drive_api_operations.cc

@@ -4,39 +4,24 @@
 
 #include "chrome/browser/google_apis/drive_api_operations.h"
 
-#include "base/string_number_conversions.h"
-#include "base/stringprintf.h"
-#include "chrome/common/net/url_util.h"
-
 namespace google_apis {
 
-namespace {
-
-const char kDriveV2AboutURL[] = "https://www.googleapis.com/drive/v2/about";
-const char kDriveV2ApplistURL[] = "https://www.googleapis.com/drive/v2/apps";
-const char kDriveV2ChangelistURL[] =
-    "https://www.googleapis.com/drive/v2/changes";
-
-const char kDriveV2FilelistURL[] = "https://www.googleapis.com/drive/v2/files";
-const char kDriveV2FileURLFormat[] =
-    "https://www.googleapis.com/drive/v2/files/%s";
-
-}  // namespace
-
 //============================== GetAboutOperation =============================
 
 GetAboutOperation::GetAboutOperation(
     OperationRegistry* registry,
     net::URLRequestContextGetter* url_request_context_getter,
+    const DriveApiUrlGenerator& url_generator,
     const GetDataCallback& callback)
-    : GetDataOperation(registry, url_request_context_getter, callback) {
+    : GetDataOperation(registry, url_request_context_getter, callback),
+      url_generator_(url_generator) {
   DCHECK(!callback.is_null());
 }
 
 GetAboutOperation::~GetAboutOperation() {}
 
 GURL GetAboutOperation::GetURL() const {
-  return GURL(kDriveV2AboutURL);
+  return url_generator_.GetAboutUrl();
 }
 
 //============================== GetApplistOperation ===========================
@@ -44,15 +29,17 @@ GURL GetAboutOperation::GetURL() const {
 GetApplistOperation::GetApplistOperation(
     OperationRegistry* registry,
     net::URLRequestContextGetter* url_request_context_getter,
+    const DriveApiUrlGenerator& url_generator,
     const GetDataCallback& callback)
-    : GetDataOperation(registry, url_request_context_getter, callback) {
+    : GetDataOperation(registry, url_request_context_getter, callback),
+      url_generator_(url_generator) {
   DCHECK(!callback.is_null());
 }
 
 GetApplistOperation::~GetApplistOperation() {}
 
 GURL GetApplistOperation::GetURL() const {
-  return GURL(kDriveV2ApplistURL);
+  return url_generator_.GetApplistUrl();
 }
 
 //============================ GetChangelistOperation ==========================
@@ -60,24 +47,21 @@ GURL GetApplistOperation::GetURL() const {
 GetChangelistOperation::GetChangelistOperation(
     OperationRegistry* registry,
     net::URLRequestContextGetter* url_request_context_getter,
+    const DriveApiUrlGenerator& url_generator,
     const GURL& url,
     int64 start_changestamp,
     const GetDataCallback& callback)
     : GetDataOperation(registry, url_request_context_getter, callback),
-      url_(kDriveV2ChangelistURL),
+      url_generator_(url_generator),
+      url_(url),
       start_changestamp_(start_changestamp) {
   DCHECK(!callback.is_null());
-  if (!url.is_empty())
-    url_ = url;
 }
 
 GetChangelistOperation::~GetChangelistOperation() {}
 
 GURL GetChangelistOperation::GetURL() const {
-  if (start_changestamp_)
-    return chrome_common_net::AppendOrReplaceQueryParameter(
-        url_, "startChangeId", base::Int64ToString(start_changestamp_));
-  return url_;
+  return url_generator_.GetChangelistUrl(url_, start_changestamp_);
 }
 
 //============================= GetFlielistOperation ===========================
@@ -85,25 +69,21 @@ GURL GetChangelistOperation::GetURL() const {
 GetFilelistOperation::GetFilelistOperation(
     OperationRegistry* registry,
     net::URLRequestContextGetter* url_request_context_getter,
+    const DriveApiUrlGenerator& url_generator,
     const GURL& url,
     const std::string& search_string,
     const GetDataCallback& callback)
     : GetDataOperation(registry, url_request_context_getter, callback),
-      url_(kDriveV2FilelistURL),
+      url_generator_(url_generator),
+      url_(url),
       search_string_(search_string) {
   DCHECK(!callback.is_null());
-  if (!url.is_empty())
-    url_ = url;
 }
 
 GetFilelistOperation::~GetFilelistOperation() {}
 
 GURL GetFilelistOperation::GetURL() const {
-  if (!search_string_.empty()) {
-    return chrome_common_net::AppendOrReplaceQueryParameter(
-        url_, "q", search_string_);
-  }
-  return url_;
+  return url_generator_.GetFilelistUrl(url_, search_string_);
 }
 
 //=============================== GetFlieOperation =============================
@@ -111,9 +91,11 @@ GURL GetFilelistOperation::GetURL() const {
 GetFileOperation::GetFileOperation(
     OperationRegistry* registry,
     net::URLRequestContextGetter* url_request_context_getter,
+    const DriveApiUrlGenerator& url_generator,
     const std::string& file_id,
     const GetDataCallback& callback)
     : GetDataOperation(registry, url_request_context_getter, callback),
+      url_generator_(url_generator),
       file_id_(file_id) {
   DCHECK(!callback.is_null());
 }
@@ -121,7 +103,7 @@ GetFileOperation::GetFileOperation(
 GetFileOperation::~GetFileOperation() {}
 
 GURL GetFileOperation::GetURL() const {
-  return GURL(base::StringPrintf(kDriveV2FileURLFormat, file_id_.c_str()));
+  return url_generator_.GetFileUrl(file_id_);
 }
 
 }  // namespace google_apis

chrome/browser/google_apis/drive_api_operations.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "chrome/browser/google_apis/base_operations.h"
+#include "chrome/browser/google_apis/drive_api_url_generator.h"
 
 namespace net {
 class URLRequestContextGetter;
@@ -22,6 +23,7 @@ class GetAboutOperation : public GetDataOperation {
  public:
   GetAboutOperation(OperationRegistry* registry,
                     net::URLRequestContextGetter* url_request_context_getter,
+                    const DriveApiUrlGenerator& url_generator,
                     const GetDataCallback& callback);
   virtual ~GetAboutOperation();
 
@@ -30,6 +32,8 @@ class GetAboutOperation : public GetDataOperation {
   virtual GURL GetURL() const OVERRIDE;
 
  private:
+  const DriveApiUrlGenerator url_generator_;
+
   DISALLOW_COPY_AND_ASSIGN(GetAboutOperation);
 };
 
@@ -40,6 +44,7 @@ class GetApplistOperation : public GetDataOperation {
  public:
   GetApplistOperation(OperationRegistry* registry,
                       net::URLRequestContextGetter* url_request_context_getter,
+                      const DriveApiUrlGenerator& url_generator,
                       const GetDataCallback& callback);
   virtual ~GetApplistOperation();
 
@@ -48,6 +53,8 @@ class GetApplistOperation : public GetDataOperation {
   virtual GURL GetURL() const OVERRIDE;
 
  private:
+  const DriveApiUrlGenerator url_generator_;
+
   DISALLOW_COPY_AND_ASSIGN(GetApplistOperation);
 };
 
@@ -64,6 +71,7 @@ class GetChangelistOperation : public GetDataOperation {
   GetChangelistOperation(
       OperationRegistry* registry,
       net::URLRequestContextGetter* url_request_context_getter,
+      const DriveApiUrlGenerator& url_generator,
       const GURL& url,
       int64 start_changestamp,
       const GetDataCallback& callback);
@@ -74,6 +82,7 @@ class GetChangelistOperation : public GetDataOperation {
   virtual GURL GetURL() const OVERRIDE;
 
  private:
+  const DriveApiUrlGenerator url_generator_;
   GURL url_;
   int64 start_changestamp_;
 
@@ -88,6 +97,7 @@ class GetFilelistOperation : public GetDataOperation {
   GetFilelistOperation(
       OperationRegistry* registry,
       net::URLRequestContextGetter* url_request_context_getter,
+      const DriveApiUrlGenerator& url_generator,
       const GURL& url,
       const std::string& search_string,
       const GetDataCallback& callback);
@@ -98,6 +108,7 @@ class GetFilelistOperation : public GetDataOperation {
   virtual GURL GetURL() const OVERRIDE;
 
  private:
+  const DriveApiUrlGenerator url_generator_;
   GURL url_;
   std::string search_string_;
 
@@ -111,6 +122,7 @@ class GetFileOperation : public GetDataOperation {
  public:
   GetFileOperation(OperationRegistry* registry,
                    net::URLRequestContextGetter* url_request_context_getter,
+                   const DriveApiUrlGenerator& url_generator,
                    const std::string& file_id,
                    const GetDataCallback& callback);
   virtual ~GetFileOperation();
@@ -120,6 +132,7 @@ class GetFileOperation : public GetDataOperation {
   virtual GURL GetURL() const OVERRIDE;
 
  private:
+  const DriveApiUrlGenerator url_generator_;
   std::string file_id_;
 
   DISALLOW_COPY_AND_ASSIGN(GetFileOperation);

chrome/browser/google_apis/drive_api_url_generator.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/google_apis/drive_api_url_generator.h"
+
+#include "base/string_number_conversions.h"
+#include "base/stringprintf.h"
+#include "chrome/common/net/url_util.h"
+
+namespace google_apis {
+
+namespace {
+
+// Hard coded URLs for communication with a google drive server.
+const char kDriveV2AboutUrl[] = "https://www.googleapis.com/drive/v2/about";
+const char kDriveV2ApplistUrl[] = "https://www.googleapis.com/drive/v2/apps";
+const char kDriveV2ChangelistUrl[] =
+    "https://www.googleapis.com/drive/v2/changes";
+
+const char kDriveV2FilelistUrl[] = "https://www.googleapis.com/drive/v2/files";
+const char kDriveV2FileUrlFormat[] =
+    "https://www.googleapis.com/drive/v2/files/%s";
+
+}  // namespace
+
+DriveApiUrlGenerator::DriveApiUrlGenerator() {
+  // Do nothing.
+}
+
+DriveApiUrlGenerator::~DriveApiUrlGenerator() {
+  // Do nothing.
+}
+
+GURL DriveApiUrlGenerator::GetAboutUrl() const {
+  return GURL(kDriveV2AboutUrl);
+}
+
+GURL DriveApiUrlGenerator::GetApplistUrl() const {
+  return GURL(kDriveV2ApplistUrl);
+}
+
+GURL DriveApiUrlGenerator::GetChangelistUrl(
+    const GURL& override_url, int64 start_changestamp) const {
+  // Use override_url if not empty,
+  // otherwise use the default url (kDriveV2Changelisturl).
+  const GURL& url =
+      override_url.is_empty() ? GURL(kDriveV2ChangelistUrl) : override_url;
+  return start_changestamp ?
+      chrome_common_net::AppendOrReplaceQueryParameter(
+          url, "startChangeId", base::Int64ToString(start_changestamp)) :
+      url;
+}
+
+GURL DriveApiUrlGenerator::GetFilelistUrl(
+    const GURL& override_url, const std::string& search_string) const {
+  // Use override_url if not empty,
+  // otherwise use the default url (kDriveV2FilelistUrl).
+  const GURL& url =
+      override_url.is_empty() ? GURL(kDriveV2FilelistUrl) : override_url;
+
+  return search_string.empty() ?
+      url :
+      chrome_common_net::AppendOrReplaceQueryParameter(
+          url, "q", search_string);
+}
+
+GURL DriveApiUrlGenerator::GetFileUrl(const std::string& file_id) const {
+  return GURL(base::StringPrintf(kDriveV2FileUrlFormat, file_id.c_str()));
+}
+
+}  // namespace google_apis

chrome/browser/google_apis/drive_api_url_generator.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_GOOGLE_APIS_DRIVE_API_URL_GENERATOR_H_
+#define CHROME_BROWSER_GOOGLE_APIS_DRIVE_API_URL_GENERATOR_H_
+
+#include <string>
+
+#include "googleurl/src/gurl.h"
+
+namespace google_apis {
+
+// This class is used to generate URLs for communicating with drive api
+// servers for production, and a local server for testing.
+class DriveApiUrlGenerator {
+ public:
+  // TODO(hidehiko): Pass server name to a constructor in order to inject
+  // server path for testing.
+  DriveApiUrlGenerator();
+  ~DriveApiUrlGenerator();
+
+  // Returns a URL to fetch "about" data.
+  GURL GetAboutUrl() const;
+
+  // Returns a URL to fetch "applist" data.
+  GURL GetApplistUrl() const;
+
+  // Returns a URL to fetch a list of changes.
+  // override_url:
+  //   The base url for the fetch. If empty, the default url is used.
+  // start_changestamp:
+  //   The starting point of the requesting change list, or 0 if all changes
+  //   are necessary.
+  GURL GetChangelistUrl(
+      const GURL& override_url, int64 start_changestamp) const;
+
+  // Returns a URL to fetch a list of files with the given |search_string|.
+  // override_url:
+  //   The base url for the fetching. If empty, the default url is used.
+  // search_string: The search query.
+  GURL GetFilelistUrl(
+      const GURL& override_url, const std::string& search_string) const;
+
+  // Returns a URL to fecth a file content.
+  GURL GetFileUrl(const std::string& file_id) const;
+
+  // This class is copyable hence no DISALLOW_COPY_AND_ASSIGN here.
+};
+
+}  // namespace google_apis
+
+#endif  // CHROME_BROWSER_GOOGLE_APIS_DRIVE_API_URL_GENERATOR_H_

chrome/browser/google_apis/drive_api_url_generator_unittest.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/google_apis/drive_api_url_generator.h"
+
+#include "googleurl/src/gurl.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace google_apis {
+
+class DriveApiUrlGeneratorTest : public testing::Test {
+ public:
+  DriveApiUrlGeneratorTest() {
+  }
+
+ protected:
+  DriveApiUrlGenerator url_generator_;
+};
+
+// Make sure the hard-coded urls are returned.
+// TODO(hidehiko): More detailed tests when we support server path injecting.
+TEST_F(DriveApiUrlGeneratorTest, GetAboutUrl) {
+  EXPECT_EQ("https://www.googleapis.com/drive/v2/about",
+            url_generator_.GetAboutUrl().spec());
+}
+
+TEST_F(DriveApiUrlGeneratorTest, GetApplistUrl) {
+  EXPECT_EQ("https://www.googleapis.com/drive/v2/apps",
+            url_generator_.GetApplistUrl().spec());
+}
+
+TEST_F(DriveApiUrlGeneratorTest, GetChangelistUrl) {
+  // Use default URL, if |override_url| is empty.
+  // Do not add startChangeId parameter if |start_changestamp| is 0.
+  EXPECT_EQ("https://www.googleapis.com/drive/v2/changes",
+            url_generator_.GetChangelistUrl(GURL(), 0).spec());
+
+  // Set startChangeId parameter if |start_changestamp| is given.
+  EXPECT_EQ("https://www.googleapis.com/drive/v2/changes?startChangeId=100",
+            url_generator_.GetChangelistUrl(GURL(), 100).spec());
+
+  // Use the |override_url| for the base URL if given.
+  // The behavior for the |start_changestamp| should be as same as above cases.
+  EXPECT_EQ("https://localhost/drive/v2/changes",
+            url_generator_.GetChangelistUrl(
+                GURL("https://localhost/drive/v2/changes"), 0).spec());
+  EXPECT_EQ("https://localhost/drive/v2/changes?startChangeId=200",
+            url_generator_.GetChangelistUrl(
+                GURL("https://localhost/drive/v2/changes"), 200).spec());
+}
+
+TEST_F(DriveApiUrlGeneratorTest, GetFilelistUrl) {
+  // Use default URL, if |override_url| is empty.
+  // Do not add q parameter if |search_string| is empty.
+  EXPECT_EQ("https://www.googleapis.com/drive/v2/files",
+            url_generator_.GetFilelistUrl(GURL(), "").spec());
+
+  // Set q parameter if non-empty |search_string| is given.
+  EXPECT_EQ("https://www.googleapis.com/drive/v2/files?q=query",
+            url_generator_.GetFilelistUrl(GURL(), "query").spec());
+
+  // Use the |override_url| for the base URL if given.
+  // The behavior for the |search_string| should be as same as above cases.
+  EXPECT_EQ("https://localhost/drive/v2/files",
+            url_generator_.GetFilelistUrl(
+                GURL("https://localhost/drive/v2/files"), "").spec());
+  EXPECT_EQ("https://localhost/drive/v2/files?q=query",
+            url_generator_.GetFilelistUrl(
+                GURL("https://localhost/drive/v2/files"), "query").spec());
+}
+
+TEST_F(DriveApiUrlGeneratorTest, GetFileUrl) {
+  // |file_id| should be embedded into the url.
+  EXPECT_EQ("https://www.googleapis.com/drive/v2/files/0ADK06pfg",
+            url_generator_.GetFileUrl("0ADK06pfg").spec());
+  EXPECT_EQ("https://www.googleapis.com/drive/v2/files/0Bz0bd074",
+            url_generator_.GetFileUrl("0Bz0bd074").spec());
+}
+
+}  // namespace google_apis

chrome/chrome_browser.gypi

@@ -671,6 +671,8 @@
         'browser/google_apis/drive_api_operations.h',
         'browser/google_apis/drive_api_parser.cc',
         'browser/google_apis/drive_api_parser.h',
+        'browser/google_apis/drive_api_url_generator.cc',
+        'browser/google_apis/drive_api_url_generator.h',
         'browser/google_apis/drive_api_util.cc',
         'browser/google_apis/drive_api_util.h',
         'browser/google_apis/drive_entry_kinds.h',

chrome/chrome_tests_unit.gypi

@@ -762,6 +762,7 @@
         'browser/google/google_util_unittest.cc',
         'browser/google_apis/base_operations_unittest.cc',
         'browser/google_apis/drive_api_parser_unittest.cc',
+        'browser/google_apis/drive_api_url_generator_unittest.cc',
         'browser/google_apis/drive_uploader_unittest.cc',
         'browser/google_apis/gdata_wapi_parser_unittest.cc',
         'browser/google_apis/gdata_wapi_operations_unittest.cc',