Drive: Remove gdata_params.h

Drive: Remove gdata_params.h

Moves the definitions in gdata_params.h to each appropriate file.

BUG=135306
TBR=ben@chromium.org
# for chrome/chrome_browser.gypi (two files removed)

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

[yoshiki, 2012-08-07 23:05:26]

Satoru-san, could you take a look?

[satorux1, 2012-08-07 23:08:17]

The two files are marked "M" in the code review page. I thought deleted files
were marked "D"

M	 chrome/browser/chromeos/gdata/gdata_params.h
M	 chrome/browser/chromeos/gdata/gdata_params.cc

[satorux1, 2012-08-07 23:13:30]

We want to refrain from submitting large refactoring patches right after the
branch cut. This patch may be in this category. Could you wait for a couple of
days before submitting this, just in case?

[achuithb, 2012-08-07 23:14:21]

I like that LoadRootFeedParams is now moved to gdata_wapi_feed_loader.*

One other option is to rename gdata_params to gdata_document_service_params.
Otherwise, you've basically just made the gdata_document_service.h file bigger
and included it everywhere where gdata_params was included even though all
that's necessary are the callback typedefs.

http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
File chrome/browser/chromeos/gdata/gdata_documents_service.h (right):

http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
chrome/browser/chromeos/gdata/gdata_documents_service.h:18: #include
"chrome/browser/chromeos/gdata/gdata_documents_service.h"
Isn't this gdata_documents_service.h?

[satorux1, 2012-08-07 23:15:38]

On 2012/08/07 23:14:21, achuith.bhandarkar wrote:
> I like that LoadRootFeedParams is now moved to gdata_wapi_feed_loader.*
> 
> One other option is to rename gdata_params to gdata_document_service_params.
> Otherwise, you've basically just made the gdata_document_service.h file bigger
> and included it everywhere where gdata_params was included even though all
> that's necessary are the callback typedefs.

I like the idea!

> 
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> File chrome/browser/chromeos/gdata/gdata_documents_service.h (right):
> 
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> chrome/browser/chromeos/gdata/gdata_documents_service.h:18: #include
> "chrome/browser/chromeos/gdata/gdata_documents_service.h"
> Isn't this gdata_documents_service.h?

[yoshiki, 2012-08-07 23:27:42]

On 2012/08/07 23:15:38, satorux1 wrote:
> On 2012/08/07 23:14:21, achuith.bhandarkar wrote:
> > I like that LoadRootFeedParams is now moved to gdata_wapi_feed_loader.*
> > 
> > One other option is to rename gdata_params to gdata_document_service_params.
> > Otherwise, you've basically just made the gdata_document_service.h file
bigger
> > and included it everywhere where gdata_params was included even though all
> > that's necessary are the callback typedefs.
> 
> I like the idea!

Thanks for advice, I'll do so.

> > 
> >
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> > File chrome/browser/chromeos/gdata/gdata_documents_service.h (right):
> > 
> >
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> > chrome/browser/chromeos/gdata/gdata_documents_service.h:18: #include
> > "chrome/browser/chromeos/gdata/gdata_documents_service.h"
> > Isn't this gdata_documents_service.h?

[yoshiki, 2012-08-07 23:30:15]

How about AuthStatusCallback? Should we move it to gdata_auth_service.h, or keep
it on gdata_document_service_params.h?

On 2012/08/07 23:27:42, yoshiki wrote:
> On 2012/08/07 23:15:38, satorux1 wrote:
> > On 2012/08/07 23:14:21, achuith.bhandarkar wrote:
> > > I like that LoadRootFeedParams is now moved to gdata_wapi_feed_loader.*
> > > 
> > > One other option is to rename gdata_params to
gdata_document_service_params.
> > > Otherwise, you've basically just made the gdata_document_service.h file
> bigger
> > > and included it everywhere where gdata_params was included even though all
> > > that's necessary are the callback typedefs.
> > 
> > I like the idea!
> 
> Thanks for advice, I'll do so.
> 
> > > 
> > >
> >
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> > > File chrome/browser/chromeos/gdata/gdata_documents_service.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> > > chrome/browser/chromeos/gdata/gdata_documents_service.h:18: #include
> > > "chrome/browser/chromeos/gdata/gdata_documents_service.h"
> > > Isn't this gdata_documents_service.h?

[satorux1, 2012-08-08 00:35:52]

On 2012/08/07 23:30:15, yoshiki wrote:
> How about AuthStatusCallback? Should we move it to gdata_auth_service.h, or
keep
> it on gdata_document_service_params.h?

Maybe AuthStatusCallback should belong to  operations_base.h ? This seems to be
the lowest level user. DocumentsService is built on top of this, right?

Similarly, gdata_operations.h seems to be the lowest level users of callbacks
and types defined in gdata_documents_service_params.h.

That said, we should rename gdata_documents_service_params.h to
gdata_operations_callback.h, instead. :)

I think You can put AuthStatusCallback to operations_base.h as it's just one
simple typedef.

> 
> On 2012/08/07 23:27:42, yoshiki wrote:
> > On 2012/08/07 23:15:38, satorux1 wrote:
> > > On 2012/08/07 23:14:21, achuith.bhandarkar wrote:
> > > > I like that LoadRootFeedParams is now moved to gdata_wapi_feed_loader.*
> > > > 
> > > > One other option is to rename gdata_params to
> gdata_document_service_params.
> > > > Otherwise, you've basically just made the gdata_document_service.h file
> > bigger
> > > > and included it everywhere where gdata_params was included even though
all
> > > > that's necessary are the callback typedefs.
> > > 
> > > I like the idea!
> > 
> > Thanks for advice, I'll do so.
> > 
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> > > > File chrome/browser/chromeos/gdata/gdata_documents_service.h (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> > > > chrome/browser/chromeos/gdata/gdata_documents_service.h:18: #include
> > > > "chrome/browser/chromeos/gdata/gdata_documents_service.h"
> > > > Isn't this gdata_documents_service.h?

[yoshiki, 2012-08-08 19:11:42]

Thanks for advice! Done. PTAL

On 2012/08/08 00:35:52, satorux1 wrote:
> On 2012/08/07 23:30:15, yoshiki wrote:
> > How about AuthStatusCallback? Should we move it to gdata_auth_service.h, or
> keep
> > it on gdata_document_service_params.h?
> 
> Maybe AuthStatusCallback should belong to  operations_base.h ? This seems to
be
> the lowest level user. DocumentsService is built on top of this, right?
> 
> Similarly, gdata_operations.h seems to be the lowest level users of callbacks
> and types defined in gdata_documents_service_params.h.
> 
> That said, we should rename gdata_documents_service_params.h to
> gdata_operations_callback.h, instead. :)
> 
> I think You can put AuthStatusCallback to operations_base.h as it's just one
> simple typedef.
> 
> > 
> > On 2012/08/07 23:27:42, yoshiki wrote:
> > > On 2012/08/07 23:15:38, satorux1 wrote:
> > > > On 2012/08/07 23:14:21, achuith.bhandarkar wrote:
> > > > > I like that LoadRootFeedParams is now moved to
gdata_wapi_feed_loader.*
> > > > > 
> > > > > One other option is to rename gdata_params to
> > gdata_document_service_params.
> > > > > Otherwise, you've basically just made the gdata_document_service.h
file
> > > bigger
> > > > > and included it everywhere where gdata_params was included even though
> all
> > > > > that's necessary are the callback typedefs.
> > > > 
> > > > I like the idea!
> > > 
> > > Thanks for advice, I'll do so.
> > > 
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> > > > > File chrome/browser/chromeos/gdata/gdata_documents_service.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
http://codereview.chromium.org/10855034/diff/10001/chrome/browser/chromeos/gd...
> > > > > chrome/browser/chromeos/gdata/gdata_documents_service.h:18: #include
> > > > > "chrome/browser/chromeos/gdata/gdata_documents_service.h"
> > > > > Isn't this gdata_documents_service.h?

[satorux1, 2012-08-08 20:21:19]

http://codereview.chromium.org/10855034/diff/3005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_operations_callback.h (right):

http://codereview.chromium.org/10855034/diff/3005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_operations_callback.h:6: #define
CHROME_BROWSER_CHROMEOS_GDATA_GDATA_OPERATIONS_CALLBACK_H_
Sorry to be nit-picky, but the name does not look correct. This file contains
callbacks, and structs. However, I don't have a good idea. I guess we can just
put this in gdata_operations.h. It'll bloat the header file, but shouldn't be a
big deal.

Besides, we'll eventually split gdata_operations.h, that contains too many
classes, to smaller files (one class per file, maybe in a sub directory).

http://codereview.chromium.org/10855034/diff/3005/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/operations_base.h (right):

http://codereview.chromium.org/10855034/diff/3005/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/operations_base.h:12: #include
"chrome/browser/chromeos/gdata/gdata_operations_callback.h"
do we need this?

[yoshiki, 2012-08-08 22:24:26]

On 2012/08/08 20:21:19, satorux1 wrote:
> Sorry to be nit-picky, but the name does not look correct. This file contains
> callbacks, and structs. However, I don't have a good idea. I guess we can just
> put this in gdata_operations.h. It'll bloat the header file, but shouldn't be
a
> big deal.

We can't all of the callbacks to gdata_operations.h, because gdata_operations.h
includes operations_base.h but operations_base.h uses some of callbacks
(EntryActionCallback & GetDataCallback). 

I think we may have 3 options.
1) Move all callbacks to operations_base.h
2) Move only EntryActionCallback & GetDataCallback to operations_base.h.
3) Keep it separate file, and rename some good name (gdata_operations_params.h
or something else)?

[satorux1, 2012-08-08 22:31:35]

On 2012/08/08 22:24:26, yoshiki wrote:
> On 2012/08/08 20:21:19, satorux1 wrote:
> > Sorry to be nit-picky, but the name does not look correct. This file
contains
> > callbacks, and structs. However, I don't have a good idea. I guess we can
just
> > put this in gdata_operations.h. It'll bloat the header file, but shouldn't
be
> a
> > big deal.
> 
> We can't all of the callbacks to gdata_operations.h, because
gdata_operations.h
> includes operations_base.h but operations_base.h uses some of callbacks
> (EntryActionCallback & GetDataCallback). 
> 
> I think we may have 3 options.
> 1) Move all callbacks to operations_base.h
> 2) Move only EntryActionCallback & GetDataCallback to operations_base.h.
> 3) Keep it separate file, and rename some good name (gdata_operations_params.h
> or something else)?

I'd suggest 2) for now. I've filed a bug for aggressive splitting.
crbug.com/141469. Then things will be saner.

[yoshiki, 2012-08-08 22:54:37]

I did (2), and moved FindEntryCallback to gdata_files.h in addition. PTAL

https://chromiumcodereview.appspot.com/10855034/diff/3005/chrome/browser/chro...
File chrome/browser/chromeos/gdata/operations_base.h (right):

https://chromiumcodereview.appspot.com/10855034/diff/3005/chrome/browser/chro...
chrome/browser/chromeos/gdata/operations_base.h:12: #include
"chrome/browser/chromeos/gdata/gdata_operations_callback.h"
On 2012/08/08 20:21:19, satorux1 wrote:
> do we need this?

Done.

[satorux1, 2012-08-09 20:53:48]

http://codereview.chromium.org/10855034/diff/2020/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_operations.h (right):

http://codereview.chromium.org/10855034/diff/2020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_operations.h:28: const FilePath& temp_file)>
DownloadActionCallback;
Please see my comments in operations_base.h

http://codereview.chromium.org/10855034/diff/2020/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/operations_base.h (right):

http://codereview.chromium.org/10855034/diff/2020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/operations_base.h:26: const std::string& token)>
AuthStatusCallback;
move this to where class AuthOperation is defined? That'll make it easier to
split this file at a later time.

http://codereview.chromium.org/10855034/diff/2020/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/operations_base.h:32: scoped_ptr<base::Value>
feed_data)> GetDataCallback;
ditto. move it to GetDataOperation section.

[yoshiki, 2012-08-09 22:00:46]

PTAL

https://chromiumcodereview.appspot.com/10855034/diff/2020/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_operations.h (right):

https://chromiumcodereview.appspot.com/10855034/diff/2020/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_operations.h:28: const FilePath& temp_file)>
DownloadActionCallback;
On 2012/08/09 20:53:48, satorux1 wrote:
> Please see my comments in operations_base.h

Done.

https://chromiumcodereview.appspot.com/10855034/diff/2020/chrome/browser/chro...
File chrome/browser/chromeos/gdata/operations_base.h (right):

https://chromiumcodereview.appspot.com/10855034/diff/2020/chrome/browser/chro...
chrome/browser/chromeos/gdata/operations_base.h:26: const std::string& token)>
AuthStatusCallback;
On 2012/08/09 20:53:48, satorux1 wrote:
> move this to where class AuthOperation is defined? That'll make it easier to
> split this file at a later time.

Done.

https://chromiumcodereview.appspot.com/10855034/diff/2020/chrome/browser/chro...
chrome/browser/chromeos/gdata/operations_base.h:32: scoped_ptr<base::Value>
feed_data)> GetDataCallback;
On 2012/08/09 20:53:48, satorux1 wrote:
> ditto. move it to GetDataOperation section.

Done.

[satorux1, 2012-08-09 22:13:09]

almost there!

https://chromiumcodereview.appspot.com/10855034/diff/7018/chrome/browser/chro...
File chrome/browser/chromeos/gdata/gdata_operations.h (right):

https://chromiumcodereview.appspot.com/10855034/diff/7018/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_operations.h:22: // DocumentServiceInterface
calls.
The function comment seems to be out of sync. What about:

// Callback type for getting the content from URLFetcher::GetResponseAsString().

https://chromiumcodereview.appspot.com/10855034/diff/7018/chrome/browser/chro...
chrome/browser/chromeos/gdata/gdata_operations.h:25: scoped_ptr<std::string>
download_data)> GetDownloadDataCallback;
renamed this to GetContentCallback?


I think we can move this to URlFetchOperationBase section in operations_base.h.
Because this is useful for sub classes of URlFetchOperationBase which want to
get the content.

[yoshiki, 2012-08-10 02:11:44]

Done! PTAL.

http://codereview.chromium.org/10855034/diff/7018/chrome/browser/chromeos/gda...
File chrome/browser/chromeos/gdata/gdata_operations.h (right):

http://codereview.chromium.org/10855034/diff/7018/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_operations.h:22: // DocumentServiceInterface
calls.
On 2012/08/09 22:13:09, satorux1 wrote:
> The function comment seems to be out of sync. What about:
> 
> // Callback type for getting the content from
URLFetcher::GetResponseAsString().

Done.

http://codereview.chromium.org/10855034/diff/7018/chrome/browser/chromeos/gda...
chrome/browser/chromeos/gdata/gdata_operations.h:25: scoped_ptr<std::string>
download_data)> GetDownloadDataCallback;
On 2012/08/09 22:13:09, satorux1 wrote:
> renamed this to GetContentCallback?
> 
> 
> I think we can move this to URlFetchOperationBase section in
operations_base.h.
> Because this is useful for sub classes of URlFetchOperationBase which want to
> get the content.

Done.

[satorux1, 2012-08-10 17:40:16]

LGTM

I think you can submit this now. This patch could potentially make merging to
m22 harder, but the probability seems low now.

BTW, uploading a "fix & rebase" patch should be avoided (though I often do it).
you should 1) upload after rebase, then 2) upload a patch with your changes.
this way will make code reviews easier.

[commit-bot: I haz the power, 2012-08-10 17:52:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10855034/6022

[commit-bot: I haz the power, 2012-08-10 17:52:33]

Presubmit check for 10855034-6022 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/browser/chromeos/gdata/gdata_contacts_service.cc:435
    operation->set_feed_url_for_testing( \
  chrome/browser/chromeos/gdata/gdata_contacts_service.cc:493
    operation->set_feed_url_for_testing(

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.7s to calculate.

[yoshiki, 2012-08-10 17:55:21]

updated the description, retrying...

[commit-bot: I haz the power, 2012-08-10 17:55:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10855034/6022

[yoshiki, 2012-08-10 18:09:01]

On 2012/08/10 17:40:16, satorux1 wrote:
> LGTM
> 
> I think you can submit this now. This patch could potentially make merging to
> m22 harder, but the probability seems low now.
> 
> BTW, uploading a "fix & rebase" patch should be avoided (though I often do
it).
> you should 1) upload after rebase, then 2) upload a patch with your changes.
> this way will make code reviews easier.

I think so, that's annoying. Especially a large CL like this....

[commit-bot: I haz the power, 2012-08-10 18:21:24]

Try job failure for 10855034-6022 on linux_chromeos for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-08-10 20:21:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10855034/6022

[commit-bot: I haz the power, 2012-08-10 21:49:18]

Change committed as 151125