Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(9)

Issue 10444082: Refresh drive file system metadata for entries in search results. (Closed)

Created:
8 years, 6 months ago by tbarzic
Modified:
8 years, 6 months ago
Reviewers:
satorux1
CC:
chromium-reviews, achuith+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : comment #

Patch Set 4 : rebase #

Patch Set 5 : . #

Total comments: 8

Patch Set 6 : feedback #

Patch Set 7 : a nit #

Total comments: 2

Patch Set 8 : . #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -41 lines) Patch
M chrome/browser/chromeos/gdata/gdata_file_system.cc View 1 2 3 4 5 6 7 3 chunks +42 lines, -41 lines 0 comments Download
M chrome/browser/chromeos/gdata/gdata_files.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/gdata/gdata_files.cc View 1 2 3 4 5 6 7 1 chunk +15 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/gdata/gdata_files_unittest.cc View 1 2 3 4 5 6 7 1 chunk +35 lines, -0 lines 3 comments Download

Messages

Total messages: 8 (0 generated)
tbarzic
this should take care of thumbnails not showing for search results.
8 years, 6 months ago (2012-05-30 20:36:09 UTC) #1
satorux1
http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gdata/gdata_file_system.cc File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gdata/gdata_file_system.cc#newcode2919 chrome/browser/chromeos/gdata/gdata_file_system.cc:2919: iter != feed->entries().end(); ++iter) { nit: you can do ...
8 years, 6 months ago (2012-05-31 15:56:01 UTC) #2
tbarzic
http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gdata/gdata_file_system.cc File chrome/browser/chromeos/gdata/gdata_file_system.cc (right): http://codereview.chromium.org/10444082/diff/9001/chrome/browser/chromeos/gdata/gdata_file_system.cc#newcode2919 chrome/browser/chromeos/gdata/gdata_file_system.cc:2919: iter != feed->entries().end(); ++iter) { On 2012/05/31 15:56:01, satorux1 ...
8 years, 6 months ago (2012-05-31 19:25:17 UTC) #3
satorux1
http://codereview.chromium.org/10444082/diff/1005/chrome/browser/chromeos/gdata/gdata_files.h File chrome/browser/chromeos/gdata/gdata_files.h (right): http://codereview.chromium.org/10444082/diff/1005/chrome/browser/chromeos/gdata/gdata_files.h#newcode384 chrome/browser/chromeos/gdata/gdata_files.h:384: void RefreshFile(scoped_ptr<GDataEntry> fresh_entry); scoped_ptr<GDataFile> ?
8 years, 6 months ago (2012-05-31 19:41:07 UTC) #4
tbarzic
https://chromiumcodereview.appspot.com/10444082/diff/1005/chrome/browser/chromeos/gdata/gdata_files.h File chrome/browser/chromeos/gdata/gdata_files.h (right): https://chromiumcodereview.appspot.com/10444082/diff/1005/chrome/browser/chromeos/gdata/gdata_files.h#newcode384 chrome/browser/chromeos/gdata/gdata_files.h:384: void RefreshFile(scoped_ptr<GDataEntry> fresh_entry); On 2012/05/31 19:41:07, satorux1 wrote: > ...
8 years, 6 months ago (2012-05-31 20:03:53 UTC) #5
satorux1
LGTM with a request: http://codereview.chromium.org/10444082/diff/5005/chrome/browser/chromeos/gdata/gdata_files_unittest.cc File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right): http://codereview.chromium.org/10444082/diff/5005/chrome/browser/chromeos/gdata/gdata_files_unittest.cc#newcode83 chrome/browser/chromeos/gdata/gdata_files_unittest.cc:83: initial_file_entry->set_resource_id("file:file_resource_id"); it's a bit more ...
8 years, 6 months ago (2012-05-31 20:36:17 UTC) #6
tbarzic
https://chromiumcodereview.appspot.com/10444082/diff/5005/chrome/browser/chromeos/gdata/gdata_files_unittest.cc File chrome/browser/chromeos/gdata/gdata_files_unittest.cc (right): https://chromiumcodereview.appspot.com/10444082/diff/5005/chrome/browser/chromeos/gdata/gdata_files_unittest.cc#newcode83 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 ...
8 years, 6 months ago (2012-05-31 20:56:33 UTC) #7
satorux1
8 years, 6 months ago (2012-05-31 21:44:17 UTC) #8
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

Powered by Google App Engine
This is Rietveld 408576698