Remove GenericRequest templates from WebDataService

Remove GenericRequest templates from WebDataService.

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

[Cait (Slow), 2013-01-10 15:48:27]

Hi Erik -- Can you take a look at this change, before I send it off for OWNERS
review?

Thanks,
Cait

[erikwright (departed), 2013-01-10 19:48:10]

A few questions. Overall, LVG.

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:212: WebDataService::Handle handle =
ScheduleDBTaskWithResult(FROM_HERE,
is there a reason for the local variable?

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:237: DCHECK(consumer);
How about putting a DCHECK like this inside ScheduleDBTaskWithResult?

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:554: WebDataRequest* request = new
WebDataRequest(this, NULL,
wrap after '=' or '('

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:554: WebDataRequest* request = new
WebDataRequest(this, NULL,
Your changes here and in your previous CL reveal the fact that there is an
ownership gap here.

Should the task not be posted, or be posted but not run, or for its
implementation to fail to call RequestCompleted for some reason, the
WebDataRequest object is leaked.

Offline we discussed the following:

1) Assign to a scoped_ptr and bind using Passed
2) Keep a pointer to WebDataRequestManager in WebDataRequest
3) Upon completion or cancellation, null out the pointer from (2) (i.e., during
unregistration).
4) Upon WebDataRequest destruction, if still registered, cancel via the request
manager.
5) Upon RequestManager destruction, cancel outstanding requests.

Although the gaps already existed, I think they should be addressed, though in a
separate CL.

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:601: ScheduleTask(FROM_HERE,
Bind(&WebDataService::Commit, this));
Is this now the only caller of ScheduleTask? If so, inline.

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:679: WDTypedResult*
WebDataService::GetWebAppImagesImpl(const GURL& app_url) {
The new style is to return scoped_ptr<WDTypedResult> in cases like this.

[Cait (Slow), 2013-01-11 19:40:38]

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:212: WebDataService::Handle handle =
ScheduleDBTaskWithResult(FROM_HERE,
On 2013/01/10 19:48:10, erikwright wrote:
> is there a reason for the local variable?

Done.

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:237: DCHECK(consumer);
On 2013/01/10 19:48:10, erikwright wrote:
> How about putting a DCHECK like this inside ScheduleDBTaskWithResult?

Done.

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:554: WebDataRequest* request = new
WebDataRequest(this, NULL,
On 2013/01/10 19:48:10, erikwright wrote:
> Your changes here and in your previous CL reveal the fact that there is an
> ownership gap here.
> 
> Should the task not be posted, or be posted but not run, or for its
> implementation to fail to call RequestCompleted for some reason, the
> WebDataRequest object is leaked.
> 
> Offline we discussed the following:
> 
> 1) Assign to a scoped_ptr and bind using Passed
> 2) Keep a pointer to WebDataRequestManager in WebDataRequest
> 3) Upon completion or cancellation, null out the pointer from (2) (i.e.,
during
> unregistration).
> 4) Upon WebDataRequest destruction, if still registered, cancel via the
request
> manager.
> 5) Upon RequestManager destruction, cancel outstanding requests.
> 
> Although the gaps already existed, I think they should be addressed, though in
a
> separate CL.
Ownership gap is fixed in https://codereview.chromium.org/11862010/ (depends on
this CL).

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:554: WebDataRequest* request = new
WebDataRequest(this, NULL,
On 2013/01/10 19:48:10, erikwright wrote:
> wrap after '=' or '('

Done.

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:601: ScheduleTask(FROM_HERE,
Bind(&WebDataService::Commit, this));
On 2013/01/10 19:48:10, erikwright wrote:
> Is this now the only caller of ScheduleTask? If so, inline.
It's actually called in 5 places (init'ing and shutting down DB etc.) so I'm
leaving it in for now.

https://codereview.chromium.org/11783091/diff/8002/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.cc:679: WDTypedResult*
WebDataService::GetWebAppImagesImpl(const GURL& app_url) {
On 2013/01/10 19:48:10, erikwright wrote:
> The new style is to return scoped_ptr<WDTypedResult> in cases like this.

Done.

[Cait (Slow), 2013-01-14 15:26:43]

Hi David -- PTAL. This is round 2 of WebDataService refactoring. I removed the
GenericRequest templates (which I think were left over from the pre-base::Bind
era), and added a couple task-wrappers get rid of some boiler-plate code.

Thanks,
Cait

[erikwright (departed), 2013-01-14 15:51:35]

LGTM with two small changes.

Big improvement!

https://codereview.chromium.org/11783091/diff/17001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.cc (right):

https://codereview.chromium.org/11783091/diff/17001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.cc:576:
request->SetResult(result.release());
remove line 575

line 576:
request->SetResult(task.Run().Pass());

scoped_ptr::release should be pretty rare in new code.

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

https://codereview.chromium.org/11783091/diff/17001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.h:16: #include "base/callback.h"
I don't see why this is required here, but if it is, the subsequent include of
callback_forward should be removed.

Try moving this to the .cc file.

[dhollowa, 2013-01-14 19:12:49]

Very nice.  Glad to see this code finally moving into the modern age.  LGTM.

[commit-bot: I haz the power, 2013-01-14 19:15:29]

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

[commit-bot: I haz the power, 2013-01-15 01:45:00]

Change committed as 176775