Add FileResource/FileList parser.

Add FileResource/FileList parser.

Still multiple list (uses next_link and token), parent
folder handling etc. are missing, but minimal information
is captured.

BUG=chromium:127728
TEST=unit_tests --gtest_filter="DriveApiParserTest.*"

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

[kochi, 2012-07-27 10:11:25]

Satoru,

Could you review this?  Thanks!

[satorux1, 2012-07-27 23:03:10]

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

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:260: // FileResource
reporesents a file in Drive.
a file or a directory? This class contains IsFolder().

If this can be a directory, "FileResource" may not be a good name. Maybe just
Resource, or ResourceEntry?

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:273: bool IsFolder() const;
IsDirectory()?

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:276: const std::string& id()
const { return id_; }
Is this equivalent of resource ID? maybe resource_id() would be a better name.

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:324: // FileList represents a
collection of files.
File is a not good name if it means file or directory. Maybe ResourceList?

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:339: // Returns the page token
for the next page of files.
how does it look like? what's the "page token"? please document.

[kochi, 2012-07-30 07:46:52]

Thanks for the review!

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

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:260: // FileResource
reporesents a file in Drive.
I agree that this might be a confusing name, but I am trying
to name these classes for Drive APIs as close as possible
to Drive v2 API documents.

"Resource" is a term that REST architecture proponents
tend tou se for referring data that is passed via HTTP requests.

So I would like to keep the class name as is.
I've updated the comment as
// FileResource represents a file or directory metadata in Drive.

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:273: bool IsFolder() const;
The term "directory" is never used in Drive API docs.
Even if we use "directory" in our internal code and
"folder" in externally-facing (drive API) stuff, it may
be still confusing... I'd like to keep using "folder" for
interfaces that directly handles Drive API.

Eventually when we reach time to remove old WAPI stuff,
I'd like to clean up these terminology but during the
migration period, the scope that used Drive API terminology
can be restricted to Drive API specific code.

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:276: const std::string& id()
const { return id_; }
No, this is different from old WAPI's resource ID.
Resource ID is not used in Drive API any more.

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:324: // FileList represents a
collection of files.
The same for FileResource.  Updated the class comment.

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:339: // Returns the page token
for the next page of files.
Added more comments, and also added checks in unit_tests.

[satorux1, 2012-07-30 09:01:13]

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

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:260: // FileResource
reporesents a file in Drive.
On 2012/07/30 07:46:53, Takayoshi Kochi wrote:
> I agree that this might be a confusing name, but I am trying
> to name these classes for Drive APIs as close as possible
> to Drive v2 API documents.
> 
> "Resource" is a term that REST architecture proponents
> tend tou se for referring data that is passed via HTTP requests.
> 
> So I would like to keep the class name as is.
> I've updated the comment as
> // FileResource represents a file or directory metadata in Drive.

Here's the background about my concern:

We originally used the term "file" sloppily in the code in this directory, but
it made the code very hard-to-read as "file" meant a regular file in some
places, but "file" meant a regular file or a directory elsewhere. Eventually, we
replaced the latter with "entry", and the code became easier to read.

I'm file with FileResorce, if this class will not be used widely. I guess
FileResurce is converted to GDataEntryProto before used in GDataFileSystem,
right?

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:273: bool IsFolder() const;
On 2012/07/30 07:46:53, Takayoshi Kochi wrote:
> The term "directory" is never used in Drive API docs.
> Even if we use "directory" in our internal code and
> "folder" in externally-facing (drive API) stuff, it may
> be still confusing... I'd like to keep using "folder" for
> interfaces that directly handles Drive API.
> 
> Eventually when we reach time to remove old WAPI stuff,
> I'd like to clean up these terminology but during the
> migration period, the scope that used Drive API terminology
> can be restricted to Drive API specific code.

I think we should use the term "directory" in most places, which is the term
used in HTML5 file system API.

However, I'm fine with IsFolder() here, assuming that this class won't be used
widely. Please also mention that folder == directory.

http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser.h:276: const std::string& id()
const { return id_; }
On 2012/07/30 07:46:53, Takayoshi Kochi wrote:
> No, this is different from old WAPI's resource ID.
> Resource ID is not used in Drive API any more.

But the purpose is the same? If so, please also mention that this ID is used as
unique resource ID?

[satorux1, 2012-07-30 16:34:32]

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

http://codereview.chromium.org/10829056/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser_unittest.cc:81: IF_EXPECT_EQ(2U,
applist->items().size()) {
ASSERT_EQ

http://codereview.chromium.org/10829056/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser_unittest.cc:95: IF_EXPECT_EQ(1U,
app1.primary_mimetypes().size()) {
ASSERT_EQ

http://codereview.chromium.org/10829056/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser_unittest.cc:181: IF_EXPECT_EQ(3U,
filelist->items().size()) {
ASSERT_EQ(3U, filelist->items().size());

IF_EXPECT_EQ seems to be a bad idea. Can you get rid of it?

[kochi, 2012-07-31 07:41:07]

On 2012/07/30 09:01:13, satorux1 wrote:
>
http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/drive_api_parser.h (right):
> 
>
http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/drive_api_parser.h:260: // FileResource
> reporesents a file in Drive.
> On 2012/07/30 07:46:53, Takayoshi Kochi wrote:
> > I agree that this might be a confusing name, but I am trying
> > to name these classes for Drive APIs as close as possible
> > to Drive v2 API documents.
> > 
> > "Resource" is a term that REST architecture proponents
> > tend tou se for referring data that is passed via HTTP requests.
> > 
> > So I would like to keep the class name as is.
> > I've updated the comment as
> > // FileResource represents a file or directory metadata in Drive.
> 
> Here's the background about my concern:
> 
> We originally used the term "file" sloppily in the code in this directory, but
> it made the code very hard-to-read as "file" meant a regular file in some
> places, but "file" meant a regular file or a directory elsewhere. Eventually,
we
> replaced the latter with "entry", and the code became easier to read.
> 
> I'm file with FileResorce, if this class will not be used widely. I guess
> FileResurce is converted to GDataEntryProto before used in GDataFileSystem,
> right?

Thanks for the background.  And yes, FileResource will be converted
to GDataEntryProto.

 
http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/drive_api_parser.h:273: bool IsFolder() const;
> On 2012/07/30 07:46:53, Takayoshi Kochi wrote:
> > The term "directory" is never used in Drive API docs.
> > Even if we use "directory" in our internal code and
> > "folder" in externally-facing (drive API) stuff, it may
> > be still confusing... I'd like to keep using "folder" for
> > interfaces that directly handles Drive API.
> > 
> > Eventually when we reach time to remove old WAPI stuff,
> > I'd like to clean up these terminology but during the
> > migration period, the scope that used Drive API terminology
> > can be restricted to Drive API specific code.
> 
> I think we should use the term "directory" in most places, which is the term
> used in HTML5 file system API.
> 
> However, I'm fine with IsFolder() here, assuming that this class won't be used
> widely. Please also mention that folder == directory.

OK, added comments about this.  I renamed this to IsDirectory().


http://codereview.chromium.org/10829056/diff/2001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/drive_api_parser.h:276: const std::string& id()
> const { return id_; }
> On 2012/07/30 07:46:53, Takayoshi Kochi wrote:
> > No, this is different from old WAPI's resource ID.
> > Resource ID is not used in Drive API any more.
> 
> But the purpose is the same? If so, please also mention that this ID is used
as
> unique resource ID?

I renamed this field to file_id, as other IDs (application ID,
change ID etc. also exists in Drive API.

file_id is unique in files, but application_id and change_id are
in different format and should be named differently for avoid confusion.

[kochi, 2012-07-31 08:07:58]

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

http://codereview.chromium.org/10829056/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser_unittest.cc:81: IF_EXPECT_EQ(2U,
applist->items().size()) {
On 2012/07/30 16:34:33, satorux1 wrote:
> ASSERT_EQ

Done.

http://codereview.chromium.org/10829056/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser_unittest.cc:95: IF_EXPECT_EQ(1U,
app1.primary_mimetypes().size()) {
On 2012/07/30 16:34:33, satorux1 wrote:
> ASSERT_EQ

Done.

http://codereview.chromium.org/10829056/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser_unittest.cc:181: IF_EXPECT_EQ(3U,
filelist->items().size()) {
Done.

I copied the macro from gdata_wapi_parser_unittest.cc,
and I thought the idea is that if the first check fails,
the following enclosed tests also fail, and log would
get messy.

Anyway, failing test fails at ASSERT, so this looks
better to me.

[satorux1, 2012-08-01 04:35:45]

LGTM with a request:

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

http://codereview.chromium.org/10829056/diff/9001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/drive_api_parser_unittest.cc:181: IF_EXPECT_EQ(3U,
filelist->items().size()) {
On 2012/07/31 08:07:58, Takayoshi Kochi wrote:
> Done.
> 
> I copied the macro from gdata_wapi_parser_unittest.cc,
> and I thought the idea is that if the first check fails,
> the following enclosed tests also fail, and log would
> get messy.
> 
> Anyway, failing test fails at ASSERT, so this looks
> better to me.

Could you also remove these IF_ macros from  gdata_wapi_parser_unittest.cc?

[kochi, 2012-08-01 04:51:23]

Thanks for the review!

> Could you also remove these IF_ macros from  gdata_wapi_parser_unittest.cc?

Filed a separate issue at crbug.com/139956 and will create a separate
patch.