Separate WebDataRequest functionality from WebDataService

Move WebDataRequest management code into a separate class
(which can be reused when WebDataService is broken up).

TBR=ben@chromium.org (for gyp changes)
TEST=No visible change
BUG=166488
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175965

[Cait (Slow), 2013-01-03 21:37:34]

pkasting@:
This is a first pass at separating WebDataRequest functionality from
WebDataService, in preparation for splitting up WebDataService (more details
here:
https://docs.google.com/a/chromium.org/document/d/1KXsn-OgeJdX3dtVjeDo-OdVkZH...).

PTAL and let me know if I am on the right track. 

Thanks,

[Peter Kasting, 2013-01-03 22:03:07]

I think you want dhollowa, not me.

[Cait (Slow), 2013-01-03 22:26:35]

dhollowa@:
This is a first pass at separating WebDataRequest functionality from
WebDataService, in preparation for splitting up WebDataService (more details
here:
https://docs.google.com/a/chromium.org/document/d/1KXsn-OgeJdX3dtVjeDo-OdVkZH...).

PTAL and let me know if I am on the right track.

Thanks,

[dhollowa, 2013-01-04 01:34:50]

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_request_manager.cc (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: remove the "(c)" bit.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.cc:10: #include
"base/threading/thread.h"
not used

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.cc:58: // TODO(caitkp): get rid
of Autofill DEPS here.
Yes, this is unfortunate.  A possible solution would be to extend the WDResult
class to take an optional callback to be called when its (new) |Destroy| method
is called.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_request_manager.h (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: remove the "(c)" bit.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:13: #include <string>
string and vector not used.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:16: #include
"base/callback_forward.h"
A good chunk of these are not required.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:56: bool
IsCancelled(WebDataServiceConsumer** consumer) const;
Let's take the opportunity to split out a separate accessor for the consumer. 
Mixing the two like this has always bugged me.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:72:
scoped_refptr<WebDataService> service_;
Let's add some doc to these members.  Eg.

 service_ -> Used to notify service of request completion.
 message_loop_ -> Tracks loop that the request originated on.
 handle_ -> Identifier for this request.
 consumer_ -> The originator of the service request.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:87: class
WebDataRequestManager {
The order of the classes in this header is opposite of the order of definition
in the .cc.  Please change the .cc to match the .h.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:87: class
WebDataRequestManager {
Add description of this class, making note that this is an "internal" interface,
not to be used outside of webdata/.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service.cc:157:
wdr_mgr_->RegisterRequest(request);
The common pattern here is:
  (1) new GenericRequest<Foo>(wdr_mgr->GetNextRequestHandle()),...
  (2) wdr_mgr_->RegisterRequest(request)

It would be preferable now to simply pass the wdr_mrg into the constructor of
the request, and let the request get its own handle and register itself.  This
will remove a tonne of boilerplate code in this file.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service.h:109: class GenericRequest : public
WebDataRequest {
These templates should be moved into web_data_request_manager.h as well.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service.h:522: scoped_ptr<WebDataRequestManager>
wdr_mgr_;
nit: no abbreviations.  How about |request_manager_|?

[Cait (Slow), 2013-01-04 22:59:28]

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_request_manager.cc (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/01/04 01:34:50, dhollowa wrote:
> nit: remove the "(c)" bit.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.cc:10: #include
"base/threading/thread.h"
On 2013/01/04 01:34:50, dhollowa wrote:
> not used

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.cc:58: // TODO(caitkp): get rid
of Autofill DEPS here.
I added the optional callback and |Destroy| method to WDResult to break this
dependency. It would be nice if we could avoid passing in "dummy" callbacks for
all the other cases though.

On 2013/01/04 01:34:50, dhollowa wrote:
> Yes, this is unfortunate.  A possible solution would be to extend the WDResult
> class to take an optional callback to be called when its (new) |Destroy|
method
> is called.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_request_manager.h (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/01/04 01:34:50, dhollowa wrote:
> nit: remove the "(c)" bit.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:13: #include <string>
On 2013/01/04 01:34:50, dhollowa wrote:
> string and vector not used.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:16: #include
"base/callback_forward.h"
On 2013/01/04 01:34:50, dhollowa wrote:
> A good chunk of these are not required.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:56: bool
IsCancelled(WebDataServiceConsumer** consumer) const;
On 2013/01/04 01:34:50, dhollowa wrote:
> Let's take the opportunity to split out a separate accessor for the consumer. 
> Mixing the two like this has always bugged me.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:72:
scoped_refptr<WebDataService> service_;
On 2013/01/04 01:34:50, dhollowa wrote:
> Let's add some doc to these members.  Eg.
> 
>  service_ -> Used to notify service of request completion.
>  message_loop_ -> Tracks loop that the request originated on.
>  handle_ -> Identifier for this request.
>  consumer_ -> The originator of the service request.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:87: class
WebDataRequestManager {
On 2013/01/04 01:34:50, dhollowa wrote:
> Add description of this class, making note that this is an "internal"
interface,
> not to be used outside of webdata/.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.h:87: class
WebDataRequestManager {
On 2013/01/04 01:34:50, dhollowa wrote:
> The order of the classes in this header is opposite of the order of definition
> in the .cc.  Please change the .cc to match the .h.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service.cc:157:
wdr_mgr_->RegisterRequest(request);
On 2013/01/04 01:34:50, dhollowa wrote:
> The common pattern here is:
>   (1) new GenericRequest<Foo>(wdr_mgr->GetNextRequestHandle()),...
>   (2) wdr_mgr_->RegisterRequest(request)
> 
> It would be preferable now to simply pass the wdr_mrg into the constructor of
> the request, and let the request get its own handle and register itself.  This
> will remove a tonne of boilerplate code in this file.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service.h:109: class GenericRequest : public
WebDataRequest {
On 2013/01/04 01:34:50, dhollowa wrote:
> These templates should be moved into web_data_request_manager.h as well.

Done.

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_service.h:522: scoped_ptr<WebDataRequestManager>
wdr_mgr_;
On 2013/01/04 01:34:50, dhollowa wrote:
> nit: no abbreviations.  How about |request_manager_|?

Done.

[dhollowa, 2013-01-04 23:57:36]

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
File chrome/browser/webdata/web_data_request_manager.cc (right):

https://codereview.chromium.org/11761016/diff/1/chrome/browser/webdata/web_da...
chrome/browser/webdata/web_data_request_manager.cc:58: // TODO(caitkp): get rid
of Autofill DEPS here.
Agreed.  A second constructor would solve this.

On 2013/01/04 22:59:29, caitkp wrote:
> I added the optional callback and |Destroy| method to WDResult to break this
> dependency. It would be nice if we could avoid passing in "dummy" callbacks
for
> all the other cases though.
> 
> On 2013/01/04 01:34:50, dhollowa wrote:
> > Yes, this is unfortunate.  A possible solution would be to extend the
WDResult
> > class to take an optional callback to be called when its (new) |Destroy|
> method
> > is called.
>

https://codereview.chromium.org/11761016/diff/7001/chrome/browser/api/webdata...
File chrome/browser/api/webdata/web_data_results.h (right):

https://codereview.chromium.org/11761016/diff/7001/chrome/browser/api/webdata...
chrome/browser/api/webdata/web_data_results.h:57: WDTypedResult(WDResultType
type, const DestroyCallback& callback)
It seems reasonable to me in this case to have a two constructors, one with the
callback param and one without.  Then we won't have to litter the common case
with dummy callbacks.

https://codereview.chromium.org/11761016/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/11761016/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.h:312: virtual void
DestroyAutofillProfileResult(const WDTypedResult* result);
Can these be private?

[Cait (Slow), 2013-01-07 17:08:54]

https://codereview.chromium.org/11761016/diff/7001/chrome/browser/api/webdata...
File chrome/browser/api/webdata/web_data_results.h (right):

https://codereview.chromium.org/11761016/diff/7001/chrome/browser/api/webdata...
chrome/browser/api/webdata/web_data_results.h:57: WDTypedResult(WDResultType
type, const DestroyCallback& callback)
On 2013/01/04 23:57:36, dhollowa wrote:
> It seems reasonable to me in this case to have a two constructors, one with
the
> callback param and one without.  Then we won't have to litter the common case
> with dummy callbacks.

Done.

https://codereview.chromium.org/11761016/diff/7001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/11761016/diff/7001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.h:312: virtual void
DestroyAutofillProfileResult(const WDTypedResult* result);
On 2013/01/04 23:57:36, dhollowa wrote:
> Can these be private?

Done.

[dhollowa, 2013-01-07 21:27:13]

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_results.h (right):

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_results.h:76: explicit WDResult(WDResultType
type, const T& v)
nit: omit "explicit", only needed for single-param ctor.

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.h:479: scoped_ptr<WebDataRequestManager>
request_manager_;
It appears the request_manager_'s lifetime is the same as the WDS.  So we could
omit the scoped_ptr and embed directly, yes?

[Cait (Slow), 2013-01-07 22:43:31]

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_results.h (right):

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_results.h:76: explicit WDResult(WDResultType
type, const T& v)
On 2013/01/07 21:27:13, dhollowa wrote:
> nit: omit "explicit", only needed for single-param ctor.

Done.

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.h:479: scoped_ptr<WebDataRequestManager>
request_manager_;
On 2013/01/07 21:27:13, dhollowa wrote:
> It appears the request_manager_'s lifetime is the same as the WDS.  So we
could
> omit the scoped_ptr and embed directly, yes?

Done.

[dhollowa, 2013-01-07 23:25:51]

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/11761016/diff/20001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.h:479: scoped_ptr<WebDataRequestManager>
request_manager_;
On 2013/01/07 22:43:32, caitkp wrote:
> On 2013/01/07 21:27:13, dhollowa wrote:
> > It appears the request_manager_'s lifetime is the same as the WDS.  So we
> could
> > omit the scoped_ptr and embed directly, yes?
> 
> Done.

Sorry, should have been more clear.  Embed the object, not the pointer to the
object.

https://codereview.chromium.org/11761016/diff/27001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_results.h (right):

https://codereview.chromium.org/11761016/diff/27001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_results.h:57: explicit
WDTypedResult(WDResultType type)
Looks like Mac is not happy with inline ctor.

[dhollowa, 2013-01-08 21:24:32]

https://codereview.chromium.org/11761016/diff/37001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.h (right):

https://codereview.chromium.org/11761016/diff/37001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:148: virtual
~WebDataRequestManager();
Do any of the methods in this file need to be virtual?  I would think not since
this class is only used as a utility / implementation class, and there are no
tests.

[Cait (Slow), 2013-01-08 22:43:13]

https://codereview.chromium.org/11761016/diff/37001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_request_manager.h (right):

https://codereview.chromium.org/11761016/diff/37001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_request_manager.h:148: virtual
~WebDataRequestManager();
On 2013/01/08 21:24:33, dhollowa wrote:
> Do any of the methods in this file need to be virtual?  I would think not
since
> this class is only used as a utility / implementation class, and there are no
> tests.

Done.

[dhollowa, 2013-01-08 22:56:38]

LGTM.  Thanks.  This is a nice separation.

[Cait (Slow), 2013-01-09 17:19:16]

[+Jói] for c/b/api changes.

Jói: PTAL at the changes to c/b/api/web_data_request api. I added a "Destroy"
method and optional callback to it to handle the case where the data needs to be
explicitly destroyed if the request is cancelled (e.g. credit cards).

Unfortunately, the addition of the callback tripped the Chromium Style Checker
and it demanded a .cc file, which I'm certain does not belong in c/b/api. Where
should it live though (maybe c/b/webdata/web_data_request_impl)?

Thanks,
Cait

[Jói, 2013-01-09 19:05:52]

LGTM, nice.

https://codereview.chromium.org/11761016/diff/39002/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_results.cc (right):

https://codereview.chromium.org/11761016/diff/39002/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_results.cc:7: //  TODO(caitkp): This should
not live in chrome/browser/api...
nit: Just one space after the //

I think it's actually fine for the .cc file to live here for now, but fine to
keep the TODO around since eventually the whole class should move.

[commit-bot: I haz the power, 2013-01-09 19:35:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11761016/41003

[commit-bot: I haz the power, 2013-01-09 19:36:02]

Presubmit check for 11761016-41003 failed and returned exit status 1.


Running presubmit commit checks ...

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

Presubmit checks took 1.1s to calculate.

[commit-bot: I haz the power, 2013-01-09 19:44:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/11761016/41003

[commit-bot: I haz the power, 2013-01-10 01:25:41]

Change committed as 175965