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

chrome/browser/sync_file_system/drive_file_sync_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync_file_system/drive_file_sync_service.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 752 matching lines @@
   typedef ScopedVector<google_apis::ResourceEntry>::const_iterator iterator;
   for (iterator itr = feed->entries().begin();
        itr != feed->entries().end(); ++itr) {
     const google_apis::ResourceEntry& entry = **itr;
     AppendRemoteChange(origin, entry, largest_changestamp,
                        RemoteChangeHandler::REMOTE_SYNC_TYPE_BATCH);
 
     // 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.

 
     base::FilePath path = TitleToPath(entry.title());
     fileapi::FileSystemURL url(CreateSyncableFileSystemURL(
         origin, kServiceName, path));
     // TODO(calvinlo): Write metadata and origin data as single batch command
     // so it's not possible for the DB to contain a DriveMetadata with an
     // unknown origin.
     metadata_store_->UpdateEntry(url, metadata,
                                  base::Bind(&EmptyStatusCallback));
   }
@@ skipping 948 matching lines @@
     metadata_store_->SetOriginRootDirectory(origin, resource_id);
   }
   callback.Run(status, resource_id);
 }
 
 std::string DriveFileSyncService::sync_root_resource_id() {
   return metadata_store_->sync_root_directory();
 }
 
 }  // namespace sync_file_system