Pass parsed ChangeList to GDataFileSystem.

Pass parsed ChangeList to GDataFileSystem.

Get ChangeList using Drive V2 API and then converts to
DocumentFeed.  This is not optimal but change can be minimal
to make file list appear in FileBrowser UI, which makes
other development/debugging/refactoring easier.

This depends on the change in
https://chromiumcodereview.appspot.com/10837201/
for GetKind() function.

BUG=chromium:127728
TEST=specify "--enable-drive-v2-api" and open file browser (Ctrl-M), then go to drive folder and see if file listing works.

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

[kochi, 2012-08-10 09:44:20]

Hi Satoru,

As I understand it is not an optimal to convert Drive API
ChangeList into WAPI DocumentFeed, then convert to proto,
but the change is getting very large.

So I thought check this small change in first, then work on
directly converting to GDataEntry would make reviewer's life
easier.

What do you think?

[satorux1, 2012-08-10 18:14:34]

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/drive_api_parser.cc (right):

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.cc:461: return
DocumentEntry::FILE;
I think the changes in drive_api_parser.h/cc can be submitted separately in a
smaller patch.

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/drive_api_parser.h (right):

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:18: #include
"chrome/browser/chromeos/gdata/gdata_wapi_parser.h"
This dependency is worrisome. This is for DocumentEntry::EntryKind, right? This
sounds bad, as DocumentEntry will eventually be gone once the migration is done.

We should probably move EntryKind out of DocumentEntry, and put it in a separate
header file like parser_constants.h

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (right):

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:446: DVLOG(1) <<
"LoadFeedFromServer";
remove this?

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_wapi_parser.cc (right):

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_parser.cc:960: scoped_ptr<DocumentFeed>
DocumentFeed::CreateFromFileList(
This doesn't seem to be used.

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_wapi_parser.h (right):

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_parser.h:379: // Creates document entry
from FileResource.
So you plan to remove this right? Please file a bug and add a TODO here.

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_parser.h:383: static DocumentEntry*
CreateFromChangeResource(const ChangeResource& change);
ditto.

http://codereview.chromium.org/10837201/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_parser.h:526: static
scoped_ptr<DocumentFeed> CreateFromFileList(const FileList& filelist);
ditto.

[kochi, 2012-08-13 09:08:39]

Thanks for the review!

I split the drive_api_parser part in a separate CL:
https://chromiumcodereview.appspot.com/10828277/

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/drive_api_parser.cc (right):

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_api_parser.cc:461: return
DocumentEntry::FILE;
On 2012/08/10 18:14:34, satorux1 wrote:
> I think the changes in drive_api_parser.h/cc can be submitted separately in a
> smaller patch.

Done.
https://chromiumcodereview.appspot.com/10828277/

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/drive_api_parser.h (right):

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/drive_api_parser.h:18: #include
"chrome/browser/chromeos/gdata/gdata_wapi_parser.h"
On 2012/08/10 18:14:34, satorux1 wrote:
> This dependency is worrisome. This is for DocumentEntry::EntryKind, right?
This
> sounds bad, as DocumentEntry will eventually be gone once the migration is
done.
> 
> We should probably move EntryKind out of DocumentEntry, and put it in a
separate
> header file like parser_constants.h

Once migration is done, EntryKind will not necessary and
parser_constants.h will be empty.  Drive API parser is the
only parser in this directory, drive_api_parser.h is enough
and no reason to have a separate constant definition header.

Even before complete migration to Drive API, once we directly convert to proto,
this isn't necessary.

So I added TODO to remove this dependency.  The same
as the comment for GetKind().

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (right):

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:446: DVLOG(1) <<
"LoadFeedFromServer";
On 2012/08/10 18:14:34, satorux1 wrote:
> remove this?

Done.

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_wapi_parser.cc (right):

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_parser.cc:960: scoped_ptr<DocumentFeed>
DocumentFeed::CreateFromFileList(
Removed.

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_wapi_parser.h (right):

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_parser.h:379: // Creates document entry
from FileResource.
On 2012/08/10 18:14:34, satorux1 wrote:
> So you plan to remove this right? Please file a bug and add a TODO here.

Done.

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_parser.h:383: static DocumentEntry*
CreateFromChangeResource(const ChangeResource& change);
On 2012/08/10 18:14:34, satorux1 wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/10837201/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_parser.h:526: static
scoped_ptr<DocumentFeed> CreateFromFileList(const FileList& filelist);
On 2012/08/10 18:14:34, satorux1 wrote:
> ditto.

Done.

[satorux1, 2012-08-13 17:05:43]

http://codereview.chromium.org/10837201/diff/7002/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (right):

http://codereview.chromium.org/10837201/diff/7002/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:763: }
In OnGetDocuments, OnNotifyDocumentFeedFetched() is called, but
OnNotifyDocumentFeedFetched() is not called in this function. I think this is
wrong.

OnNotifyDocumentFeedFetched() is needed to report the fetch progress more
smoothly. Please copy this behavior.

[kochi, 2012-08-14 02:09:19]

https://chromiumcodereview.appspot.com/10837201/diff/7002/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (right):

https://chromiumcodereview.appspot.com/10837201/diff/7002/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:763: }
On 2012/08/13 17:05:43, satorux1 wrote:
> In OnGetDocuments, OnNotifyDocumentFeedFetched() is called, but
> OnNotifyDocumentFeedFetched() is not called in this function. I think this is
> wrong.
> 
> OnNotifyDocumentFeedFetched() is needed to report the fetch progress more
> smoothly. Please copy this behavior.

Done.

[satorux1, 2012-08-14 06:02:45]

http://codereview.chromium.org/10837201/diff/2010/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (right):

http://codereview.chromium.org/10837201/diff/2010/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:343: if
(!callback.is_null()) {
BTW, could you check if it's safe to remove this check? If all callers of the
class are always passing non-null callback, we can eliminate checks like this,
and instead add DCHECK(!callback.is_null()). Please do this in a separate patch.

See also http://crbug.com/126634

[satorux1, 2012-08-14 06:05:25]

Please update the TEST= line and describe how to test this feature. You also
tested the progress update part you added in the last patch set, right?

LGTM.

[kochi, 2012-08-14 07:16:55]

Updated the description.

For checking callback.is_null, I'll work on another CL.

https://chromiumcodereview.appspot.com/10837201/diff/2010/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (right):

https://chromiumcodereview.appspot.com/10837201/diff/2010/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:343: if
(!callback.is_null()) {
Ok, I'll examine these checks in a separate CL.

On 2012/08/14 06:02:45, satorux1 wrote:
> BTW, could you check if it's safe to remove this check? If all callers of the
> class are always passing non-null callback, we can eliminate checks like this,
> and instead add DCHECK(!callback.is_null()). Please do this in a separate
patch.
> 
> See also http://crbug.com/126634

[commit-bot: I haz the power, 2012-08-14 07:22:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/10837201/2010

[commit-bot: I haz the power, 2012-08-14 09:46:58]

Change committed as 151445

[satorux1, 2012-08-15 00:57:31]

On Tue, Aug 14, 2012 at 12:16 AM, <kochi@chromium.org> wrote:


> Updated the description.

> For checking callback.is_null, I'll work on another CL.


Please don't do this. I've already done that:
http://codereview.chromium.org/10824303/





> https://chromiumcodereview.**appspot.com/10837201/diff/**
>
2010/chrome/browser/chromeos/**gdata/gdata_wapi_feed_loader.**cc<https://chromiumcodereview.appspot.com/10837201/diff/2010/chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc>
> File chrome/browser/chromeos/gdata/**gdata_wapi_feed_loader.cc (right):

> https://chromiumcodereview.**appspot.com/10837201/diff/**
> 2010/chrome/browser/chromeos/**gdata/gdata_wapi_feed_loader.**
>
cc#newcode343<https://chromiumcodereview.appspot.com/10837201/diff/2010/chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc#newcode343>
> chrome/browser/chromeos/gdata/**gdata_wapi_feed_loader.cc:343: if
> (!callback.is_null()) {
> Ok, I'll examine these checks in a separate CL.


> On 2012/08/14 06:02:45, satorux1 wrote:

>> BTW, could you check if it's safe to remove this check? If all callers

> of the

>> class are always passing non-null callback, we can eliminate checks

> like this,

>> and instead add DCHECK(!callback.is_null()). Please do this in a

> separate patch.

>   See also http://crbug.com/126634


>
https://chromiumcodereview.**appspot.com/10837201/<https://chromiumcodereview...