Get AboutResource as account metadata for Drive V2.

Get AboutResource as account metadata for Drive V2.

In Drive V2 api, change ID is extended to int64, so we have to extend the size to 64bit.

For getting account metadata, WAPI:Drive V2 is not 1:1
mapping. Getting application list is separated from this
CL.

As refactoring gdata_wapi_feed_loader and making a separate
drive_api_feed_loader makes very redundant copy & paste,
so used IsDriveV2APIEnabled() for separating different JSON
feed handlings in completion callbacks.

CLs for getting file changelists and applists will follow.

BUG=chromium:127728
TEST=manually make sure this doesn't break FileBrowser


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

[kochi, 2012-08-08 10:46:02]

Hi Satoru,

Could you review this?
Thanks,

[satorux1, 2012-08-08 13:18:51]

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata.proto (right):

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata.proto:85: optional int32
deprecated_largest_changestamp = 2;
please remove this.

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

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2189:
base::Bind(&GDataFileSystem::OnGetQuotaStats,
OnGetAboutResource

We usually match the function name and the callback function.

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2229: scoped_ptr<base::Value>
data) {
data -> better name?

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2234: callback.Run(error, -1,
-1);
Historically, we check if the callback is null almost everywhere in
gdata_file_system.cc, as we allow null callbacks in certain places. However,
this is not a good practice. I'd suggest to add

DCHECK(!callback.is_null()) to this function, OnGetAvailableSpace(), and
GetAvailableSpaceOnUIThread()

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

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:555:
SetRootResourceId(kGDataRootDirectoryResourceId);
The function name looks wrong. Here, we are doing more than setting the resource
id. Maybe we don't need to introduce a function for this?

if (!util::IsDriveV2ApiEnabled()) {
   ...
}

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_files.cc:1053: largest_changestamp_ =
proto.largest_changestamp();
Because you made an incompatible change in gdata.proto,
proto.largest_changestamp() will be 0 if you read an existing proto file, right?

We should reject an existing proto file if you make an incompatible change.
Please increment the kProtoVersion in gdata_files.h.

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

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:233: if
(gdata::util::IsDriveV2ApiEnabled()) {
I named the file "gdata_wapi_feed_loader.cc", assuming that the Drive v2
implementation will be a separate file. However, it doesn't seem to be the case.
We should rename the file and the class name to be something like
"drive_feed_loader.cc" at a later time...

http://codereview.chromium.org/10823226/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:360:
LoadFromServer(initial_origin,
LoadFromServer has too many parameters. Could you fix this? Filed a bug:
crbug.com/141359

[kochi, 2012-08-08 16:53:27]

Thanks for the review!

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata.proto (right):

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata.proto:85: optional int32
deprecated_largest_changestamp = 2;
On 2012/08/08 13:18:51, satorux1 wrote:
> please remove this.

Done.

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

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2189:
base::Bind(&GDataFileSystem::OnGetQuotaStats,
On 2012/08/08 13:18:51, satorux1 wrote:
> OnGetAboutResource
> 
> We usually match the function name and the callback function.

Done.

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2229: scoped_ptr<base::Value>
data) {
On 2012/08/08 13:18:51, satorux1 wrote:
> data -> better name?

Done.

named resource_json.

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2234: callback.Run(error, -1,
-1);
On 2012/08/08 13:18:51, satorux1 wrote:
> Historically, we check if the callback is null almost everywhere in
> gdata_file_system.cc, as we allow null callbacks in certain places. However,
> this is not a good practice. I'd suggest to add
> 
> DCHECK(!callback.is_null()) to this function, OnGetAvailableSpace(), and
> GetAvailableSpaceOnUIThread()

Done.

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_files.cc (right):

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_files.cc:555:
SetRootResourceId(kGDataRootDirectoryResourceId);
On 2012/08/08 13:18:51, satorux1 wrote:
> The function name looks wrong. Here, we are doing more than setting the
resource
> id. Maybe we don't need to introduce a function for this?
> 
> if (!util::IsDriveV2ApiEnabled()) {
>    ...
> }

Nope, I introduced the function as Drive v2 uses a unique ID for root
(not a fixed string) and we have to set the resource ID after we get
the id in About resource.

InitializeRootEntry(resource_id) sound good to you?

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_files.cc:1053: largest_changestamp_ =
proto.largest_changestamp();
On 2012/08/08 13:18:51, satorux1 wrote:
> Because you made an incompatible change in gdata.proto,
> proto.largest_changestamp() will be 0 if you read an existing proto file,
right?
> 
> We should reject an existing proto file if you make an incompatible change.
> Please increment the kProtoVersion in gdata_files.h.

Done.

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
File chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc (right):

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:233: if
(gdata::util::IsDriveV2ApiEnabled()) {
On 2012/08/08 13:18:51, satorux1 wrote:
> I named the file "gdata_wapi_feed_loader.cc", assuming that the Drive v2
> implementation will be a separate file. However, it doesn't seem to be the
case.
> We should rename the file and the class name to be something like
> "drive_feed_loader.cc" at a later time...

I could have a separate file but it turned out that the amount of copy & paste
is too much and it doubles the maintenance cost until WAPI stuff goes away.

So I think it makes sense to have this if-clause.

http://chromiumcodereview.appspot.com/10823226/diff/9001/chrome/browser/chrom...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:360:
LoadFromServer(initial_origin,
On 2012/08/08 13:18:51, satorux1 wrote:
> LoadFromServer has too many parameters. Could you fix this? Filed a bug:
> crbug.com/141359

Will work on a separate CL.

[satorux1, 2012-08-08 20:13:43]

LGTM

[commit-bot: I haz the power, 2012-08-09 02:15:19]

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

[commit-bot: I haz the power, 2012-08-09 02:15:23]

Failed to apply patch for chrome/browser/chromeos/gdata/gdata_files.h:
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/gdata/gdata_files.h
Hunk #2 FAILED at 404.
Hunk #3 succeeded at 527 (offset 3 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/gdata/gdata_files.h.rej

[commit-bot: I haz the power, 2012-08-09 02:31:30]

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

[commit-bot: I haz the power, 2012-08-09 04:00:22]

Change committed as 150718