Refresh drive file system metadata for entries in search results.

Refresh drive file system metadata for entries in search results.

Metadata received in serach result feed is fresher than the one we have cached.

BUG=129071
TEST=existing gdata tests

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

[tbarzic, 2012-05-30 20:36:09]

this should take care of thumbnails not showing for search results.

[satorux1, 2012-05-31 15:56:01]

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

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2919: iter !=
feed->entries().end(); ++iter) {
nit: you can do 

for (size_t i = 0; i < feed->entries().size(); ++i) {

unless you like iterators.

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2931:
GDataEntry::FromDocumentEntry(NULL, doc, root_.get()));
Could you add some comment why you have to create another instance of the entry
here? You cannot use |entry| here, right?

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:556:
DCHECK(old_entry->AsGDataFile());
here, we assume |fresh_entry| to be a file. The function name should probably be
changed to RefreshFile().

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:384: void
RefreshEntry(scoped_ptr<GDataEntry> fresh_entry);
can you add a unit test for this in gdata_files_unittest.cc?

[tbarzic, 2012-05-31 19:25:17]

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

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2919: iter !=
feed->entries().end(); ++iter) {
On 2012/05/31 15:56:01, satorux1 wrote:
> nit: you can do 
> 
> for (size_t i = 0; i < feed->entries().size(); ++i) {
> 
> unless you like iterators.

I can't say they're my favorites..

Done.

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2931:
GDataEntry::FromDocumentEntry(NULL, doc, root_.get()));
On 2012/05/31 15:56:01, satorux1 wrote:
> Could you add some comment why you have to create another instance of the
entry
> here? You cannot use |entry| here, right?

Done.

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:556:
DCHECK(old_entry->AsGDataFile());
On 2012/05/31 15:56:01, satorux1 wrote:
> here, we assume |fresh_entry| to be a file. The function name should probably
be
> changed to RefreshFile(). 

Done.

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:384: void
RefreshEntry(scoped_ptr<GDataEntry> fresh_entry);
On 2012/05/31 15:56:01, satorux1 wrote:
> can you add a unit test for this in gdata_files_unittest.cc?

Done.

[satorux1, 2012-05-31 19:41:07]

http://codereview.chromium.org/10444082/diff/1005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

http://codereview.chromium.org/10444082/diff/1005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.h:384: void
RefreshFile(scoped_ptr<GDataEntry> fresh_entry);
scoped_ptr<GDataFile> ?

[tbarzic, 2012-05-31 20:03:53]

https://chromiumcodereview.appspot.com/10444082/diff/1005/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_files.h (right):

https://chromiumcodereview.appspot.com/10444082/diff/1005/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_files.h:384: void
RefreshFile(scoped_ptr<GDataEntry> fresh_entry);
On 2012/05/31 19:41:07, satorux1 wrote:
> scoped_ptr<GDataFile> ?

Done.

[satorux1, 2012-05-31 20:36:17]

LGTM with a request:

http://codereview.chromium.org/10444082/diff/5005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

http://codereview.chromium.org/10444082/diff/5005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:83:
initial_file_entry->set_resource_id("file:file_resource_id");
it's a bit more interesting to set a thumbnail URL, and replace it with the new
file entry.

[tbarzic, 2012-05-31 20:56:33]

https://chromiumcodereview.appspot.com/10444082/diff/5005/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

https://chromiumcodereview.appspot.com/10444082/diff/5005/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:83:
initial_file_entry->set_resource_id("file:file_resource_id");
On 2012/05/31 20:36:18, satorux1 wrote:
> it's a bit more interesting to set a thumbnail URL, and replace it with the
new
> file entry.

I don't see why. We test that, after RefreshEntry, root directory returns
new_file_entry when we query for "file:file_resource_id". So, all metadata (not
just thumbnail url) we get from the file system for the given resource id will
come from the new entry.

I'm not sure that we have to check that thumbnail url specifically has changed.
Or am I missing something?

[satorux1, 2012-05-31 21:44:17]

LGTM. forget about my last comment.

https://chromiumcodereview.appspot.com/10444082/diff/5005/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right):

https://chromiumcodereview.appspot.com/10444082/diff/5005/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_files_unittest.cc:83:
initial_file_entry->set_resource_id("file:file_resource_id");
On 2012/05/31 20:56:33, tbarzic wrote:
> On 2012/05/31 20:36:18, satorux1 wrote:
> > it's a bit more interesting to set a thumbnail URL, and replace it with the
> new
> > file entry.
> 
> I don't see why. We test that, after RefreshEntry, root directory returns
> new_file_entry when we query for "file:file_resource_id". So, all metadata
(not
> just thumbnail url) we get from the file system for the given resource id will
> come from the new entry.
> 
> I'm not sure that we have to check that thumbnail url specifically has
changed.
> Or am I missing something?

You confirm that the file is replaced by:

ASSERT_EQ(new_file_entry, root.GetEntryByResourceId("file:file_resource_id"));

This is comparing a pointer, which is good enough, but I'm not good at
understanding abstract things like pointers, so I thought it'd be easier to
understand if we also compare thumbnail URLs. However, I think it's unnecessary