gdata: Move ownership of GDataCache to GDataSystemService

chrome/browser/chromeos/gdata/gdata_sync_client.h

 // 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.
 
 #ifndef CHROME_BROWSER_CHROMEOS_GDATA_GDATA_SYNC_CLIENT_H_
 #define CHROME_BROWSER_CHROMEOS_GDATA_GDATA_SYNC_CLIENT_H_
 #pragma once
 
 #include <deque>
 #include <string>
@@ skipping 44 matching lines @@
 class GDataSyncClient
     : public GDataSyncClientInterface,
       public GDataFileSystem::Observer,
       public chromeos::NetworkLibrary::NetworkManagerObserver,
       public content::NotificationObserver {
  public:
   // |profile| is used to access user preferences.
   // |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.
   virtual void Initialize() OVERRIDE;
 
   // GDataFileSystem::Observer overrides.
   virtual void OnCacheInitialized() OVERRIDE;
   virtual void OnFilePinned(const std::string& resource_id,
                             const std::string& md5) OVERRIDE;
   virtual void OnFileUnpinned(const std::string& resource_id,
@@ skipping 44 matching lines @@
 
   // chromeos::NetworkLibrary::NetworkManagerObserver override.
   virtual void OnNetworkManagerChanged(
       chromeos::NetworkLibrary* network_library) OVERRIDE;
 
   // content::NotificationObserver override.
   virtual void Observe(int type,
                        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
   // the background thread. Note that this class does not use a lock to
   // protect |queue_| as all methods touching |queue_| run on the UI thread.
   std::deque<std::string> queue_;
 
   // True if the fetch loop is running.
   bool fetch_loop_is_running_;
 
   base::WeakPtrFactory<GDataSyncClient> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(GDataSyncClient);
 };
 
 }  // namespace gdata
 
 #endif  // CHROME_BROWSER_CHROMEOS_GDATA_GDATA_SYNC_CLIENT_H_