GDataFileSystem is no longer a friend of GDataDirectory.

GDataFileSystem is no longer a friend of GDataDirectory.

* Add public methods AddEntryToDirectory, RemoveEntryFromParent and RefreshDirectory to GDataDirectoryService, as well as private helper RefreshDirectoryInternal. Also make RefreshFileInternal private. All of these take FileMoveCallback.
* Add OnDirectoryChangeFileMoveCallback to handle callbacks of RemoveEntryFromParent and RefreshDirectory. AddEntryToDirectory uses OnMoveEntryToDirectoryWithFileMoveCallback.
* AddEntryToDirectory and RemoveEntryFromParent just call GDataDirectory's methods AddEntry and RemoveEntry.
* Logic of GDataFileSystem::RequestDirectoryRefreshByEntry is now in GDataDirectoryService::RefreshDirectoryInternal.
* AddUploadedFileOnUIThread has been broken up with new continuation functions ContinueAddUploadedFile and AddUploadedFileToCache, as well as AddUploadedFileParams, to ensure proper ordering of function invocations.
* MoveEntryToDirectory no longer accepts null callbacks.
* Ensure parent_ pointer is correct at all times. It needs to be reset in RemoveChild, and be NULL in AddEntry.

BUG=137725
TEST=unit tests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151605

[achuithb, 2012-08-10 11:25:04]

Please take a look, Satoru-san.

[satorux1, 2012-08-10 16:14:44]

Cool. Things are getting untangled. :)

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:395:
AddEntryToDirectoryCallback;
nit: add a blank line here.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.h:535: GDataDirectory*
parent_dir,
indentation

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:300: iter->second->SetParent(NULL);
iter->second looks cryptic. maybe add:


  GDataFile* file = iter->second;

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:308: iter->second->SetParent(NULL);
ditto

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:574: const base::Closure& callback)
{
As of now, this function won't fail as we are dealing with in-memory data,
however, with leveldb, I guess this operation may fail. We might want to use
FileOperationCallback for this. What do you think?

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:575: DCHECK(!callback.is_null());
maybe add DCHECK(directory) and DCHECK(new_entry) ?

please add a blank line after DCHECKs

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:611: const base::Closure& callback)
{
ditto. FileOperationCallback?

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:765: const base::Closure& callback)
{
DCHECK(!callback.is_null())

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:766:
GetEntryByResourceIdAsync(directory_resource_id,
nit: please move directory_resource_id to the next line per sky format.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:777: DCHECK(!callback.is_null());
nit: add a blank line like we do elsewhere

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.h:491: // refreshed.
please document if |callback| is optional or not.

[satorux1, 2012-08-10 23:05:54]

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2458: GDataEntry* entry =
directory_service_->FromDocumentEntry(doc_entry.get());
we might want to create a proto instead. See my next comment.

GDataEntryProto* entry_proto =
directory_service_->FromDocumentEntry(doc_entry.get());

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2469: entry,
would it be possible for this function to take GDataEntryProto instead?

I think GDataDirectoryService:: should take proto rater than GDataEntry family.

If it's too much work to do in this patch, you can do it in a separate patch. In
this case, please file a bug.

FWIW, GDataDirectory::AddEntry() is also used in AddNewDirectory() and I want to
get rid of FindEntryByPathSync() there, but I need to be able to add a new entry
to the GDataDirectoryService, as proto, rather than GDataEntry*.

[satorux1, 2012-08-11 00:12:21]

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2469: entry,
On 2012/08/10 23:05:55, satorux1 wrote:
> would it be possible for this function to take GDataEntryProto instead?
> 
> I think GDataDirectoryService:: should take proto rater than GDataEntry
family.
> 
> If it's too much work to do in this patch, you can do it in a separate patch.
In
> this case, please file a bug.
> 
> FWIW, GDataDirectory::AddEntry() is also used in AddNewDirectory() and I want
to
> get rid of FindEntryByPathSync() there, but I need to be able to add a new
entry
> to the GDataDirectoryService, as proto, rather than GDataEntry*.

Filed a bug crbug.com/142048

[achuithb, 2012-08-11 00:37:45]

The major change is replacing base::Closure with FileMoveCallback. This was an
excellent suggestion. PTAL, Satoru-san.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:395:
AddEntryToDirectoryCallback;
On 2012/08/10 16:14:44, satorux1 wrote:
> nit: add a blank line here.

This is gone.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2458: GDataEntry* entry =
directory_service_->FromDocumentEntry(doc_entry.get());
On 2012/08/10 23:05:55, satorux1 wrote:
> we might want to create a proto instead. See my next comment.
> 
> GDataEntryProto* entry_proto =
> directory_service_->FromDocumentEntry(doc_entry.get());

Let's do this in a separate patch.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2469: entry,
On 2012/08/10 23:05:55, satorux1 wrote:
> would it be possible for this function to take GDataEntryProto instead?
> 
> I think GDataDirectoryService:: should take proto rater than GDataEntry
family.
> 
> If it's too much work to do in this patch, you can do it in a separate patch.
In
> this case, please file a bug.
> 
> FWIW, GDataDirectory::AddEntry() is also used in AddNewDirectory() and I want
to
> get rid of FindEntryByPathSync() there, but I need to be able to add a new
entry
> to the GDataDirectoryService, as proto, rather than GDataEntry*.

I'll file a bug and add a TODO.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_file_system.h:535: GDataDirectory*
parent_dir,
On 2012/08/10 16:14:44, satorux1 wrote:
> indentation

Done.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:300: iter->second->SetParent(NULL);
On 2012/08/10 16:14:44, satorux1 wrote:
> iter->second looks cryptic. maybe add:
> 
> 
>   GDataFile* file = iter->second;

Done.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:308: iter->second->SetParent(NULL);
On 2012/08/10 16:14:44, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:574: const base::Closure& callback)
{
On 2012/08/10 16:14:44, satorux1 wrote:
> As of now, this function won't fail as we are dealing with in-memory data,
> however, with leveldb, I guess this operation may fail. We might want to use
> FileOperationCallback for this. What do you think?

This is an excellent suggestion, however we want to use FileMoveCallback. This
simplifies a bunch of code!

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:575: DCHECK(!callback.is_null());
On 2012/08/10 16:14:44, satorux1 wrote:
> maybe add DCHECK(directory) and DCHECK(new_entry) ?
> 
> please add a blank line after DCHECKs

Done.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:611: const base::Closure& callback)
{
On 2012/08/10 16:14:44, satorux1 wrote:
> ditto. FileOperationCallback?

Done. FileMoveCallback here and also in RefreshDirectory.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:765: const base::Closure& callback)
{
On 2012/08/10 16:14:44, satorux1 wrote:
> DCHECK(!callback.is_null())

Done.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:766:
GetEntryByResourceIdAsync(directory_resource_id,
On 2012/08/10 16:14:44, satorux1 wrote:
> nit: please move directory_resource_id to the next line per sky format.

Done.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.cc:777: DCHECK(!callback.is_null());
On 2012/08/10 16:14:44, satorux1 wrote:
> nit: add a blank line like we do elsewhere

Done.

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10829277/diff/1/chrome/browser/chromeos/gdata/...
chrome/browser/chromeos/gdata/gdata_files.h:491: // refreshed.
On 2012/08/10 16:14:44, satorux1 wrote:
> please document if |callback| is optional or not.

Done here and elsewhere.

[satorux1, 2012-08-11 11:48:16]

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:494: // Note that |callback| is only
called if the directory is successfully
this behavior looks unusual. In other places, we call a callback with an error
code when an operation failed. I think it'd be nicer to do the same here too.

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:526: // first was found.
Thanks!

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:549: // Remove all child files of
|directory| and replace with file_map.
Removes

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:551: // refreshed. |callback| may
not be null.
Ditto. This behavior is worrisome.

[achuithb, 2012-08-13 22:18:10]

PTAL, Satoru-san.

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:494: // Note that |callback| is only
called if the directory is successfully
On 2012/08/11 11:48:17, satorux1 wrote:
> this behavior looks unusual. In other places, we call a callback with an error
> code when an operation failed. I think it'd be nicer to do the same here too.

Good point. Done.

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:526: // first was found.
On 2012/08/11 11:48:17, satorux1 wrote:
> Thanks!

Np.

http://codereview.chromium.org/10829277/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_files.h:551: // refreshed. |callback| may
not be null.
On 2012/08/11 11:48:17, satorux1 wrote:
> Ditto. This behavior is worrisome.

Done.

[satorux1, 2012-08-13 22:35:53]

http://codereview.chromium.org/10829277/diff/5004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10829277/diff/5004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2941: parent_dir));
This code path is rather worrisome. I think the original code was broken...

Here, we request to remove an existing entry asynchronously. Then request to add
a new entry below, *without* waiting until the remove operation is done. This
looks like a race. Shouldn't we request to add an entry after the remove
operation is complete?

http://codereview.chromium.org/10829277/diff/5004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10829277/diff/5004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:432: // UI thread. |callback| may be
null.
Looking at the callers in gdata_file_system.cc, callback seems to be always
supplied. If so, could you change this to "may not be null"?

[achuithb, 2012-08-14 00:18:43]

I've had to introduce AddUploadedFileParams and 2 more functions
ContinueAddUploadedFile and AddUploadedFileToCache to ensure operations take
place in the right order after AddUploadedFileOnUIThread is called.

As a side effect, GDataDirectoryService::RemoveEntryFromParent no longer needs
to accept a null callback.

http://codereview.chromium.org/10829277/diff/5004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10829277/diff/5004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2941: parent_dir));
On 2012/08/13 22:35:53, satorux1 wrote:
> This code path is rather worrisome. I think the original code was broken...
> 
> Here, we request to remove an existing entry asynchronously. Then request to
add
> a new entry below, *without* waiting until the remove operation is done. This
> looks like a race. Shouldn't we request to add an entry after the remove
> operation is complete?

Done.

http://codereview.chromium.org/10829277/diff/5004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10829277/diff/5004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:432: // UI thread. |callback| may be
null.
On 2012/08/13 22:35:53, satorux1 wrote:
> Looking at the callers in gdata_file_system.cc, callback seems to be always
> supplied. If so, could you change this to "may not be null"?

Done.

[achuithb, 2012-08-14 00:18:53]

PTAL

[satorux1, 2012-08-14 06:11:32]

LGTM with a nit. Thanks!

http://codereview.chromium.org/10829277/diff/1013/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10829277/diff/1013/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2965: cache_operation,
callback_runner.Release());
nit: could you list parameters vertically?

[satorux1, 2012-08-14 06:11:51]

Less friendship is good. :)

[achuithb, 2012-08-14 23:33:59]

Thanks for the review!

http://codereview.chromium.org/10829277/diff/1013/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/10829277/diff/1013/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2965: cache_operation,
callback_runner.Release());
On 2012/08/14 06:11:32, satorux1 wrote:
> nit: could you list parameters vertically?

Done.