[SyncFileSystem] Push metadata with empty MD5 for to_be_fetched file.

chrome/browser/sync_file_system/drive_file_sync_service.cc

Index: chrome/browser/sync_file_system/drive_file_sync_service.cc
diff --git a/chrome/browser/sync_file_system/drive_file_sync_service.cc b/chrome/browser/sync_file_system/drive_file_sync_service.cc
index ee21075fe2aa6a9d1d09226a570d67cb995a1daa..b26eaa136f6143cf8f6237233c73e847c046ed19 100644
--- a/chrome/browser/sync_file_system/drive_file_sync_service.cc
+++ b/chrome/browser/sync_file_system/drive_file_sync_service.cc
@@ -770,7 +770,7 @@ void DriveFileSyncService::DidGetDirectoryContentForBatchSync(
     // Save to be fetched file to DB for restore in case of crash.
     DriveMetadata metadata;
     metadata.set_resource_id(entry.resource_id());
-    metadata.set_md5_checksum(entry.file_md5());
+    metadata.set_md5_checksum(std::string());

[calvinlo, 2013/05/29 06:39:00]

While this change works, I think it's honestly quite hacky. It forces the coder
to know that inside api_util L696 that there's an MD5 check that returns early
when it thinks the contents on drive and local match.

Instead of trying to workaround this if condition by setting the md5_checksum to
be empty here, I would suggest adding an extra param to DownloadFile. something
like bool (to_be_fetched), or (initial_download), (force_download), etc. Or as
an alternative, have two versions of the function, one that does the MD5 check
and another which doesn't.

Then go back and change DriveFileSyncService L1015 to use the value of
to_be_fetched.

I think if you do this, you'll also be able to remove lines 1008-1013. 

[tzik, 2013/05/29 07:08:11]

This does not need .cc impl, but needs the documentation on
api_util_interface.h.
And, I think we all should know the function we call.

Adding another flag to DownloadFile in this case will needs another
to_be_fetched flag handling in the download function.
That looks not cleaner to me.

The md5_checksum should hold the md5 value of the file we fetched last time.
Which we should detects the remote change.
So, I believe, setting this value to empty string is not so hacky.

[calvinlo, 2013/05/29 07:50:09]

I'm not sure that I agree. Right now the function is called DownloadFile but it
doesn't give you an indication that it will actually not download the file if
the given md5 matches the existing one.

A suggestion, you can make one function called
DownloadFile() which doesn't take in the MD5 and always downloads the file. Then
add another DownloadFileIfUpdated which does take in the MD5. The function names
and parameters make the code self documenting.

     metadata.set_conflicted(false);
     metadata.set_to_be_fetched(true);

[kinuko, 2013/05/29 06:34:28]

Question... if to_be_fetched==true should md5_checksum always empty? Then maybe
we can have additional check somewhere to enforce it?

[tzik, 2013/05/29 06:41:53]

Yes, we should check it.
I think the best places are in DriveMetadataStore::{ReadEntry,UpdateEntry}. 

[calvinlo, 2013/05/29 07:50:09]

If you go this route, I think it'd also be a good idea to note this in the
protobuf. i.e. md5_checksum is only populated once the file has been downloaded
and if it's empty then to_be_fetched must be true.