Mojo: DataPipe: Implement "may discard" mode for simple writes.

Mojo: DataPipe: Implement "may discard" mode for simple writes.

(Still need to implement for two-phase writes.)

R=darin@chromium.org

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

[darin (slow to review), 2014-01-06 21:29:27]

LGTM

https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe.cc
File mojo/system/local_data_pipe.cc (right):

https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe.c...
mojo/system/local_data_pipe.cc:70: // TODO(vtl): Consider this return value.
Maybe "out of range" when greater
hmm, your approach makes sense to me.

https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe_u...
File mojo/system/local_data_pipe_unittest.cc (right):

https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe_u...
mojo/system/local_data_pipe_unittest.cc:484: TEST(LocalDataPipeTest, MayDiscard)
{
would be good to test the out of range case.

[viettrungluu, 2014-01-06 21:52:39]

Thanks.

On 2014/01/06 21:29:27, darin wrote:
> LGTM
> 
> https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe.cc
> File mojo/system/local_data_pipe.cc (right):
> 
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe.c...
> mojo/system/local_data_pipe.cc:70: // TODO(vtl): Consider this return value.
> Maybe "out of range" when greater
> hmm, your approach makes sense to me.

It made so much sense to me that I did it ... but forgot to remove the TODO. :)

> 
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe_u...
> File mojo/system/local_data_pipe_unittest.cc (right):
> 
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe_u...
> mojo/system/local_data_pipe_unittest.cc:484: TEST(LocalDataPipeTest,
MayDiscard)
> {
> would be good to test the out of range case.

Yes, which is why I still have a TODO for the all-or-none case. So many tests to
write....

[viettrungluu, 2014-01-06 21:54:03]

Committed patchset #2 manually as r243176 (presubmit successful).

[viettrungluu, 2014-01-06 23:13:13]

On 2014/01/06 21:52:39, viettrungluu wrote:
> Thanks.
> 
> On 2014/01/06 21:29:27, darin wrote:
> > LGTM
> > 
> >
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe.cc
> > File mojo/system/local_data_pipe.cc (right):
> > 
> >
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe.c...
> > mojo/system/local_data_pipe.cc:70: // TODO(vtl): Consider this return value.
> > Maybe "out of range" when greater
> > hmm, your approach makes sense to me.
> 
> It made so much sense to me that I did it ... but forgot to remove the TODO.
:)

D'oh, I realized that "should wait" doesn't really make sense, since you can't
wait for a specified amount of data (only for there to be a nonzero amount of
data). So I guess it'll have to be "should wait" if the amount of data is one
element (and the consumer is still open), otherwise "out of range".

> 
> > 
> >
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe_u...
> > File mojo/system/local_data_pipe_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe_u...
> > mojo/system/local_data_pipe_unittest.cc:484: TEST(LocalDataPipeTest,
> MayDiscard)
> > {
> > would be good to test the out of range case.
> 
> Yes, which is why I still have a TODO for the all-or-none case. So many tests
to
> write....

[viettrungluu, 2014-01-06 23:30:22]

On 2014/01/06 23:13:13, viettrungluu wrote:
> On 2014/01/06 21:52:39, viettrungluu wrote:
> > Thanks.
> > 
> > On 2014/01/06 21:29:27, darin wrote:
> > > LGTM
> > > 
> > >
> https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe.cc
> > > File mojo/system/local_data_pipe.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe.c...
> > > mojo/system/local_data_pipe.cc:70: // TODO(vtl): Consider this return
value.
> > > Maybe "out of range" when greater
> > > hmm, your approach makes sense to me.
> > 
> > It made so much sense to me that I did it ... but forgot to remove the TODO.
> :)
> 
> D'oh, I realized that "should wait" doesn't really make sense, since you can't
> wait for a specified amount of data (only for there to be a nonzero amount of
> data). So I guess it'll have to be "should wait" if the amount of data is one
> element (and the consumer is still open), otherwise "out of range".

On second thought, this behavior is kind of hard to explain, so probably it
should always return "out of range" in "all or none" mode.

> 
> > 
> > > 
> > >
> >
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe_u...
> > > File mojo/system/local_data_pipe_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/98013005/diff/1/mojo/system/local_data_pipe_u...
> > > mojo/system/local_data_pipe_unittest.cc:484: TEST(LocalDataPipeTest,
> > MayDiscard)
> > > {
> > > would be good to test the out of range case.
> > 
> > Yes, which is why I still have a TODO for the all-or-none case. So many
tests
> to
> > write....

[darin (slow to review), 2014-01-07 04:29:33]

OK. Yeah, I found it confusing.


On Mon, Jan 6, 2014 at 3:30 PM, <viettrungluu@chromium.org> wrote:

> On 2014/01/06 23:13:13, viettrungluu wrote:
>
>> On 2014/01/06 21:52:39, viettrungluu wrote:
>> > Thanks.
>> >
>> > On 2014/01/06 21:29:27, darin wrote:
>> > > LGTM
>> > >
>> > >
>> https://codereview.chromium.org/98013005/diff/1/mojo/
>> system/local_data_pipe.cc
>> > > File mojo/system/local_data_pipe.cc (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/98013005/diff/1/mojo/
> system/local_data_pipe.cc#newcode70
>
>> > > mojo/system/local_data_pipe.cc:70: // TODO(vtl): Consider this return
>>
> value.
>
>> > > Maybe "out of range" when greater
>> > > hmm, your approach makes sense to me.
>> >
>> > It made so much sense to me that I did it ... but forgot to remove the
>> TODO.
>> :)
>>
>
>  D'oh, I realized that "should wait" doesn't really make sense, since you
>> can't
>> wait for a specified amount of data (only for there to be a nonzero
>> amount of
>> data). So I guess it'll have to be "should wait" if the amount of data is
>> one
>> element (and the consumer is still open), otherwise "out of range".
>>
>
> On second thought, this behavior is kind of hard to explain, so probably it
> should always return "out of range" in "all or none" mode.
>
>
>
>  >
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/98013005/diff/1/mojo/
> system/local_data_pipe_unittest.cc
>
>> > > File mojo/system/local_data_pipe_unittest.cc (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/98013005/diff/1/mojo/
> system/local_data_pipe_unittest.cc#newcode484
>
>> > > mojo/system/local_data_pipe_unittest.cc:484: TEST(LocalDataPipeTest,
>> > MayDiscard)
>> > > {
>> > > would be good to test the out of range case.
>> >
>> > Yes, which is why I still have a TODO for the all-or-none case. So many
>>
> tests
>
>> to
>> > write....
>>
>
>
>
> https://codereview.chromium.org/98013005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.