Added logic that will return the result of the first chunk of root feed and continue fetching the r…

Added logic that will return the result of the first chunk of root feed and continue fetching the rest in background. This logic will kick in only if we don't have cached feed in place already.

This CL also fixes a minor link parser problem and makes /gdata watchable by file manager. The root directory also does not contain any orphaned/shared content now.


BUG=chromium-os:28381, chromium-os:28382
TEST=Manual: make sure /gdata root content shows up pretty fast even for new accounts and that we refresh it later with the rest. Unit: Added orphaned file checks to GDataFileSystemTest.CachedFeedLoading.

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

[zel, 2012-03-24 23:42:02]

working on unit test for orphaned feed elements...

[satorux1, 2012-03-25 00:22:26]

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1458: bool inital_read =
false;
nit: initial

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1467: if (!has_more_data ||
inital_read) {
nit: make the order consistent with the code above:

 if (inital_read || !has_more_data)

Please also add some comment that why we are reporting to the callback when
initial_read is true. I know the intent, but it's not obvious from the code
alone.

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1493: // uninitialized root
feed. Instead, just continue with entire feed fetch
// uninitialized root feed.
->
// uninitialized root feed, because we already reported.

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1496: inital_read ?
FindFileCallback() : callback;
So we won't call the callback when the last chunk arrives. I found it a bit
confusing, so please explain what's gonna happen in the end, and why it's ok.

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1999:
scoped_ptr<GDataRootDirectory> orphans(new GDataRootDirectory(NULL));
This part doesn't seem to be related to the directory refresh business. Is this
just a general bug fix? If so, would be nice to file a bug and add BUG= to it,
for the record.

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2009: DVLOG(1) << "Found
orphaned kitten file '" << file->file_name()
Does "kitten file" mean files shared by someone else, that we were talking
about? Please add some comment.

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2027: // |file| instance is
gone now, deleted by either this scoped_ptr (orphans)
I'm confused. |orphans| doesn't seem to be used to collect orphans.

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2030: // add themselves as
chikdren.
children

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:3475:
console.log(event);
remove the debug logging?

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/main.html (left):

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:9: <if expr="0">
you plan to get <if expr="0"> back before submission, right?

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:36: <!-- metrics.js initiates
load performance tracking
ditto.

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/main.html (right):

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/main.html:31: localStorage.base =
'http://zel-linux.mtv.corp.google.com:1080/file_manager/'
remove

[zel, 2012-03-25 02:06:41]

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1458: bool inital_read =
false;
On 2012/03/25 00:22:26, satorux1 wrote:
> nit: initial

Done.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1467: if (!has_more_data ||
inital_read) {
On 2012/03/25 00:22:26, satorux1 wrote:
> nit: make the order consistent with the code above:
> 
>  if (inital_read || !has_more_data)
> 
> Please also add some comment that why we are reporting to the callback when
> initial_read is true. I know the intent, but it's not obvious from the code
> alone.

Done.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1493: // uninitialized root
feed. Instead, just continue with entire feed fetch
On 2012/03/25 00:22:26, satorux1 wrote:
> // uninitialized root feed.
> ->
> // uninitialized root feed, because we already reported.

Done.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1496: inital_read ?
FindFileCallback() : callback;
On 2012/03/25 00:22:26, satorux1 wrote:
> So we won't call the callback when the last chunk arrives. I found it a bit
> confusing, so please explain what's gonna happen in the end, and why it's ok.

The callback that returns results of find operation (i.e. directory content
read) will be triggered as soon as we have something to show. This could be when
we read the metadata file from the cache or when the first batch of content feed
arrives over the net. In both cases, if there is more feed data to be fetched to
complete the accurate picture about how FS really looks like, we will continue
that in the background. When the final feed fetching finishes, we will raise
signal directory change notification and client will be able to refresh their
views.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1999:
scoped_ptr<GDataRootDirectory> orphans(new GDataRootDirectory(NULL));
On 2012/03/25 00:22:26, satorux1 wrote:
> This part doesn't seem to be related to the directory refresh business. Is
this
> just a general bug fix? If so, would be nice to file a bug and add BUG= to it,
> for the record.

Done.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2009: DVLOG(1) << "Found
orphaned kitten file '" << file->file_name()
On 2012/03/25 00:22:26, satorux1 wrote:
> Does "kitten file" mean files shared by someone else, that we were talking
> about? Please add some comment.

These are files that have parents that don't belong to the current user. Shared
is one example, might be others. In Kan's case, these are really kitten images
:)

...removed 'kitten' part.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2027: // |file| instance is
gone now, deleted by either this scoped_ptr (orphans)
On 2012/03/25 00:22:26, satorux1 wrote:
> I'm confused. |orphans| doesn't seem to be used to collect orphans.

orphans was supposed to be removed. done.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2030: // add themselves as
chikdren.
On 2012/03/25 00:22:26, satorux1 wrote:
> children

Done.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/resou...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/resou...
chrome/browser/resources/file_manager/js/file_manager.js:3475:
console.log(event);
On 2012/03/25 00:22:26, satorux1 wrote:
> remove the debug logging?

Done.

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/resou...
File chrome/browser/resources/file_manager/main.html (left):

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/resou...
chrome/browser/resources/file_manager/main.html:9: <if expr="0">
On 2012/03/25 00:22:26, satorux1 wrote:
> you plan to get <if expr="0"> back before submission, right?

reverted

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/resou...
chrome/browser/resources/file_manager/main.html:36: <!-- metrics.js initiates
load performance tracking
On 2012/03/25 00:22:26, satorux1 wrote:
> ditto.

reverted

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/resou...
File chrome/browser/resources/file_manager/main.html (right):

https://chromiumcodereview.appspot.com/9836086/diff/2001/chrome/browser/resou...
chrome/browser/resources/file_manager/main.html:31: localStorage.base =
'http://zel-linux.mtv.corp.google.com:1080/file_manager/'
On 2012/03/25 00:22:26, satorux1 wrote:
> remove

reverted

[Ben Chan, 2012-03-25 02:21:32]

lgtm

[zel, 2012-03-25 03:22:10]

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9836086/diff/2001/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1999:
scoped_ptr<GDataRootDirectory> orphans(new GDataRootDirectory(NULL));
On 2012/03/25 02:06:41, zel wrote:
> On 2012/03/25 00:22:26, satorux1 wrote:
> > This part doesn't seem to be related to the directory refresh business. Is
> this
> > just a general bug fix? If so, would be nice to file a bug and add BUG= to
it,
> > for the record.
> 
> Done.

My unit tests convinced me that this on was still needed at the end to do the
proper cleanup. It's wired properly in the loop below.

[satorux1, 2012-03-25 17:28:27]

LGTM

http://codereview.chromium.org/9836086/diff/2008/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9836086/diff/2008/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1469: // batch of content
feed, we might want to update |root_| content and report
nit: to make it very clear, we might want to describe the motivation here.

initial batch of content feed so that we can show the initial set of files to
the user as soon as the first chunk arrives, rather than waiting for the last
chunk to arrive (this can take quite some time if the user has 25K files for
instance).

[zel, 2012-03-25 17:39:20]

http://codereview.chromium.org/9836086/diff/2008/chrome/browser/chromeos/gdat...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://codereview.chromium.org/9836086/diff/2008/chrome/browser/chromeos/gdat...
chrome/browser/chromeos/gdata/gdata_file_system.cc:1469: // batch of content
feed, we might want to update |root_| content and report
On 2012/03/25 17:28:27, satorux1 wrote:
> nit: to make it very clear, we might want to describe the motivation here.
> 
> initial batch of content feed so that we can show the initial set of files to
> the user as soon as the first chunk arrives, rather than waiting for the last
> chunk to arrive (this can take quite some time if the user has 25K files for
> instance).

Done.