Implement copy and move operations within the same remote file system.

Implement copy and move operations within the same remote file system.

This CL makes the following changes:
1. Implement copy and move operations in RemoteFileSystemOperation,
   DocumentsService, and GDataFileSystem. Copying of regular files and
   directories is yet to be implemented.
2. Add corresponding unit tests in GDataFileSystemTest.

BUG=chromium-os:26962
TEST=Tested the following:
1. Run "unit_tests --gtest_filter='*GData*'"
2. Manually test copy, move, and rename operations in file manager.

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

[Ben Chan, 2012-03-11 06:15:32]

Notes:

I will upload the changes to file manager javascript in a separated CL. Right
now, I'm debugging a regression in file manager (crosbug.com/26109).

The unit tests for Copy operation are missing, but should be ready soon.

[Ben Chan, 2012-03-12 03:04:19]

file manager CL: http://chromiumcodereview.appspot.com/9667040/

[satorux1, 2012-03-12 17:48:24]

This is awesome stuff. I think it'd be nicer to split the patch like:

1) gdata_files stuff
2) other gdata stuff
3) webkit fileapi stuff.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.cc:389: // Gets URL for GET request.
Is this only for GET? Seems to be used for POST as well?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.cc:800: const FilePath::StringType&
new_name);
just fyi: matter of taste, but for small classes like this, defined in .cc file,
you can inline all functions in the class definition than to define them
outside, which results in less repetitive code. just fyi, again.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata.h (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.h:244: // when needed. Upon completion,
invokes |callback| with results.
nit: add "on the calling thread"

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.h:253: // invokes |callback| with results.
ditto.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.h:261: // 'self' link |resource_url| to a
collection with a content link
nit: a collection -> a collection (directory)

I think it'd be useful to have some documentation about how directories are
represented in gdata, like gdata is not hierarchical and directories are
represented as collections which are nested labels, hence a document can be
present in multiple directories, which are like hard links on Unix.

We might also want to document some terminology used in this code as some terms
are fairly confusing:

- collections vs directories
- documents vs files vs hosted documents

It'd be nice if we could have decent documentation about these topics at the
beginning of the file.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:659: // TODO(benchan):
Implement copy for regular files and directories.
Please document this in the header file too. :) Please also file bugs and add
URLs to them.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:661: // file manager.
How does this work?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:680: callback);
Shouldn't we pass MessageLoopProxy::current() so |callback| is run on the origin
thread?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:715: std::string file_name =
new_name;
std::string -> FilePath::StringType

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:755: if
(src_file_path.DirName() == dest_parent_path) {
Having a comment would make it a bit easier to read:

// If the file is moved to the same directory, just rename it.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:759: callback);
Shouldn't we pass MessageLoopProxy::current() so the |callback| is run on the
origin thread?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:766: FilePathUpdateCallback
add_file_to_directory_callback =
This logic is a bit hard to follow. If I follow right, the code behaves like:

1) Rename the file to basename(dest_file_path) in the current directory
2) Remove it from the current directory
3) Add it to the target directory

Is this correct? If so, the step 1 looks unnecessary. Removing it first looks
scary. maybe we should add it first? Maybe I'm just not following right.

Please also add some comment about the behavior here to make it easier to
follow.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:770: callback);
 MessageLoopProxy::current() ?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:128: // Copies |src_file_path|
to |dest_file_path| on the file system.
Please document that if this allows copy of directories. What if |src_file_path|
is a directory?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:130: // of |dest_file_path|
need to be present in the in-memory representation
Please also document if |dest_file_path| can be a directory. "cp" command allows
the destination to be a directory (ex. cp /etc/hosts /tmp"), but here, we don't
allow it, right?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:131: // of the file system.
//
// Can be called from any thread.

?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:139: // of the file system.
ditto.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:321: virtual void
FileOperationCallback(base::PlatformFileError error) {
I think we should add a DCHECK() to ensure that the callback is run on the
origin thread.

DCHECK(CurrentlyOn(UI));

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.cc:149: file->original_file_name_ =
UTF16ToUTF8(doc->title());
this looks like an important change. It'd be nicer to change this separately in
a separate patch, rather than part of this large patch.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.h:53: void
set_original_file_name(const FilePath::StringType& name) {
While you are at it, could you document the difference between |file_name| and
|original_file_name| ?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.h:57: void set_parent(GDataDirectory*
parent) { parent_ = parent; }
Looks scary. Let's move this to protected: section, so it won't be misused from
random client code.

The tree will be messed up if this function is used wrong. for instance:

  dest_dir->AddFile(file);
  // what if we forget to call this?
  // file->set_parent(dest_dir);

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.h:59: const std::string& resource()
const { return resource_id_; }
resource() -> resource_id().

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.h:162: bool MoveFile(GDataFileBase*
file, GDataDirectory* dir);
Instead of this, what about adding a function like

  // Takes the file. The parent of |file| is changed to this directory.
  bool TakeFile(GDataFileBase* file);

One less parameter would be nice.

[satorux1, 2012-03-12 19:59:20]

BTW, splitting patches has its own cost. Please feel free to keep it in a single
patch, if splitting makes your work harder.

[zel, 2012-03-12 20:40:40]

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.cc:1033: return URLFetcher::DELETE_REQUEST;
will this delete the file completely or just remove it from the collection?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:632: return false;
there is a helper method GetGDataFileInfoFromPath() that give you everything
from 624-632. We just need to make it thread safe, I can show you what's wrong
with it in other comments.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:792: if
(!dir_params.is_directory) {
nit: remove {}

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:800: if (!callback.is_null())
{
nit: remove {}

[zel, 2012-03-12 20:46:46]

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:38: entry.name =
file.file_name();
aren't you supposed to append GDataFileBase::document_extension_ to hosted
documents here?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.cc:149: file->original_file_name_ =
UTF16ToUTF8(doc->title());
Is title required for regular (non-hosted) files too?

[Ben Chan, 2012-03-13 00:29:21]

The gdata_files changes this CL is now in
http://codereview.chromium.org/9694016/. Once that CL is merged, I will rebase
this CL to get rid of gdata_files changes.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.cc:389: // Gets URL for GET request.
On 2012/03/12 17:48:24, satorux1 wrote:
> Is this only for GET? Seems to be used for POST as well?

Fixed the comments. Also added comments for GetExtraRequestHeaders and
GetContentData

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.cc:800: const FilePath::StringType&
new_name);
On 2012/03/12 17:48:24, satorux1 wrote:
> just fyi: matter of taste, but for small classes like this, defined in .cc
file,
> you can inline all functions in the class definition than to define them
> outside, which results in less repetitive code. just fyi, again.

I had a discussion with Zel that we would like to move all operation classes to
gdata_operations.{h,cc} as gdata.cc is getting big. But because GData is a
moving target, I defer the change until things settle down a bit.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.cc:1033: return URLFetcher::DELETE_REQUEST;
On 2012/03/12 20:40:40, zel wrote:
> will this delete the file completely or just remove it from the collection?

It only removes the file from collection, not the file itself.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata.h (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.h:244: // when needed. Upon completion,
invokes |callback| with results.
On 2012/03/12 17:48:24, satorux1 wrote:
> nit: add "on the calling thread"

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.h:253: // invokes |callback| with results.
On 2012/03/12 17:48:24, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata.h:261: // 'self' link |resource_url| to a
collection with a content link
On 2012/03/12 17:48:24, satorux1 wrote:
> nit: a collection -> a collection (directory)
> 
> I think it'd be useful to have some documentation about how directories are
> represented in gdata, like gdata is not hierarchical and directories are
> represented as collections which are nested labels, hence a document can be
> present in multiple directories, which are like hard links on Unix.
> 
> We might also want to document some terminology used in this code as some
terms
> are fairly confusing:
> 
> - collections vs directories
> - documents vs files vs hosted documents
> 
> It'd be nice if we could have decent documentation about these topics at the
> beginning of the file. 

Yes, we should indeed make the terminology/naming convention more consistent in
the code. Some methods currently referring to document may apply to other kinds
of resource.

However, as GData is a moving target, I think it'd be better to minimize changes
in the meantime in order to reduce merge conflict. 

How about I make a follow-up CL to clean that up when things settle down a bit? 
I added a TODO for that.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:659: // TODO(benchan):
Implement copy for regular files and directories.
On 2012/03/12 17:48:24, satorux1 wrote:
> Please document this in the header file too. :) Please also file bugs and add
> URLs to them.

Clarified in the header. I'll keep the bug open until this is fixed.  To copy
regular files, it seems like we need to first download the file and then upload
it, which requires wiring the download/upload pipeline.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:661: // file manager.
On 2012/03/12 17:48:24, satorux1 wrote:
> How does this work?

The file manager recursively traverses the directory being copied and invokes
FileEntry.copyTo on each file encountered. That way, the file manager can show
progress of the copy operations. We will probably use the same approach for
Gdata file system, regardless of whether or not directory copy being implemented
in GDataFileSystem::Copy().

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:680: callback);
On 2012/03/12 17:48:24, satorux1 wrote:
> Shouldn't we pass MessageLoopProxy::current() so |callback| is run on the
origin
> thread?

IIUC, the DocumentsService layer handles the transition between the origin
thread and the UI thread. Rename(), AddResourceToDirectory(), and
RemoveResourceFromDirectory() run on the same thread as Copy() and Move(), and
they use MessageLoop::current(). Do we still need to pass MessageLoop::current()
as an argument to the callback?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:715: std::string file_name =
new_name;
On 2012/03/12 17:48:24, satorux1 wrote:
> std::string -> FilePath::StringType

Done.  Good catch!

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:755: if
(src_file_path.DirName() == dest_parent_path) {
On 2012/03/12 17:48:24, satorux1 wrote:
> Having a comment would make it a bit easier to read:
> 
> // If the file is moved to the same directory, just rename it.

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:759: callback);
On 2012/03/12 17:48:24, satorux1 wrote:
> Shouldn't we pass MessageLoopProxy::current() so the |callback| is run on the
> origin thread?

Rename(), AddResourceToDirectory(), and RemoveResourceFromDirectory() currently
run on the same thread as Copy() and Move(), and they all use
MessageLoop::current(). Should MessageLoop::current() be passed as an argument
instead?

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:766: FilePathUpdateCallback
add_file_to_directory_callback =
On 2012/03/12 17:48:24, satorux1 wrote:
> This logic is a bit hard to follow. If I follow right, the code behaves like:
> 
> 1) Rename the file to basename(dest_file_path) in the current directory
> 2) Remove it from the current directory
> 3) Add it to the target directory
> 
> Is this correct? If so, the step 1 looks unnecessary. Removing it first looks
> scary. maybe we should add it first? Maybe I'm just not following right.
> 
> Please also add some comment about the behavior here to make it easier to
> follow.

Done. Added more details in comments.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:792: if
(!dir_params.is_directory) {
On 2012/03/12 20:40:40, zel wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:800: if (!callback.is_null())
{
On 2012/03/12 20:40:40, zel wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.h (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:128: // Copies |src_file_path|
to |dest_file_path| on the file system.
On 2012/03/12 17:48:24, satorux1 wrote:
> Please document that if this allows copy of directories. What if
|src_file_path|
> is a directory?

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:130: // of |dest_file_path|
need to be present in the in-memory representation
On 2012/03/12 17:48:24, satorux1 wrote:
> Please also document if |dest_file_path| can be a directory. "cp" command
allows
> the destination to be a directory (ex. cp /etc/hosts /tmp"), but here, we
don't
> allow it, right?

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:131: // of the file system.
On 2012/03/12 17:48:24, satorux1 wrote:
> //
> // Can be called from any thread.
> 
> ?

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.h:139: // of the file system.
On 2012/03/12 17:48:24, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system_proxy.cc:38: entry.name =
file.file_name();
On 2012/03/12 20:46:46, zel wrote:
> aren't you supposed to append GDataFileBase::document_extension_ to hosted
> documents here? 

GDataFile::file_name_ should already include the document extension. This is
done in:

void GDataFile::UseOriginalFileName() {
  if (is_hosted_document_) {
    file_name_ = EscapeUtf8FileName(original_file_name_ + document_extension_);
  } else {
    ...
  }
}

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:321: virtual void
FileOperationCallback(base::PlatformFileError error) {
On 2012/03/12 17:48:24, satorux1 wrote:
> I think we should add a DCHECK() to ensure that the callback is run on the
> origin thread.
> 
> DCHECK(CurrentlyOn(UI));

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.cc:149: file->original_file_name_ =
UTF16ToUTF8(doc->title());
On 2012/03/12 17:48:24, satorux1 wrote:
> this looks like an important change. It'd be nicer to change this separately
in
> a separate patch, rather than part of this large patch.

Agree. Let me split the CL.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.cc:149: file->original_file_name_ =
UTF16ToUTF8(doc->title());
On 2012/03/12 20:46:46, zel wrote:
> Is title required for regular (non-hosted) files too?

Yes, I noticed that both the web interface and the desktop client name files by
the 'title' attribute. The 'filename' attribute keeps the file name at the time
of upload. But renaming a file changes the 'title' attribute. Also, 'title'
allows name duplication.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.h:53: void
set_original_file_name(const FilePath::StringType& name) {
On 2012/03/12 17:48:24, satorux1 wrote:
> While you are at it, could you document the difference between |file_name| and
> |original_file_name| ?

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.h:57: void set_parent(GDataDirectory*
parent) { parent_ = parent; }
On 2012/03/12 17:48:24, satorux1 wrote:
> Looks scary. Let's move this to protected: section, so it won't be misused
from
> random client code.
> 
> The tree will be messed up if this function is used wrong. for instance:
> 
>   dest_dir->AddFile(file);
>   // what if we forget to call this?
>   // file->set_parent(dest_dir);

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.h:59: const std::string& resource()
const { return resource_id_; }
On 2012/03/12 17:48:24, satorux1 wrote:
> resource() -> resource_id().

Done.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_files.h:162: bool MoveFile(GDataFileBase*
file, GDataDirectory* dir);
On 2012/03/12 17:48:24, satorux1 wrote:
> Instead of this, what about adding a function like
> 
>   // Takes the file. The parent of |file| is changed to this directory.
>   bool TakeFile(GDataFileBase* file);
> 
> One less parameter would be nice.

Done.

[satorux1, 2012-03-13 01:12:17]

LGTM. might want to wait for zel's comments.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:661: // file manager.
On 2012/03/13 00:29:21, Ben Chan wrote:
> On 2012/03/12 17:48:24, satorux1 wrote:
> > How does this work?
> 
> The file manager recursively traverses the directory being copied and invokes
> FileEntry.copyTo on each file encountered. That way, the file manager can show
> progress of the copy operations. We will probably use the same approach for
> Gdata file system, regardless of whether or not directory copy being
implemented
> in GDataFileSystem::Copy().

thanks.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:680: callback);
On 2012/03/13 00:29:21, Ben Chan wrote:
> On 2012/03/12 17:48:24, satorux1 wrote:
> > Shouldn't we pass MessageLoopProxy::current() so |callback| is run on the
> origin
> > thread?
> 
> IIUC, the DocumentsService layer handles the transition between the origin
> thread and the UI thread. Rename(), AddResourceToDirectory(), and
> RemoveResourceFromDirectory() run on the same thread as Copy() and Move(), and
> they use MessageLoop::current(). Do we still need to pass
MessageLoop::current()
> as an argument to the callback?

Oh you are right. forget about my comments.

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

http://codereview.chromium.org/9662041/diff/18001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:775: //    directory of
|dest_file_path|.
Thank you for adding this!

[Ben Chan, 2012-03-13 02:16:36]

Removed CopyMoveParams/GetCopyMoveParams based on zel's suggestion.

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9662041/diff/8001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:632: return false;
On 2012/03/12 20:40:40, zel wrote:
> there is a helper method GetGDataFileInfoFromPath() that give you everything
> from 624-632. We just need to make it thread safe, I can show you what's wrong
> with it in other comments.

Done.

[zel, 2012-03-13 05:10:21]

lgtm

[commit-bot: I haz the power, 2012-03-13 08:40:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benchan@chromium.org/9662041/21010

[commit-bot: I haz the power, 2012-03-13 10:06:09]

Change committed as 126369