gdata: Move ownership of GDataCache to GDataSystemService

gdata: Move ownership of GDataCache to GDataSystemService

BUG=131756
TEST=unit_tests --gtest_filter="GData*"


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141643

[hashimoto, 2012-06-11 13:02:22]

Took over satorux's patch (https://chromiumcodereview.appspot.com/10533073/)

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

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.h:411: GDataCache* cache,
Why not pass in GDataSystemService* instead of GDataCache* and
DocumentsServiceInterface*? We can write convenience functions to access the
cache and the doc service.

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_sync_client.h (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_sync_client.h:65: GDataFileSystemInterface*
file_system,
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]

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_sync_client.h (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_sync_client.h:65: GDataFileSystemInterface*
file_system,
On 2012/06/11 21:58:10, achuith.bhandarkar wrote:
> Why not just pass in GDataSystemService* and have helpers to access
> GDataFileSystem* and GDataCache* rather than holding on to the pointers?

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]

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.h:411: GDataCache* cache,
On 2012/06/11 21:58:10, achuith.bhandarkar wrote:
> Why not pass in GDataSystemService* instead of GDataCache* and
> DocumentsServiceInterface*? We can write convenience functions to access the
> cache and the doc service.

See my comment in gdata_sync_client.h

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_sync_client.h (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_sync_client.h:65: GDataFileSystemInterface*
file_system,
On 2012/06/12 02:00:18, hashimoto wrote:
> On 2012/06/11 21:58:10, achuith.bhandarkar wrote:
> > Why not just pass in GDataSystemService* and have helpers to access
> > GDataFileSystem* and GDataCache* rather than holding on to the pointers?
> 
> 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.

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.

[achuithb, 2012-06-12 08:02:30]

All good points. Let's drop this for now.

On 2012/06/12 06:20:12, hashimoto wrote:
>
http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_file_system.h (right):
> 
>
http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_file_system.h:411: GDataCache* cache,
> On 2012/06/11 21:58:10, achuith.bhandarkar wrote:
> > Why not pass in GDataSystemService* instead of GDataCache* and
> > DocumentsServiceInterface*? We can write convenience functions to access the
> > cache and the doc service.
> 
> See my comment in gdata_sync_client.h
> 
>
http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_sync_client.h (right):
> 
>
http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_sync_client.h:65:
GDataFileSystemInterface*
> file_system,
> On 2012/06/12 02:00:18, hashimoto wrote:
> > On 2012/06/11 21:58:10, achuith.bhandarkar wrote:
> > > Why not just pass in GDataSystemService* and have helpers to access
> > > GDataFileSystem* and GDataCache* rather than holding on to the pointers?
> > 
> > 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.
> 
> 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.

[achuithb, 2012-06-12 08:17:43]

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.h:1470:
DocumentsServiceInterface* documents_service_;
Can we also add a comment here that this is owned by GDataSystemService.

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:32: cache_(
I think GDataCache creation needs to happen on the blocking pool. I think this
is will be quite problematic because you need to PostTaskAndReply to the
blocking pool, then after creation, set the cache_ pointer in file_system_.
Until this finishes, file_system_ can't really be used.

[hashimoto, 2012-06-12 08:22:55]

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:32: cache_(
On 2012/06/12 08:17:43, achuith.bhandarkar wrote:
> I think GDataCache creation needs to happen on the blocking pool. I think this
> is will be quite problematic because you need to PostTaskAndReply to the
> blocking pool, then after creation, set the cache_ pointer in file_system_.
> Until this finishes, file_system_ can't really be used. 

Current code creates GDataCache on the UI thread, can we do it later in a
separated patch set?  I think this may be somehow tricky change and will have a
good chance to get reverted.

[hashimoto, 2012-06-12 08:25:22]

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.h:1470:
DocumentsServiceInterface* documents_service_;
On 2012/06/12 08:17:43, achuith.bhandarkar wrote:
> Can we also add a comment here that this is owned by GDataSystemService.

Done.

[achuithb, 2012-06-12 08:36:54]

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:32: cache_(
On 2012/06/12 08:22:55, hashimoto wrote:
> On 2012/06/12 08:17:43, achuith.bhandarkar wrote:
> > I think GDataCache creation needs to happen on the blocking pool. I think
this
> > is will be quite problematic because you need to PostTaskAndReply to the
> > blocking pool, then after creation, set the cache_ pointer in file_system_.
> > Until this finishes, file_system_ can't really be used. 
> 
> Current code creates GDataCache on the UI thread, can we do it later in a
> separated patch set?  I think this may be somehow tricky change and will have
a
> good chance to get reverted.

Yea - can we add back the comment and TODO then?

[hashimoto, 2012-06-12 08:40:51]

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10546093/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:32: cache_(
On 2012/06/12 08:36:54, achuith.bhandarkar wrote:
> On 2012/06/12 08:22:55, hashimoto wrote:
> > On 2012/06/12 08:17:43, achuith.bhandarkar wrote:
> > > I think GDataCache creation needs to happen on the blocking pool. I think
> this
> > > is will be quite problematic because you need to PostTaskAndReply to the
> > > blocking pool, then after creation, set the cache_ pointer in
file_system_.
> > > Until this finishes, file_system_ can't really be used. 
> > 
> > Current code creates GDataCache on the UI thread, can we do it later in a
> > separated patch set?  I think this may be somehow tricky change and will
have
> a
> > good chance to get reverted.
> 
> Yea - can we add back the comment and TODO then?

Done.

[achuithb, 2012-06-12 08:41:50]

lgtm

[commit-bot: I haz the power, 2012-06-12 09:46:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10546093/1014

[commit-bot: I haz the power, 2012-06-12 11:08:00]

Change committed as 141643