gdata: Move ownership of GDataCache to GDataSystemService

chrome/browser/chromeos/gdata/gdata_sync_client.h

Index: chrome/browser/chromeos/gdata/gdata_sync_client.h
diff --git a/chrome/browser/chromeos/gdata/gdata_sync_client.h b/chrome/browser/chromeos/gdata/gdata_sync_client.h
index 46a77b1606329d102b36d943fdadfde38ac89424..610170140e1a8f47d98f2024157e8ba32c18bfe9 100644
--- a/chrome/browser/chromeos/gdata/gdata_sync_client.h
+++ b/chrome/browser/chromeos/gdata/gdata_sync_client.h
@@ -62,7 +62,8 @@ class GDataSyncClient
   // |file_system| is used access the
   // cache (ex. store a file to the cache when the file is downloaded).
   GDataSyncClient(Profile* profile,
-                  GDataFileSystemInterface* file_system);
+                  GDataFileSystemInterface* file_system,

[achuithb, 2012/06/11 21:58:10]

Why not just pass in GDataSystemService* and have helpers to access
GDataFileSystem* and GDataCache* rather than holding on to the pointers?

[hashimoto, 2012/06/12 02:00:18]

I thought doing so will introduce unnecessary dependencies, but it turned out
that it will not be a problem as long as we keep GDataSystemService a simple
container class.

Will do.

[hashimoto, 2012/06/12 06:20:12]

I thought this was a good thing to do, but I began to think it's not because of
the following reasons, can we move ahead without passing GDataSystemService*?
If you don't like passing too many arguments to ctors, can we fix it later?

1. Testing: If we pass GDataSystemService for ctors of GDataSyncClient and
GDataFileSystem, it means that we have to create a GDataSystemService instance
in unit tests.  Why do we have to instante all GDataSystemService members when
we only need to test GDataFileSystem?  Adding to that, we have to introduce a
new setters to replace GDatasystemService members with mocks which we don't need
if we are passing pointers one-by-one for ctors.

2. Class dependencies: Passing pointer arguments one-by-one explicitly is good
to manage class dependencies, if we pass GDataSystemService* for these classes,
random dependencies can be easily made and soon we will reach to the point where
the whole code is depending to random parts of other files and we can no longer
manage it.
(BTW, I found GDataFileSystem::StartFileUploadOnUIThread and
GDataFileSystem::OnTransferCompleted are instantiating GDataSystemService and I
think this should be fixed.)

3. Efficiency: Since gdata_file_system.h is included by gdata_system_service.h,
we cannot include the latter from the former (doing it has no effect thanks to
the include guard).  So, it means we cannot inline helper functions like
GDataFileSystem::cache() and GDataFileSystem::documents_service() since
GDataSystemService is incomplete type while parsing gdata_file_system.h.  This
is not a big problem because the overhead is small, but worth noting.  This can
be fixed by separating declarations of interfaces and enums from
gdata_file_system.h (and other gdata_*.h files) by making a new header file, but
it may be a somehow big change.

+                  GDataCache* cache);
   virtual ~GDataSyncClient();
 
   // GDataSyncClientInterface overrides.
@@ -127,7 +128,8 @@ class GDataSyncClient
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
   Profile* profile_;
-  GDataFileSystemInterface* file_system_;
+  GDataFileSystemInterface* file_system_;  // Owned by GDataSystemService.
+  GDataCache* cache_;  // Owned by GDataSystemService.
   scoped_ptr<PrefChangeRegistrar> registrar_;
 
   // The queue of resource IDs used to fetch pinned-but-not-fetched files in