Make FileStream::Seek async and add FileStream::SeekSync for sync operation

Make FileStream::Seek async and add FileStream::SeekSync for sync operation

BUG=75548, 113300
TEST=existing tests should pass

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

[kinuko, 2012-04-05 13:00:08]

A new cut for Seek. PTL thanks!

[willchan no longer on Chromium, 2012-04-05 15:12:28]

First set of comments. Some of my comments in the posix implementation apply to
the windows implementation. I need to read things in detail to verify
correctness.

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (left):

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:260: DCHECK(callback_.is_null());
This DCHECK is useful. Let's keep it. It's basically asserting there's no other
async operation currently happening. Please
DCHECK(!weak_ptr_factory_.HasWeakPtrs());

Can you likewise add this for all the IO operations?

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:243: if (!stream.get())
You don't need this, base::Bind() will take care of it for you when you pass a
WeakPtr. See
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/bind_helpers.h&exact_...
and
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/bind_internal.h&l=873.

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:624: void FileStreamPosix::ResetOnIOComplete() {
I'd invalidate weak pointers at the WeakPtrFactory now, because otherwise it'll
only be invalidated when the base::Callback goes out of scope, but since we're
going to call the "reply" callback on async completion right now, we may
re-enter FileStream. This would mean the
DCHECK(!weak_ptr_factory_.HasWeakPtrs()); assertion I suggested would fail.

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_unittes...
File net/base/file_stream_unittest.cc (right):

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_unittes...
net/base/file_stream_unittest.cc:158: ASSERT_EQ(0,
read_stream.SeekSync(FROM_BEGIN, 0));
We need new tests for the asynchronous API too.

[scherkus (not reviewing), 2012-04-05 18:05:02]

(preemptive LGTM for webkit/media/)

[kinuko, 2012-04-06 12:42:37]

Thanks, updated the patch.

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (left):

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:260: DCHECK(callback_.is_null());
On 2012/04/05 15:12:28, willchan wrote:
> This DCHECK is useful. Let's keep it. It's basically asserting there's no
other
> async operation currently happening. Please
> DCHECK(!weak_ptr_factory_.HasWeakPtrs());
> 
> Can you likewise add this for all the IO operations?

Done.  I expect DCHECK(!on_io_complete_.get()) does the similar check as well.

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:243: if (!stream.get())
On 2012/04/05 15:12:28, willchan wrote:
> You don't need this, base::Bind() will take care of it for you when you pass a
> WeakPtr. See
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/bind_helpers.h&exact_...
> and
> http://code.google.com/searchframe#OAMlx_jo-ck/src/base/bind_internal.h&l=873.

In this case stream is not a target object and I doubt Bind takes care of this
case.  (Actually without this if test fails with null point reference)

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:624: void FileStreamPosix::ResetOnIOComplete() {
On 2012/04/05 15:12:28, willchan wrote:
> I'd invalidate weak pointers at the WeakPtrFactory now, because otherwise
it'll
> only be invalidated when the base::Callback goes out of scope, but since we're
> going to call the "reply" callback on async completion right now, we may
> re-enter FileStream. This would mean the
> DCHECK(!weak_ptr_factory_.HasWeakPtrs()); assertion I suggested would fail.

Good idea, added the invalidation and DCHECKs.

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_unittes...
File net/base/file_stream_unittest.cc (right):

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_unittes...
net/base/file_stream_unittest.cc:158: ASSERT_EQ(0,
read_stream.SeekSync(FROM_BEGIN, 0));
On 2012/04/05 15:12:28, willchan wrote:
> We need new tests for the asynchronous API too.

Added AsyncSeekAround test and replaced SeekSync with Seek in Async{Read,Write}
tests.

[willchan no longer on Chromium, 2012-04-09 12:33:57]

Looks like there's a crash in net_unittests on Windows. Probably a regression,
you should check it out.

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.cc
File net/base/file_stream_posix.cc (right):

http://codereview.chromium.org/9949011/diff/3003/net/base/file_stream_posix.c...
net/base/file_stream_posix.cc:243: if (!stream.get())
On 2012/04/06 12:42:37, kinuko wrote:
> On 2012/04/05 15:12:28, willchan wrote:
> > You don't need this, base::Bind() will take care of it for you when you pass
a
> > WeakPtr. See
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/bind_helpers.h&exact_...
> > and
> >
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/bind_internal.h&l=873.
> 
> In this case stream is not a target object and I doubt Bind takes care of this
> case.  (Actually without this if test fails with null point reference)

Woops, totally misread that :)

http://codereview.chromium.org/9949011/diff/4024/net/base/test_completion_cal...
File net/base/test_completion_callback.h (right):

http://codereview.chromium.org/9949011/diff/4024/net/base/test_completion_cal...
net/base/test_completion_callback.h:26: namespace internal {
This should be nested within the net namespace. The TestCompletionCallbackBase
should be nested too.

http://codereview.chromium.org/9949011/diff/4024/net/base/test_completion_cal...
net/base/test_completion_callback.h:55: have_result_ = false;  // Auto-reset for
next callback.
Why did this move out to the template? Why isn't this done in the base class?

[kinuko, 2012-04-09 17:04:36]

Fixed net_unittests on Windows.

http://codereview.chromium.org/9949011/diff/4024/net/base/test_completion_cal...
File net/base/test_completion_callback.h (right):

http://codereview.chromium.org/9949011/diff/4024/net/base/test_completion_cal...
net/base/test_completion_callback.h:26: namespace internal {
On 2012/04/09 12:33:57, willchan wrote:
> This should be nested within the net namespace. The TestCompletionCallbackBase
> should be nested too.

Done.

http://codereview.chromium.org/9949011/diff/4024/net/base/test_completion_cal...
net/base/test_completion_callback.h:55: have_result_ = false;  // Auto-reset for
next callback.
On 2012/04/09 12:33:57, willchan wrote:
> Why did this move out to the template? Why isn't this done in the base class?

Fixed.

[willchan no longer on Chromium, 2012-04-10 06:48:33]

lgtm

http://codereview.chromium.org/9949011/diff/10005/net/base/file_stream_win.cc
File net/base/file_stream_win.cc (right):

http://codereview.chromium.org/9949011/diff/10005/net/base/file_stream_win.cc...
net/base/file_stream_win.cc:393: // Unretained should be fine as we wait signal
on
Nit: s/we wait signal on/we wait for a signal on/