Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9582037:
Make document service an interface (Closed)
|
Created:
8 years, 9 months ago by Greg Spencer (Chromium) Modified:
8 years, 9 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, achuith+watch_chromium.org, mihaip+watch_chromium.org, Aaron Boodman, rginda+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThis adds an interface for DocumentsService
so that we can mock it. It also adds a mock class
that can be used in testing.
Added a test for CreateDirectory and GetFile on the GDataFilesystem class.
BUG=chromium-os:27338
TEST=ran new unit tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125926
Patch Set 1 : Adding mock for DocumentsService #
Total comments: 9
Patch Set 2 : Review changes #
Total comments: 38
Patch Set 3 : Review changes #
Total comments: 5
Patch Set 4 : Review changes #Patch Set 5 : Review changes #Patch Set 6 : Upload after merge #
Messages
Total messages: 16 (0 generated)
+satorux, benchan
http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata.h (right): http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:199: // needed. |callback| will be run with the error code and the auth bad cat and paste, make sure this matches the correct comment from DocumentsService::Authenticate() before your CL http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:263: void Authenticate(const AuthStatusCallback& callback); 1. add comments that you are overriding DocumentsServiceInterface here 2. For this and all other methods below that are overrides from DocumentsServiceInterface: - remove function description comments since they are defined in DocumentsServiceInterface - add OVERRIDE at the end of each declaration http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:37: #define FPL(x) FILE_PATH_LITERAL(x) please avoid #define , especially those that are making code even more cryptic http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:506: // TODO(gspencer): Uncomment this when we get a blob that yes, we should wait for the callback to be signaled, then entire loop with DocumentService interaction with GDataFileSystem will be tested
http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata.h (right): http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:199: // needed. |callback| will be run with the error code and the auth On 2012/03/08 00:02:03, zel wrote: > bad cat and paste, make sure this matches the correct comment from > DocumentsService::Authenticate() before your CL Done. http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:263: void Authenticate(const AuthStatusCallback& callback); On 2012/03/08 00:02:03, zel wrote: > 1. add comments that you are overriding DocumentsServiceInterface here > > 2. For this and all other methods below that are overrides from > DocumentsServiceInterface: > - remove function description comments since they are defined in > DocumentsServiceInterface > - add OVERRIDE at the end of each declaration > Yeah, I had it this way, and lost it all in the last merge. Fixed. http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:37: #define FPL(x) FILE_PATH_LITERAL(x) On 2012/03/08 00:02:03, zel wrote: > please avoid #define , especially those that are making code even more cryptic I agree, but this is a fairly common thing to do in unit tests, and it's limited to this file. There are at least seven other tests where I've seen it done.
http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:37: #define FPL(x) FILE_PATH_LITERAL(x) On 2012/03/08 01:10:39, Greg Spencer (Chromium) wrote: > On 2012/03/08 00:02:03, zel wrote: > > please avoid #define , especially those that are making code even more cryptic > > I agree, but this is a fairly common thing to do in unit tests, and it's limited > to this file. There are at least seven other tests where I've seen it done. I'm with zel. we already use FILE_PATH_LITERAL in many places in this file, so replacing them doesn't seem to be necessary. However, it's not worth debating about it, so I'm also ok with FPL. :) http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata.h (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:191: virtual ~DocumentsServiceInterface(); you could inline {} for pure virtual interface classes like this. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:249: virtual void InitiateUpload(const InitiateUploadParams& upload_file_info, upload_file_info -> params http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:255: virtual void ResumeUpload(const ResumeUploadParams& upload_file_info, ditto. params http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:269: void Initialize(Profile* profile) OVERRIDE; Please add 'virtual' to these. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system.h:369: void ReplaceDocumentsService( Rather than replacing it, wouldn't it be cleaner to inject DocumentsService from the constructor or Init()? http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (left): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:441: // mockable. Thank you for addressing my TODO! http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:50: file_system_ = GDataFileSystemFactory::GetForProfile(profile_.get()); For testing, let's do new GDataFileSystem. You can do it as GDataFileSystemTest is listed as a friend. Then, injecting MockDocumentService should be easy. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:58: file_system_->ReplaceDocumentsService(service.Pass()); Cannot you just pass mock_doc_service_? I thought it worked, but I may be wrong. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:160: class CallbackHelper nit: class comment missing. what's this used for? http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_mock.cc (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_mock.cc:20: namespace { nit: we usually have a blank line here. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_mock.cc:36: } ditto. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_mock.h (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_mock.h:89: } nit: blank line here.
https://chromiumcodereview.appspot.com/9582037/diff/9001/chrome/browser/chrom... File chrome/browser/chromeos/gdata/gdata.cc (right): https://chromiumcodereview.appspot.com/9582037/diff/9001/chrome/browser/chrom... chrome/browser/chromeos/gdata/gdata.cc:1095: DocumentsServiceInterface::DocumentsServiceInterface() { this can be removed. https://chromiumcodereview.appspot.com/9582037/diff/9001/chrome/browser/chrom... File chrome/browser/chromeos/gdata/gdata.h (right): https://chromiumcodereview.appspot.com/9582037/diff/9001/chrome/browser/chrom... chrome/browser/chromeos/gdata/gdata.h:190: DocumentsServiceInterface(); nit: per style guide, the empty default constructor can be removed. https://chromiumcodereview.appspot.com/9582037/diff/9001/chrome/browser/chrom... chrome/browser/chromeos/gdata/gdata.h:214: // Delete a document identified by its 'self' |url| and |etag|. nit: for consistency, Delete -> Deletes https://chromiumcodereview.appspot.com/9582037/diff/9001/chrome/browser/chrom... chrome/browser/chromeos/gdata/gdata.h:246: // Initiate uploading of a document/file. nit: for consistency, Initiate -> Initiates https://chromiumcodereview.appspot.com/9582037/diff/9001/chrome/browser/chrom... chrome/browser/chromeos/gdata/gdata.h:252: // Resume uploading of a document/file on the calling thread. nit: for consistency, Resume -> Resumes
http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/9582037/diff/4008/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:37: #define FPL(x) FILE_PATH_LITERAL(x) On 2012/03/08 01:48:37, satorux1 wrote: > I'm with zel. Two against. It's outta there. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata.cc (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.cc:1095: DocumentsServiceInterface::DocumentsServiceInterface() { On 2012/03/08 02:01:19, Ben Chan wrote: > this can be removed. Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata.h (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:190: DocumentsServiceInterface(); On 2012/03/08 02:01:19, Ben Chan wrote: > nit: per style guide, the empty default constructor can be removed. Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:191: virtual ~DocumentsServiceInterface(); On 2012/03/08 01:48:37, satorux1 wrote: > you could inline {} for pure virtual interface classes like this. Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:214: // Delete a document identified by its 'self' |url| and |etag|. On 2012/03/08 02:01:19, Ben Chan wrote: > nit: for consistency, Delete -> Deletes Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:246: // Initiate uploading of a document/file. On 2012/03/08 02:01:19, Ben Chan wrote: > nit: for consistency, Initiate -> Initiates Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:249: virtual void InitiateUpload(const InitiateUploadParams& upload_file_info, On 2012/03/08 01:48:37, satorux1 wrote: > upload_file_info -> params Done. Didn't mean to change this. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:252: // Resume uploading of a document/file on the calling thread. On 2012/03/08 02:01:19, Ben Chan wrote: > nit: for consistency, Resume -> Resumes Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:255: virtual void ResumeUpload(const ResumeUploadParams& upload_file_info, On 2012/03/08 01:48:37, satorux1 wrote: > ditto. params Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata.h:269: void Initialize(Profile* profile) OVERRIDE; On 2012/03/08 01:48:37, satorux1 wrote: > Please add 'virtual' to these. Done. Another casualty of that one bad merge. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system.h:369: void ReplaceDocumentsService( On 2012/03/08 01:48:37, satorux1 wrote: > Rather than replacing it, wouldn't it be cleaner to inject DocumentsService from > the constructor or Init()? It would, but I worried about the following too much: Where should I supply it, since this is a singleton created by the profile creation service, and would it cause problems with the embedded dependency mechanism in the profile creation code (now, or maybe in the future) if I bypassed that and created one directly. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (left): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:441: // mockable. On 2012/03/08 01:48:37, satorux1 wrote: > Thank you for addressing my TODO! No problem. :-) http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:50: file_system_ = GDataFileSystemFactory::GetForProfile(profile_.get()); On 2012/03/08 01:48:37, satorux1 wrote: > For testing, let's do new GDataFileSystem. You can do it as GDataFileSystemTest > is listed as a friend. > > Then, injecting MockDocumentService should be easy. Yeah, but are we bypassing anything important in GetForProfile and the profile dependency system built around it? http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:58: file_system_->ReplaceDocumentsService(service.Pass()); On 2012/03/08 01:48:37, satorux1 wrote: > Cannot you just pass mock_doc_service_? I thought it worked, but I may be wrong. Nope, that doesn't work, since the constructor for scoped_ptr is explicit. I suppose I could do this: file_system_->ReplaceDocumentsService(scoped_ptr<DocuementsServiceInterface>(mock_doc_service_).Pass()); But that seems just more confusing. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:160: class CallbackHelper On 2012/03/08 01:48:37, satorux1 wrote: > nit: class comment missing. what's this used for? Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_mock.cc (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_mock.cc:20: namespace { On 2012/03/08 01:48:37, satorux1 wrote: > nit: we usually have a blank line here. Done. However, there are examples of both ways: both in our code and the style guide. I don't think it matters either way, personally. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_mock.cc:36: } On 2012/03/08 01:48:37, satorux1 wrote: > ditto. Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_mock.h (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_mock.h:89: } On 2012/03/08 01:48:37, satorux1 wrote: > nit: blank line here. Done.
http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata.h (right): http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata.h:282: const GetDataCallback& callback) OVERRIDE; indentation - here an for all methods derived from DocumentsServiceInterface http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_mock.cc (right): http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_mock.cc:43: .WillByDefault(Invoke(this, &MockDocumentsService::AuthenticateStub)); pass base::MessageLoopProxy::current() to all *Stub function - see my comments below http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_mock.cc:82: const DownloadActionCallback& callback) { This and all other *Stub methods should be an async operations. So you need to replay asynchronously on the calling thread. Pass base::MessageLoopProxy here and post replies there as: void MockDocumentsService::DownloadDocumentStub( const GURL& content_url, DocumentExportFormat format, const DownloadActionCallback& callback, scoped_refptr<base::MessageLoopProxy::current() proxy) { proxy->PostTask(FROM_HERE, base::Bind(callback, HTTP_SUCCESS, content_url, FilePath(content_url.host() + content_url.path())); }
http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system.h:369: void ReplaceDocumentsService( On 2012/03/08 19:06:35, Greg Spencer (Chromium) wrote: > On 2012/03/08 01:48:37, satorux1 wrote: > > Rather than replacing it, wouldn't it be cleaner to inject DocumentsService > from > > the constructor or Init()? > > It would, but I worried about the following too much: Where should I supply it, > since this is a singleton created by the profile creation service, and would it > cause problems with the embedded dependency mechanism in the profile creation > code (now, or maybe in the future) if I bypassed that and created one directly. As mentioned elsewhere, I think you can inject from the constructor in the unit test. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:50: file_system_ = GDataFileSystemFactory::GetForProfile(profile_.get()); On 2012/03/08 19:06:35, Greg Spencer (Chromium) wrote: > On 2012/03/08 01:48:37, satorux1 wrote: > > For testing, let's do new GDataFileSystem. You can do it as > GDataFileSystemTest > > is listed as a friend. > > > > Then, injecting MockDocumentService should be easy. > > Yeah, but are we bypassing anything important in GetForProfile and the profile > dependency system built around it? I think it's not important to use GetForProfile(). GDataFileSystemFactory::GetForProfile() is just an indirect way to create a GDataFileSystem object to make it tied with the profile lifetime. I'm more worried about creating a non-mock DocumentService and throw it away. :)
http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_mock.cc (right): http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_mock.cc:82: const DownloadActionCallback& callback) { On 2012/03/08 19:14:08, zel wrote: > This and all other *Stub methods should be an async operations. So you need to > replay asynchronously on the calling thread. Pass base::MessageLoopProxy here > and post replies there as: > > > void MockDocumentsService::DownloadDocumentStub( > const GURL& content_url, > DocumentExportFormat format, > const DownloadActionCallback& callback, > scoped_refptr<base::MessageLoopProxy::current() proxy) { > proxy->PostTask(FROM_HERE, base::Bind(callback, HTTP_SUCCESS, content_url, > FilePath(content_url.host() + content_url.path())); > } Excellent point, but I don't think I can pass the message loop proxy to the stubs: calling ::current() from the MockDocumentsService constructor isn't going to do the right thing, and I'm not sure I can add a new argument as part of the Invoke anyhow. Surely the stub is called from the same thread as the original call. Can't I just create a proxy inside the stub and post the task from there, like this: void MockDocumentsService::DownloadDocumentStub( const GURL& content_url, DocumentExportFormat format, const DownloadActionCallback& callback) { base::MessageLoopProxy::current()->PostTask( FROM_HERE, base::Bind(callback, HTTP_SUCCESS, content_url, FilePath(content_url.host() + content_url.path()))); }
http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... File chrome/browser/chromeos/gdata/gdata_mock.cc (right): http://codereview.chromium.org/9582037/diff/17001/chrome/browser/chromeos/gda... chrome/browser/chromeos/gdata/gdata_mock.cc:82: const DownloadActionCallback& callback) { I guess that would work too.
PTAL. I think I've addressed everything. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system.h (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system.h:369: void ReplaceDocumentsService( On 2012/03/08 19:28:37, satorux1 wrote: > As mentioned elsewhere, I think you can inject from the constructor in the unit > test. Done. http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right): http://codereview.chromium.org/9582037/diff/9001/chrome/browser/chromeos/gdat... chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:50: file_system_ = GDataFileSystemFactory::GetForProfile(profile_.get()); On 2012/03/08 19:28:37, satorux1 wrote: > I think it's not important to use GetForProfile(). > GDataFileSystemFactory::GetForProfile() is just an indirect way to create a > GDataFileSystem object to make it tied with the profile lifetime. > > I'm more worried about creating a non-mock DocumentService and throw it away. :) As long as we never make GDataFileSystem dependent upon anything else in the profile I guess that's fine. Done.
LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gspencer@chromium.org/9582037/25001
Change committed as 125926 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
