base::Bind: Convert most of webkit/appcache.

base::Bind: Convert most of webkit/appcache.

BUG=none
TEST=none
R=csilv

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

[James Hawkins, 2011-12-20 05:34:46]

+ajwong

[awong, 2011-12-20 23:51:46]

couple of actual comments.

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_req...
File webkit/appcache/appcache_request_handler_unittest.cc (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_req...
webkit/appcache/appcache_request_handler_unittest.cc:174: FROM_HERE,
base::Bind(&MethodWrapper<Method>::Run,
I think this can just be base::Bind(method, base::Unretained(this))

and then you can just drop the whole MethodWrapper bit.

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_res...
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_res...
webkit/appcache/appcache_response.h:124: bool IsIOPending() { return
callback_.is_null() ? false : true; }
"!callback_.is_null()"?

http://codereview.chromium.org/8991001/diff/4002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_dir_url_request_job_unittest.cc (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_dir_url_request_job_unittest.cc:288:
MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure());
This is a behavior change...I think it breaks the tests?

If I'm reading right, we want to get control back from when StartAsync()
completes, delete request_, and then make sure we don't crash.

I think restructing this test as

TestRequestNoRun(...);
// Enqueue a delete before starting the loop so we can
// test what happens if the request is deleted right
// after StartAsync() is run.
MessageLoop::current()->DeleteSoon(requeust_.release());
MessageLoop::current()->RunAllPending();

is more clear.

http://codereview.chromium.org/8991001/diff/4002/webkit/fileapi/file_system_u...
File webkit/fileapi/file_system_url_request_job_unittest.cc (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/fileapi/file_system_u...
webkit/fileapi/file_system_url_request_job_unittest.cc:343:
MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure());
Same comment as previous file.

[James Hawkins, 2011-12-21 00:43:08]

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_req...
File webkit/appcache/appcache_request_handler_unittest.cc (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_req...
webkit/appcache/appcache_request_handler_unittest.cc:174: FROM_HERE,
base::Bind(&MethodWrapper<Method>::Run,
On 2011/12/20 23:51:47, awong wrote:
> I think this can just be base::Bind(method, base::Unretained(this))
> 
> and then you can just drop the whole MethodWrapper bit.

The wrapper calls test->SetUpTest() before calling |method()|.

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_res...
File webkit/appcache/appcache_response.h (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_res...
webkit/appcache/appcache_response.h:124: bool IsIOPending() { return
callback_.is_null() ? false : true; }
On 2011/12/20 23:51:47, awong wrote:
> "!callback_.is_null()"?

Haha, done.

http://codereview.chromium.org/8991001/diff/4002/webkit/fileapi/file_system_d...
File webkit/fileapi/file_system_dir_url_request_job_unittest.cc (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/fileapi/file_system_d...
webkit/fileapi/file_system_dir_url_request_job_unittest.cc:288:
MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure());
On 2011/12/20 23:51:47, awong wrote:
> This is a behavior change...I think it breaks the tests?
> 
> If I'm reading right, we want to get control back from when StartAsync()
> completes, delete request_, and then make sure we don't crash.
> 
> I think restructing this test as
> 
> TestRequestNoRun(...);
> // Enqueue a delete before starting the loop so we can
> // test what happens if the request is deleted right
> // after StartAsync() is run.
> MessageLoop::current()->DeleteSoon(requeust_.release());
> MessageLoop::current()->RunAllPending();
> 
> is more clear.

Done.

http://codereview.chromium.org/8991001/diff/4002/webkit/fileapi/file_system_u...
File webkit/fileapi/file_system_url_request_job_unittest.cc (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/fileapi/file_system_u...
webkit/fileapi/file_system_url_request_job_unittest.cc:343:
MessageLoop::current()->PostTask(FROM_HERE, MessageLoop::QuitClosure());
On 2011/12/20 23:51:47, awong wrote:
> Same comment as previous file.

Done.

[awong, 2011-12-21 00:50:32]

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_req...
File webkit/appcache/appcache_request_handler_unittest.cc (right):

http://codereview.chromium.org/8991001/diff/4002/webkit/appcache/appcache_req...
webkit/appcache/appcache_request_handler_unittest.cc:174: FROM_HERE,
base::Bind(&MethodWrapper<Method>::Run,
On 2011/12/21 00:43:08, James Hawkins wrote:
> On 2011/12/20 23:51:47, awong wrote:
> > I think this can just be base::Bind(method, base::Unretained(this))
> > 
> > and then you can just drop the whole MethodWrapper bit.
> 
> The wrapper calls test->SetUpTest() before calling |method()|.


Gotcha.  Then can we change MethodWrapper to just a template function?

[awong, 2011-12-21 01:07:30]

LGTM