[SyncFS] Implement SyncEngineInitializer

[SyncFS] Implement SyncEngineInitializer

 * Implement the stubs of SyncEngineInitializer and add an initial test for it.

BUG=240165
TEST="unit_tests --gtest_filter=SyncEngineInitializerTest.*"

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222429

[tzik, 2013-09-05 08:28:39]

PTL.

This is not yet ready to land, (has no test for now, has some NOTIMPLEMENTED),
but will help your review to previous patch.

[tzik, 2013-09-09 07:29:43]

PTAL, I think it's ready to review.

[kinuko, 2013-09-09 12:35:50]

looks good.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:24: //
These functions touches GData WAPI classes directly.
touches -> touch
GData WAPI classes -> google_apis classes ?


The range of 'These' is a bit unclear...

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:52: //
Posts a request to continue listing.  Returns false if the list don't need
nit: don't -> doesn't

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:325:
callback.Run(SYNC_STATUS_OK);
Does this indirection have some meaning?

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:45: // 
- Find the remote sync-root folder, that title is
that -> whose (that cannot be used for possessive)

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:47: //  
 Note that if the initialization interrupted by the browser restart or an
interrupted -> is interrupted

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:48: //  
 error, the sequence leaves the folder in the root directory.  So, if we
directory -> folder?

There're many 'root' directories, it might be clearer to say 'Drive root' rather
than just 'root'

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:102:
drive::DriveServiceInterface* drive_api_;
nit: if we use this type can we use more regular name, i.e. drive_service_ here?

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer_unittest.cc
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer_unittest.cc:84:
google_apis::GDataErrorCode GetFileByTrackerID(
In MetadataDatabase 'FindFileBy...' usually means returning FileMetadata. 
FillFileResourceByTrackerID?

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer_unittest.cc:135:
EXPECT_EQ("Chrome Syncable FileSystem", sync_root->resource->title());
Can we add a few more tests?

[nhiroki, 2013-09-09 14:21:57]

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:189: //
Pick an orphaned folder or a direct child of the the root folder and
"the the" -> "the"

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:201: //
If there are more result, retrieve them.
"result" -> "results"

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:245:
void SyncEngineInitializer::DetachSyncRoot(const SyncStatusCallback& callback) {
Can we have "DCHECK(sync_root_folder_);" here?

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:54: //  
 handle as regular inactive folders until they are registered explicitly.
"but handle them handle as..." -> "but handle them as..." ?

[tzik, 2013-09-10 06:00:42]

Updated! PTAL.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:24: //
These functions touches GData WAPI classes directly.
On 2013/09/09 12:35:50, kinuko wrote:
> touches -> touch
> GData WAPI classes -> google_apis classes ?
> 
> 
> The range of 'These' is a bit unclear...

Done.
I meant the classes in gdata_wapi_parser.h.

Since we are moving from them to the classes in drive_api_parser.h.
I think wrapping the access to legacy class will make the migration easier.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:52: //
Posts a request to continue listing.  Returns false if the list don't need
On 2013/09/09 12:35:50, kinuko wrote:
> nit: don't -> doesn't

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:189: //
Pick an orphaned folder or a direct child of the the root folder and
On 2013/09/09 14:21:57, nhiroki wrote:
> "the the" -> "the"

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:201: //
If there are more result, retrieve them.
On 2013/09/09 14:21:57, nhiroki wrote:
> "result" -> "results"

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:245:
void SyncEngineInitializer::DetachSyncRoot(const SyncStatusCallback& callback) {
On 2013/09/09 14:21:57, nhiroki wrote:
> Can we have "DCHECK(sync_root_folder_);" here?

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:325:
callback.Run(SYNC_STATUS_OK);
On 2013/09/09 12:35:50, kinuko wrote:
> Does this indirection have some meaning?

Deleted.

This was meaningful only for a deleted code.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:45: // 
- Find the remote sync-root folder, that title is
On 2013/09/09 12:35:50, kinuko wrote:
> that -> whose (that cannot be used for possessive)

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:47: //  
 Note that if the initialization interrupted by the browser restart or an
On 2013/09/09 12:35:50, kinuko wrote:
> interrupted -> is interrupted

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:48: //  
 error, the sequence leaves the folder in the root directory.  So, if we
On 2013/09/09 12:35:50, kinuko wrote:
> directory -> folder?
> 
> There're many 'root' directories, it might be clearer to say 'Drive root'
rather
> than just 'root'

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:54: //  
 handle as regular inactive folders until they are registered explicitly.
On 2013/09/09 14:21:57, nhiroki wrote:
> "but handle them handle as..." -> "but handle them as..." ?

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.h:102:
drive::DriveServiceInterface* drive_api_;
On 2013/09/09 12:35:50, kinuko wrote:
> nit: if we use this type can we use more regular name, i.e. drive_service_
here?

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer_unittest.cc
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer_unittest.cc:84:
google_apis::GDataErrorCode GetFileByTrackerID(
On 2013/09/09 12:35:50, kinuko wrote:
> In MetadataDatabase 'FindFileBy...' usually means returning FileMetadata. 
> FillFileResourceByTrackerID?

Done.

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer_unittest.cc:135:
EXPECT_EQ("Chrome Syncable FileSystem", sync_root->resource->title());
On 2013/09/09 12:35:50, kinuko wrote:
> Can we add a few more tests?

That needs some more preparation. Can I add them in a separate CL?

[nhiroki, 2013-09-10 06:10:57]

LGTM

nit: can you fill the "TEST=" line in the description with "unit_tests
--gtest_filter=SyncEngineInitializerTest.*" ?

[kinuko, 2013-09-10 06:43:23]

lgtm

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:24: //
These functions touches GData WAPI classes directly.
On 2013/09/10 06:00:42, tzik wrote:
> On 2013/09/09 12:35:50, kinuko wrote:
> > touches -> touch
> > GData WAPI classes -> google_apis classes ?
> > 
> > 
> > The range of 'These' is a bit unclear...
> 
> Done.
> I meant the classes in gdata_wapi_parser.h.
> 
> Since we are moving from them to the classes in drive_api_parser.h.
> I think wrapping the access to legacy class will make the migration easier.

I see, then maybe you can just write like that?
"These functions are for wrapping the access to legacy GData WAPI classes."

[tzik, 2013-09-10 08:45:44]

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc
(right):

https://codereview.chromium.org/23949002/diff/19001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/sync_engine_initializer.cc:24: //
These functions touches GData WAPI classes directly.
On 2013/09/10 06:43:23, kinuko wrote:
> On 2013/09/10 06:00:42, tzik wrote:
> > On 2013/09/09 12:35:50, kinuko wrote:
> > > touches -> touch
> > > GData WAPI classes -> google_apis classes ?
> > > 
> > > 
> > > The range of 'These' is a bit unclear...
> > 
> > Done.
> > I meant the classes in gdata_wapi_parser.h.
> > 
> > Since we are moving from them to the classes in drive_api_parser.h.
> > I think wrapping the access to legacy class will make the migration easier.
> 
> I see, then maybe you can just write like that?
> "These functions are for wrapping the access to legacy GData WAPI classes."

Done.

[commit-bot: I haz the power, 2013-09-10 16:40:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/23949002/40001

[commit-bot: I haz the power, 2013-09-10 19:52:46]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-09-10 23:05:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/23949002/40001

[commit-bot: I haz the power, 2013-09-11 00:46:02]

Change committed as 222429