Implement SyncEngine::DumpFiles()

Implement SyncEngine::DumpFiles()

Implemented SyncEngine::DumpFiles and MetadataDatabaseTest.DumpFiles.

Dumps all the file trackers for the given app.

TEST=Automated. Unit test MetadataDatabaseTest.DumpFiles.
BUG=None.

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

[tzik, 2013-11-13 06:16:25]

Thanks for helping us.

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1536:
BuildPathForTracker(tracker->tracker_id(), &path);
This will fail if the tracker is inactive.
Could you handle error case?

I think either:
 - build path for parent tracker and add 'unknown' or synced_details().title()
if exists.
 - just skip inactive tracker.
will be fine.

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1538:
file->SetString("title", tracker->synced_details().title());
Newly added inactive tracker doesn't have synced_details.
Could you check has_synced_details() and set 'unknown' if unavailable (or leave
empty)?

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1544:
details->SetString("md5", tracker->synced_details().md5());
Could you put this only when synced_details()->file_kind() == FILE_KIND_FILE?
Also, please put 'active' entry, too.

[nhiroki, 2013-11-13 06:36:59]

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1500:
std::string FileKindToString(FileKind file_kind) {
How about moving this somewhere else (e.g. drive_backend_util.h) to reuse?

[keishi, 2013-11-13 07:10:59]

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1500:
std::string FileKindToString(FileKind file_kind) {
On 2013/11/13 06:36:59, nhiroki wrote:
> How about moving this somewhere else (e.g. drive_backend_util.h) to reuse?

Done.

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1536:
BuildPathForTracker(tracker->tracker_id(), &path);
On 2013/11/13 06:16:25, tzik wrote:
> This will fail if the tracker is inactive.
> Could you handle error case?
> 
> I think either:
>  - build path for parent tracker and add 'unknown' or synced_details().title()
> if exists.
>  - just skip inactive tracker.
> will be fine.

I'm guessing we'll want to know where inactive trackers are so I'm going with
your first proposal.

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1538:
file->SetString("title", tracker->synced_details().title());
On 2013/11/13 06:16:25, tzik wrote:
> Newly added inactive tracker doesn't have synced_details.
> Could you check has_synced_details() and set 'unknown' if unavailable (or
leave
> empty)?

Done.

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1544:
details->SetString("md5", tracker->synced_details().md5());
On 2013/11/13 06:16:25, tzik wrote:
> Could you put this only when synced_details()->file_kind() == FILE_KIND_FILE?
> Also, please put 'active' entry, too.

Done.

[tzik, 2013-11-13 07:27:43]

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/drive_backend_util.h (right):

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/drive_backend_util.h:46:
std::string FileKindToString(int file_kind);
s/int/FileKind/?

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1535:
tracker->synced_details().file_kind()));
Accessing synced_details() has a side effect if has_synced_details() is false.
Could you put 1532-1535 into <code>if (tracker->has_synced_details())</code>?

I think it's OK to leave these field unset when we don't have synced_details.

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1542:
tracker->synced_details().md5() : "");
Hmm, this is a bit hard to read.

Could you use <code>if (tracker->has_synced_details())</code> here?

[keishi, 2013-11-13 07:47:03]

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/drive_backend_util.h (right):

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/drive_backend_util.h:46:
std::string FileKindToString(int file_kind);
On 2013/11/13 07:27:44, tzik wrote:
> s/int/FileKind/?

Done.

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1535:
tracker->synced_details().file_kind()));
On 2013/11/13 07:27:44, tzik wrote:
> Accessing synced_details() has a side effect if has_synced_details() is false.
> Could you put 1532-1535 into <code>if (tracker->has_synced_details())</code>?
> 
> I think it's OK to leave these field unset when we don't have synced_details.

Done.

https://codereview.chromium.org/71183002/diff/90001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1542:
tracker->synced_details().md5() : "");
On 2013/11/13 07:27:44, tzik wrote:
> Hmm, this is a bit hard to read.
> 
> Could you use <code>if (tracker->has_synced_details())</code> here?

Done.

[nhiroki, 2013-11-13 08:50:04]

LGTM

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/drive_backend/metadata_database.cc (right):

https://codereview.chromium.org/71183002/diff/30001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/drive_backend/metadata_database.cc:1536:
BuildPathForTracker(tracker->tracker_id(), &path);
On 2013/11/13 07:10:59, keishi1 wrote:
> On 2013/11/13 06:16:25, tzik wrote:
> > This will fail if the tracker is inactive.
> > Could you handle error case?

(Not related to this CL) I think it'd be nice if we add comment about inactive
cases to BuildPathForTracker in the header file.

https://codereview.chromium.org/71183002/diff/140001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/drive_backend_util.h (right):

https://codereview.chromium.org/71183002/diff/140001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/drive_backend_util.h:42:
std::string FileKindToString(FileKind file_kind);
nit: Please add "#include <string>".

[keishi, 2013-11-13 09:00:18]

https://codereview.chromium.org/71183002/diff/140001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/drive_backend_util.h (right):

https://codereview.chromium.org/71183002/diff/140001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/drive_backend_util.h:42:
std::string FileKindToString(FileKind file_kind);
On 2013/11/13 08:50:05, nhiroki wrote:
> nit: Please add "#include <string>".

Done.

[tzik, 2013-11-13 09:27:50]

LGTM

[commit-bot: I haz the power, 2013-11-13 09:50:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/71183002/230001

[commit-bot: I haz the power, 2013-11-13 11:20:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-13 12:11:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/71183002/470001

[commit-bot: I haz the power, 2013-11-13 12:17:40]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-11-14 03:11:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/71183002/470001

[commit-bot: I haz the power, 2013-11-14 07:06:25]

Change committed as 235082