Copy folder recursively for syncfsToLocalfs export.

Copy folder recursively for syncfsToLocalfs export.

Exporting syncfs folder to localfs should copy the folder recursively.

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

[miket_OOO, 2013-06-12 19:39:24]

https://codereview.chromium.org/16402016/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/16402016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:873:
pendingCallbacksCount_ += file_list.size();
This is pretty confusing. You're adding N to pendingCallbacksCount_, but only
decrementing it once later in this method. There seems to be some kind of
recursion going on, but it's not at all obvious how it works.

Can you add more comments to explain it?

https://codereview.chromium.org/16402016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:879:
continue;
This is a good example of why it's confusing. It seems like you're adding the
whole file list size, but then doing the recursive call (the only thing that
subtracts from pendingCallbacksCount_) only once for each directory. What if
something in the list isn't a directory? Won't pendingCallbacksCount_ stay
positive forever? Code comments here would help.

https://codereview.chromium.org/16402016/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):

https://codereview.chromium.org/16402016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/developer_private/developer_private_api.h:355:
const base::FilePath& current_path);
"current_path" has special meaning: it's the current working directory. For that
reason it's surprising to see a parameter named current_path (anyone can ask
what the current path is, so why would you send it around?). I haven't seen the
.cc yet, so I can't say for sure, but I bet that it's something more like
"destination_path." Recommend changing for clarity.

[Gaurav, 2013-06-21 22:11:00]

Sorry About the long delay. The syncFileSystem was broken to test it :|

https://codereview.chromium.org/16402016/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/16402016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:873:
pendingCallbacksCount_ += file_list.size();
On 2013/06/12 19:39:24, miket wrote:
> This is pretty confusing. You're adding N to pendingCallbacksCount_, but only
> decrementing it once later in this method. There seems to be some kind of
> recursion going on, but it's not at all obvious how it works.
> 
> Can you add more comments to explain it?

Done.

https://codereview.chromium.org/16402016/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:879:
continue;
On 2013/06/12 19:39:24, miket wrote:
> This is a good example of why it's confusing. It seems like you're adding the
> whole file list size, but then doing the recursive call (the only thing that
> subtracts from pendingCallbacksCount_) only once for each directory. What if
> something in the list isn't a directory? Won't pendingCallbacksCount_ stay
> positive forever? Code comments here would help.

Done.

[miket_OOO, 2013-06-25 16:44:23]

lgtm

https://codereview.chromium.org/16402016/diff/6001/chrome/browser/extensions/...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):

https://codereview.chromium.org/16402016/diff/6001/chrome/browser/extensions/...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:881: //
operations of files are released by the CopyFile function
period at end of sentence.

[commit-bot: I haz the power, 2013-06-25 21:45:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grv@chromium.org/16402016/12001

[commit-bot: I haz the power, 2013-06-26 02:09:01]

Change committed as 208594