Use Drive v2 API: enable behind --enable-drive-api flag

Use Drive v2 API: enable behind --enable-drive-api flag

This only converts GetAccountMetadata to use Drive V2 API.
The Document List API returns InstalledApp list in the
account metadata feed, but Drive API has separate API and
that part is not implemented yet.

More conversions, tests to follow.

BUG=chromium:127728
TEST=not break all unittests

[kochi, 2012-07-09 10:45:33]

Hi Satoru,

This is the first attempt to migrate and use Drive v2 API.
I'm not still clear about the rest (especially about
the main document feed stuff), but I'd like to start
the first review iteration.

With this I have to sprinkle use_drive_api flag all over the
places, but I think it would be easy to clean up after the migration path is
complete.

[satorux1, 2012-07-09 15:58:11]

Thank you for sending this out early.

I have a feeling that this is not a good approach. Here's some initial thoughts:

1) adding |use_drive_api_| member doesn't sound like a good idea. Separating
behaviors by the boolean member would result in unclean code. It'd be cleaner to
define two separate classes with the same interface, if the implementations are
considerably different, just like you did with AccountMetadataFeed. we should
probably do the same for DocumentsService, which probably won't share much code
with the existing implementation.

2) classes don't need to have |use_drive_api_| member. we could add
gdata::util::IsDriveV2ApiEnabled(), which checkes --enable-drive-api-v2 flag,
and use it where needed. Not to mention, we should minimize use of this
function. ideally, we should only use this flag when creating objects with
factory methods like

  DocumentsServiceInterface* documents_service =
CreateDocumentsService(IsDriveV2ApiEnabled());

http://codereview.chromium.org/10693109/diff/7004/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc (right):

http://codereview.chromium.org/10693109/diff/7004/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc:227:
file_system_->Initialize(false);
boolean function parameter like this is hard to read. 

http://www.chromium.org/developers/coding-style#Code_Formatting

Begin to prefer enumerated values (e.g. "DELETE_AFTER_USE/KEEP_ALIVE_AFTER_USE")
to booleans ("bool delete_after_use") as function arguments the more the
following cases apply:

http://codereview.chromium.org/10693109/diff/7004/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (left):

http://codereview.chromium.org/10693109/diff/7004/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:114:
"https://docs.googleusercontent.com/";
could you create a separate patch to remove this?

http://codereview.chromium.org/10693109/diff/7004/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/10693109/diff/7004/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:503: const char kEnableDriveV2Api[]            
 = "enable-drive-v2-api";
could you create a separate patch that adds this flag?

[kochi, 2012-07-10 09:01:14]

Thanks for the comments.
Adding gdata::util::IsDriveV2ApiEnabled() made the change list
a lot smaller and easier to understand.
I eliminated all of the boolean use_drive_api usages.

Now this depends on
https://chromiumcodereview.appspot.com/10753024/
for --enable-drive-v2 and won't compile for now.

I'll investigate further for parsing DocumentFeed
equivalent data, and I may end up subclassing
DocumentsService or GDataFileSystem.

http://chromiumcodereview.appspot.com/10693109/diff/7004/chrome/browser/chrom...
File chrome/browser/chromeos/login/login_utils.cc (left):

http://chromiumcodereview.appspot.com/10693109/diff/7004/chrome/browser/chrom...
chrome/browser/chromeos/login/login_utils.cc:114:
"https://docs.googleusercontent.com/";
https://chromiumcodereview.appspot.com/10752021/

Done.

http://chromiumcodereview.appspot.com/10693109/diff/7004/chrome/common/chrome...
File chrome/common/chrome_switches.cc (right):

http://chromiumcodereview.appspot.com/10693109/diff/7004/chrome/common/chrome...
chrome/common/chrome_switches.cc:503: const char kEnableDriveV2Api[]            
 = "enable-drive-v2-api";
Done.
https://chromiumcodereview.appspot.com/10753024/

[satorux1, 2012-07-10 18:10:33]

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_parser.cc (right):

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_parser.cc:1027: class
AccountMetadataFeedDriveAPI : public AccountMetadataFeed {
What about creating a separate file like drive_api_parser.cc for the new Drive
API? We are going to add a bunch of code for the new Drive API, so having a
separate file would be cleaner.

The existing file should also be renamed to gdata_wapi_parser.cc or something.

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:385: return enabled;
Would be nice to create a separate patch that just adds this function. Then you
can submit it immediately.

[kochi, 2012-07-11 09:43:27]

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_parser.cc (right):

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_parser.cc:1027: class
AccountMetadataFeedDriveAPI : public AccountMetadataFeed {
At first I tried to avoid that, but it seems the difference
is significant and it makes sense to have separate parsers
and operations for Drive APIs.

For renaming this file, I have created separate CL
http://codereview.chromium.org/10692152/

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_util.cc (right):

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_util.cc:385: return enabled;
Done.

https://chromiumcodereview.appspot.com/10692149/

[satorux1, 2012-07-12 07:49:06]

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

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:918: if
(!gdata::util::IsDriveV2ApiEnabled())
does this mean the webapps registry does not work if if drive v2 is enabled?
please add some comment.

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_operations.cc:588: }
Not sure if it's worth sharing GetAccountMetadataOperation in this way.
GetAccountMetadataOperation is fairly small. Might be cleaner to create a
separate class.

btw, gdata_operations.cc has been getting big. we might want to split this...

[kochi, 2012-07-13 10:33:52]

http://chromiumcodereview.appspot.com/10693109/diff/1048/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):

http://chromiumcodereview.appspot.com/10693109/diff/1048/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:918: if
(!gdata::util::IsDriveV2ApiEnabled())
I intend to separate a new DriveFileSystem class and
will not add code such as this.

http://chromiumcodereview.appspot.com/10693109/diff/1048/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_operations.cc (right):

http://chromiumcodereview.appspot.com/10693109/diff/1048/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_operations.cc:588: }
gdata_operations.cc split is done:
https://src.chromium.org/viewvc/chrome?view=rev&revision=146548

And I intend to have another classes for Drive API calls.

[satorux1, 2012-07-14 07:15:34]

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

http://codereview.chromium.org/10693109/diff/1048/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:918: if
(!gdata::util::IsDriveV2ApiEnabled())
On 2012/07/13 10:33:52, Takayoshi Kochi wrote:
> I intend to separate a new DriveFileSystem class and
> will not add code such as this.

DriveFileSystem will likely share a lot of code with the existing
GDataFileSystem implementation. However, using inheritance to share
implementation tends to result in hard-to-read code, hence should usually be
avoided. If we are to introduce DriveFileSystem, I guess we'd need to move out
lots of code from GDataFileSystem...

Just adding some gdata::util::IsDriveV2ApiEnabled() calls in the existing code
maybe be simpler. Just for a food for thoughts.

[satorux1, 2012-08-03 17:32:23]

is this still relevant? otherwise, please close so it won't show up in our code
review dashboard. :)

[kochi, 2012-08-05 06:39:16]

On 2012/08/03 17:32:23, satorux1 wrote:
> is this still relevant? otherwise, please close so it won't show up in our
code
> review dashboard. :)

Closing the issue.