Reimplementing FileUtilProxy using PostTaskAndReply

Reimplementing FileUtilProxy using PostTaskAndReply

Splitted from http://codereview.chromium.org/8231004/

BUG=none
TEST=existing tests should pass

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

[kinuko, 2011-10-18 09:45:02]

Deprecating internal MessageLoopRelay in favor of
MessageLoopProxy::PostTaskAndReply.  Could you PTL?  Thanks!

[akalin, 2011-10-18 19:03:05]

On 2011/10/18 09:45:02, kinuko wrote:
> Deprecating internal MessageLoopRelay in favor of
> MessageLoopProxy::PostTaskAndReply.  Could you PTL?  Thanks!

Will, can you take a look at this one?  I'm not too familiar with
PostTaskAndReply.  I'll do the other one.

[willchan no longer on Chromium, 2011-10-19 00:26:51]

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc#newcode54
base/file_util_proxy.cc:54: DCHECK(result);
Why isn't this DCHECK done earlier, perhaps in ReturnAsParam()? It's better to
crash early rather than have crashes where you don't know who posted it.

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc#newcode122
base/file_util_proxy.cc:122: delete this;
I think you should get rid of all these delete this calls. Instead, in the
base::Bind(), you should use base::Owned(helper_obj) for the reply&delete
callback. That'll transfer ownership of the helper_obj to the reply&delete
callback. The reason this is important is because there's no guarantee that the
origin thread is still alive, so the task may never make it back, and we want to
make sure the helper_obj always gets deleted.

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc#newcode129
base/file_util_proxy.cc:129: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc#newcode152
base/file_util_proxy.cc:152: void Reply(scoped_refptr<MessageLoopProxy>
message_loop_proxy,
MessageLoopProxy* instead. What's important is the base::Bind() arg uses a
scoped_refptr so the Callback object will have a scoped_refptr stored there.

[kinuko, 2011-10-19 08:42:04]

Thx, updated.

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc#newcode54
base/file_util_proxy.cc:54: DCHECK(result);
On 2011/10/19 00:26:52, willchan wrote:
> Why isn't this DCHECK done earlier, perhaps in ReturnAsParam()? It's better to
> crash early rather than have crashes where you don't know who posted it.

Done.  Indeed...

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc#newcode122
base/file_util_proxy.cc:122: delete this;
On 2011/10/19 00:26:52, willchan wrote:
> I think you should get rid of all these delete this calls. Instead, in the
> base::Bind(), you should use base::Owned(helper_obj) for the reply&delete
> callback. That'll transfer ownership of the helper_obj to the reply&delete
> callback. The reason this is important is because there's no guarantee that
the
> origin thread is still alive, so the task may never make it back, and we want
to
> make sure the helper_obj always gets deleted.

Done.  Cool.

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc#newcode129
base/file_util_proxy.cc:129: };
On 2011/10/19 00:26:52, willchan wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/8339012/diff/1/base/file_util_proxy.cc#newcode152
base/file_util_proxy.cc:152: void Reply(scoped_refptr<MessageLoopProxy>
message_loop_proxy,
On 2011/10/19 00:26:52, willchan wrote:
> MessageLoopProxy* instead. What's important is the base::Bind() arg uses a
> scoped_refptr so the Callback object will have a scoped_refptr stored there.

Done.  Actually it seems the Close needs to be called in destructor (so that we
can make sure it'll be closed when *this* is destructed) so I moved the code
into dtor.

[willchan no longer on Chromium, 2011-10-21 17:56:50]

lgtm

http://codereview.chromium.org/8339012/diff/6001/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/8339012/diff/6001/base/file_util_proxy.cc#newc...
base/file_util_proxy.cc:78: scoped_refptr<MessageLoopProxy> message_loop_proxy,
Can you use const scoped_refptr<MessageLoopProxy>& here? Generally I don't
advocate using that, but here it's internal and it'll save extra
AddRef()/Release() pairs. Usually I don't nit on the extra AddRef()/Release()
pairs, but this code is low-level enough that maybe it matters.

[kinuko, 2011-10-24 10:14:17]

Thanks, will be submitting..

http://codereview.chromium.org/8339012/diff/6001/base/file_util_proxy.cc
File base/file_util_proxy.cc (right):

http://codereview.chromium.org/8339012/diff/6001/base/file_util_proxy.cc#newc...
base/file_util_proxy.cc:78: scoped_refptr<MessageLoopProxy> message_loop_proxy,
On 2011/10/21 17:56:51, willchan wrote:
> Can you use const scoped_refptr<MessageLoopProxy>& here? Generally I don't
> advocate using that, but here it's internal and it'll save extra
> AddRef()/Release() pairs. Usually I don't nit on the extra AddRef()/Release()
> pairs, but this code is low-level enough that maybe it matters.

Done.