Cleanup: separate WAPI and Drive API code in gdata_documents_service.

Cleanup: separate WAPI and Drive API code in gdata_documents_service.

BUG=142809
TEST=pass all existing tests, manually check if file browser doesn't break.
TBR=ben@chromium.org

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

[kochi, 2012-08-17 10:46:11]

Hi Satoru,

Could you review this?
Thanks,

[satorux1, 2012-08-17 12:05:48]

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

http://codereview.chromium.org/10829375/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/documents_service_interface.h:49: class
DocumentsServiceInterface {
DocumentsServiceInterface is probably not a good name. You should probably
rename it in a separate patch. Please file a bug and add a TODO here.

Maybe DriveServiceInteface

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

http://codereview.chromium.org/10829375/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2244:
documents_service_->GetAccountMetadata(
since you are now using the same interface, can you get rid of
gdata::util::IsDriveV2ApiEnabled()? Please check other places too.

You can do it in a separate patch. If so, please file a bug and add a TODO here.

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

http://codereview.chromium.org/10829375/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:215:
util::IsDriveV2ApiEnabled() ?
please don't nest ternary operators. very hard to read. please rewrite this with
if-else and eliminate ternary operators.

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

http://codereview.chromium.org/10829375/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_service.h:26: // TODO(kochi): rewrite
this comment.
Can you do this now? Please mention about what WAPI means:

 WAPI (codename for Documents List API)

[kochi, 2012-08-20 09:17:30]

Thanks for the review!

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

https://chromiumcodereview.appspot.com/10829375/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/documents_service_interface.h:49: class
DocumentsServiceInterface {
Looks like changing so touches many files and it would be
better to split CLs for it.  Will send you a review for it later.

On 2012/08/17 12:05:48, satorux1 wrote:
> DocumentsServiceInterface is probably not a good name. You should probably
> rename it in a separate patch. Please file a bug and add a TODO here.
> 
> Maybe DriveServiceInteface

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

https://chromiumcodereview.appspot.com/10829375/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2244:
documents_service_->GetAccountMetadata(
As we have different callbacks for Drive API and WAPI,
we have to use IsDriveV2ApiEnabled() even though
we call the same interface here.

Changed the code here to use tertiary operator for now.

I think we should have better layering of these
file system/documents service/operations, and the higher
level users would get the result in same format whichever
API is used.

Filed http://crbug.com/143653

On 2012/08/17 12:05:48, satorux1 wrote:
> since you are now using the same interface, can you get rid of
> gdata::util::IsDriveV2ApiEnabled()? Please check other places too.
> 
> You can do it in a separate patch. If so, please file a bug and add a TODO
here.

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

https://chromiumcodereview.appspot.com/10829375/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_system_service.cc:215:
util::IsDriveV2ApiEnabled() ?
On 2012/08/17 12:05:48, satorux1 wrote:
> please don't nest ternary operators. very hard to read. please rewrite this
with
> if-else and eliminate ternary operators.

Done.

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

https://chromiumcodereview.appspot.com/10829375/diff/4001/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_service.h:26: // TODO(kochi): rewrite
this comment.
On 2012/08/17 12:05:48, satorux1 wrote:
> Can you do this now? Please mention about what WAPI means:
> 
>  WAPI (codename for Documents List API) 

Sorry, I intended to get this done before review.
Done.

[satorux1, 2012-08-20 16:47:19]

hmm, "Patch Set 2" contains lots of unrelated changes coming from git rebase.
I'd suggest to do:

1) as soon as you rebase, upload a new patch
2) then, make changes and upload a new patch

This way, the diff is easier to read.

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

http://codereview.chromium.org/10829375/diff/4001/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_file_system.cc:2244:
documents_service_->GetAccountMetadata(
On 2012/08/20 09:17:30, Takayoshi Kochi wrote:
> As we have different callbacks for Drive API and WAPI,
> we have to use IsDriveV2ApiEnabled() even though
> we call the same interface here.
> 
> Changed the code here to use tertiary operator for now.

Could you make this if-else? Ternary operators are harder to read, and this will
result in a harder-to-read compilation errors if we mess up something here.


Looking at the two callback functions, these look fairly similar. Rather than
using separate callbacks, it may be simpler to use the same callback, then do
things differently in the callback per gdata::util::IsDriveV2ApiEnabled().


> 
> I think we should have better layering of these
> file system/documents service/operations, and the higher
> level users would get the result in same format whichever
> API is used.
> 
> Filed http://crbug.com/143653
> 
> On 2012/08/17 12:05:48, satorux1 wrote:
> > since you are now using the same interface, can you get rid of
> > gdata::util::IsDriveV2ApiEnabled()? Please check other places too.
> > 
> > You can do it in a separate patch. If so, please file a bug and add a TODO
> here.
>

[satorux1, 2012-08-20 16:52:30]

LGTM with nits. Please submit this shortly so we can move onto the next target.

http://codereview.chromium.org/10829375/diff/5035/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10829375/diff/5035/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:217:
reinterpret_cast<DocumentsServiceInterface*>(
I guess the reinterpret_cast is unnecessary.

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

http://codereview.chromium.org/10829375/diff/5035/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:463: std::string(),
Please add comments for parameter names, like

std::string(),  // foobar

[satorux1, 2012-08-20 17:01:28]

http://codereview.chromium.org/10829375/diff/5035/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

http://codereview.chromium.org/10829375/diff/5035/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_system_service.cc:217:
reinterpret_cast<DocumentsServiceInterface*>(
On 2012/08/20 16:52:30, satorux1 wrote:
> I guess the reinterpret_cast is unnecessary.

Maybe reinterpret_cast is needed because of the ternary operator. If so, please
make it if-else.

[kochi, 2012-08-21 01:36:17]

For rebase, I'll do so from next time on.

For doing different in callbacks, I'll sort it out in later CLs.

On 2012/08/20 16:47:19, satorux1 wrote:
> hmm, "Patch Set 2" contains lots of unrelated changes coming from git rebase.
> I'd suggest to do:
> 
> 1) as soon as you rebase, upload a new patch
> 2) then, make changes and upload a new patch
> 
> This way, the diff is easier to read.
> 
>
http://codereview.chromium.org/10829375/diff/4001/chrome/browser/chromeos/gda...
> File chrome/browser/chromeos/gdata/gdata_file_system.cc (right):
> 
>
http://codereview.chromium.org/10829375/diff/4001/chrome/browser/chromeos/gda...
> chrome/browser/chromeos/gdata/gdata_file_system.cc:2244:
> documents_service_->GetAccountMetadata(
> On 2012/08/20 09:17:30, Takayoshi Kochi wrote:
> > As we have different callbacks for Drive API and WAPI,
> > we have to use IsDriveV2ApiEnabled() even though
> > we call the same interface here.
> > 
> > Changed the code here to use tertiary operator for now.
> 
> Could you make this if-else? Ternary operators are harder to read, and this
will
> result in a harder-to-read compilation errors if we mess up something here.
> 
> 
> Looking at the two callback functions, these look fairly similar. Rather than
> using separate callbacks, it may be simpler to use the same callback, then do
> things differently in the callback per gdata::util::IsDriveV2ApiEnabled().
> 
> 
> > 
> > I think we should have better layering of these
> > file system/documents service/operations, and the higher
> > level users would get the result in same format whichever
> > API is used.
> > 
> > Filed http://crbug.com/143653
> > 
> > On 2012/08/17 12:05:48, satorux1 wrote:
> > > since you are now using the same interface, can you get rid of
> > > gdata::util::IsDriveV2ApiEnabled()? Please check other places too.
> > > 
> > > You can do it in a separate patch. If so, please file a bug and add a TODO
> > here.
> >

[kochi, 2012-08-21 01:42:45]

Thanks for the review!

https://chromiumcodereview.appspot.com/10829375/diff/5035/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_system_service.cc (right):

https://chromiumcodereview.appspot.com/10829375/diff/5035/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_system_service.cc:217:
reinterpret_cast<DocumentsServiceInterface*>(
On 2012/08/20 17:01:29, satorux1 wrote:
> On 2012/08/20 16:52:30, satorux1 wrote:
> > I guess the reinterpret_cast is unnecessary.
> 
> Maybe reinterpret_cast is needed because of the ternary operator. If so,
please
> make it if-else.

Yes, it was necessary for ternary operator.
Changed to use if-else here to remove these casts.

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

https://chromiumcodereview.appspot.com/10829375/diff/5035/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_wapi_feed_loader.cc:463: std::string(),
On 2012/08/20 16:52:30, satorux1 wrote:
> Please add comments for parameter names, like
> 
> std::string(),  // foobar

Done.

[kochi, 2012-08-21 01:44:39]

Adding ben@chromium.org on TBR= line for review on gypi file.