gdata: File info of dirty files should be get from the cached file.

gdata: File info of dirty files should be get from the cached file.

This CL is the part 2 (of 2) for fixing 138512.
In GDataFileSystem::GetEntry*() family of functions, check the
existence of dirty cache and if there exists, use the file
info from the cache file.

BUG=138512
TEST=unit_tests --gtest_filter='*GData*'
TEST=Manual:
- edit a file by photo editor and save as a different file.
- verify the file size and the date is displayed correctly for the edited file even if feed update intervenes.

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

[satorux1, 2012-07-25 19:28:45]

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

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4133: void
GDataFileSystem::ReflectLocalModificationToGDataEntryProto(
maybe CheckLocalModificationAndRun() would describe what the function does a bit
more nicely.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4135:
scoped_ptr<GDataEntryProto> entry_proto) {
I'd swap these parameters with the new function name.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4136:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
DCHECK(entry_proto.get()); This should be guaranteed by callers, right?

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4139: if (!entry_proto.get()
|| !entry_proto->has_file_specific_info() ||
I'd remove !entry_proto.get()

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4140:
entry_proto->file_specific_info().file_md5().empty() ||
I think we can remove this line.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4157: void
GDataFileSystem::OnGetCacheEntryForReflectLocalModification(
ReflectLocalModificationAfterGetCacheEntry() may be easier to read. I started
using this notation recently to implement a set of functions which logically
implement one function. I found it a bit easier to follow.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4173: const std::string md5 =
entry_proto->file_specific_info().file_md5();
make them const reference?

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4228:
callback.Run(GDATA_FILE_OK, entry_proto.Pass());
This looks wrong. The error code should be GDATA_FILE_NOT_FOUND if
*get_file_info_result == false.

[kinaba, 2012-07-26 03:17:52]

Thanks for the review!

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

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4133: void
GDataFileSystem::ReflectLocalModificationToGDataEntryProto(
On 2012/07/25 19:28:45, satorux1 wrote:
> maybe CheckLocalModificationAndRun() would describe what the function does a
bit
> more nicely.

Done.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4135:
scoped_ptr<GDataEntryProto> entry_proto) {
On 2012/07/25 19:28:45, satorux1 wrote:
> I'd swap these parameters with the new function name.

Done.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4136:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/07/25 19:28:45, satorux1 wrote:
> DCHECK(entry_proto.get()); This should be guaranteed by callers, right?

Yes. Done.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4139: if (!entry_proto.get()
|| !entry_proto->has_file_specific_info() ||
On 2012/07/25 19:28:45, satorux1 wrote:
> I'd remove !entry_proto.get()

Done.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4140:
entry_proto->file_specific_info().file_md5().empty() ||
On 2012/07/25 19:28:45, satorux1 wrote:
> I think we can remove this line.

Done.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4157: void
GDataFileSystem::OnGetCacheEntryForReflectLocalModification(
On 2012/07/25 19:28:45, satorux1 wrote:
> ReflectLocalModificationAfterGetCacheEntry() may be easier to read. I started
> using this notation recently to implement a set of functions which logically
> implement one function. I found it a bit easier to follow.

Done.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4173: const std::string md5 =
entry_proto->file_specific_info().file_md5();
On 2012/07/25 19:28:45, satorux1 wrote:
> make them const reference?

Done.

http://codereview.chromium.org/10826006/diff/1001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4228:
callback.Run(GDATA_FILE_OK, entry_proto.Pass());
On 2012/07/25 19:28:45, satorux1 wrote:
> This looks wrong. The error code should be GDATA_FILE_NOT_FOUND if
> *get_file_info_result == false.

Done.

[satorux1, 2012-07-26 12:08:14]

LGTM with nits

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

http://codereview.chromium.org/10826006/diff/1003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4204:
base::Bind(&GDataFileSystem::ReflectLocalModificationAfterGetCacheEntry,
CheckLocalModificationAndRunAfterGetCacheEntry? because the function was renamed

http://codereview.chromium.org/10826006/diff/1003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4232: void
GDataFileSystem::ReflectLocalModificationAfterGetCacheFile(
ditto

http://codereview.chromium.org/10826006/diff/1003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4266: void
GDataFileSystem::ReflectLocalModificationAfterGetFileInfo(
ditto

[kinaba, 2012-07-27 01:47:44]

Done.

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

http://codereview.chromium.org/10826006/diff/1003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4204:
base::Bind(&GDataFileSystem::ReflectLocalModificationAfterGetCacheEntry,
On 2012/07/26 12:08:14, satorux1 wrote:
> CheckLocalModificationAndRunAfterGetCacheEntry? because the function was
renamed

Done.

http://codereview.chromium.org/10826006/diff/1003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4232: void
GDataFileSystem::ReflectLocalModificationAfterGetCacheFile(
On 2012/07/26 12:08:14, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/10826006/diff/1003/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:4266: void
GDataFileSystem::ReflectLocalModificationAfterGetFileInfo(
On 2012/07/26 12:08:14, satorux1 wrote:
> ditto

Done.

[commit-bot: I haz the power, 2012-07-27 02:01:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinaba@chromium.org/10826006/3003

[commit-bot: I haz the power, 2012-07-27 03:15:23]

Change committed as 148712