Implements JS bindings for mojo data pipe.

Implements JS bindings for mojo data pipe.

BUG=647036
Review-Url: https://codereview.chromium.org/2732163002
Cr-Commit-Position: refs/heads/master@{#455477}
Committed: https://chromium.googlesource.com/chromium/src/+/a04226e0012f12397a1f653dd264b7849112def2

[alokp, 2017-03-07 05:48:58]

alokp@chromium.org changed reviewers:
+ jbroman@chromium.org, yzshen@chromium.org

[alokp, 2017-03-07 05:48:58]

The patch is functional, but I would appreciate any feedback on splitting
readData API into 4 separate functions.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:21: MojoReadDataResult
readData(MojoReadDataFlags flags, optional unsigned long numBytes, optional
BufferSource buffer);
yzshen/jbroman: MojoReadData is pretty heavily overloaded. It supports four
operations - querying, discarding, reading, and peeking - via MojoReadDataFlags.

I have tried to fold all these four operations into a single readData API, but
it probably makes sense to separate them into four different functions:

MojoReadDataResult queryData();

MojoReadDataResult discardData(MojoReadDataFlags flags, unsigned long numBytes);

MojoReadDataResult readData(MojoReadDataFlags flags, unsigned long numBytes,
BufferSource buffer);

MojoReadDataResult peekData(MojoReadDataFlags flags, unsigned long numBytes,
BufferSource buffer);

WDYT?

[jbroman, 2017-03-07 16:01:44]

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:162: unsigned numBytes,
Any reason for the disparity between read/write, where read requires the caller
to supply the number of bytes to be read, but write infers it from the size of
the buffer source? I could see arguments for either approach.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:185: numBytes =
std::min(bufferLength, numBytes);
Isn't it always an error on the part of the programmer to request |numBytes|
bytes and then supply a buffer smaller than |numBytes|? I'd expect an exception
to be thrown here, rather than silently reading less data.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.h (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.h:46: void
writeData(ArrayBufferOrArrayBufferView&,
nit: I think the ArrayBufferOrArrayBufferView parameter here and below can be
const&.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:21: MojoReadDataResult
readData(MojoReadDataFlags flags, optional unsigned long numBytes, optional
BufferSource buffer);
On 2017/03/07 at 05:48:58, alokp wrote:
> yzshen/jbroman: MojoReadData is pretty heavily overloaded. It supports four
operations - querying, discarding, reading, and peeking - via MojoReadDataFlags.
> 
> I have tried to fold all these four operations into a single readData API, but
it probably makes sense to separate them into four different functions:
> 
> MojoReadDataResult queryData();
> 
> MojoReadDataResult discardData(MojoReadDataFlags flags, unsigned long
numBytes);
> 
> MojoReadDataResult readData(MojoReadDataFlags flags, unsigned long numBytes,
BufferSource buffer);
> 
> MojoReadDataResult peekData(MojoReadDataFlags flags, unsigned long numBytes,
BufferSource buffer);
> 
> WDYT?

I don't feel strongly here.

To me this is a slightly clearer API, as long as the mapping to the Mojo
primitives is still clear (it is to me) and doesn't limit the future evolution
of the Mojo API (I don't see that it does).

Interestingly, the Java bindings seem to go halfway and provide discardData and
readData.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoReadDataFlags.idl (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoReadDataFlags.idl:5: dictionary
MojoReadDataFlags {
super-nit: we'd usually call these "Options" dictionaries rather than "Flags",
but this isn't really important as it's not exposed at all and is trivial to
change later.

[yzshen1, 2017-03-07 17:56:38]

yzshen@chromium.org changed reviewers:
+ rockot@chromium.org

[yzshen1, 2017-03-07 17:56:39]

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:163:
ArrayBufferOrArrayBufferView& buffer,
[just to double check] Is it because having |buffer| param (instead of making it
a return value) enables users to read data into a specified position of a
pre-allocated, bigger buffer?

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:185: numBytes =
std::min(bufferLength, numBytes);
On 2017/03/07 16:01:44, jbroman wrote:
> Isn't it always an error on the part of the programmer to request |numBytes|
> bytes and then supply a buffer smaller than |numBytes|? I'd expect an
exception
> to be thrown here, rather than silently reading less data.

We could return INVALID_ARGUMENT in this case, considering that we always use
result code instead of exception. WDYT?

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:20: MojoWriteDataResult
writeData(BufferSource buffer, optional MojoWriteDataFlags flags);
Is it the plan to add two-phase read/write in a follow-up change?

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:21: MojoReadDataResult
readData(MojoReadDataFlags flags, optional unsigned long numBytes, optional
BufferSource buffer);
On 2017/03/07 05:48:58, alokp wrote:
> yzshen/jbroman: MojoReadData is pretty heavily overloaded. It supports four
> operations - querying, discarding, reading, and peeking - via
MojoReadDataFlags.
> 
> I have tried to fold all these four operations into a single readData API, but
> it probably makes sense to separate them into four different functions:
> 
> MojoReadDataResult queryData();
> 
> MojoReadDataResult discardData(MojoReadDataFlags flags, unsigned long
numBytes);
> 
> MojoReadDataResult readData(MojoReadDataFlags flags, unsigned long numBytes,
> BufferSource buffer);
> 
> MojoReadDataResult peekData(MojoReadDataFlags flags, unsigned long numBytes,
> BufferSource buffer);
> 
> WDYT?

I am fine with this. +CC rockot for API discussion.

[Ken Rockot(use gerrit already), 2017-03-07 18:09:51]

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:21: MojoReadDataResult
readData(MojoReadDataFlags flags, optional unsigned long numBytes, optional
BufferSource buffer);
On 2017/03/07 at 17:56:39, yzshen1 wrote:
> On 2017/03/07 05:48:58, alokp wrote:
> > yzshen/jbroman: MojoReadData is pretty heavily overloaded. It supports four
> > operations - querying, discarding, reading, and peeking - via
MojoReadDataFlags.
> > 
> > I have tried to fold all these four operations into a single readData API,
but
> > it probably makes sense to separate them into four different functions:
> > 
> > MojoReadDataResult queryData();
> > 
> > MojoReadDataResult discardData(MojoReadDataFlags flags, unsigned long
numBytes);
> > 
> > MojoReadDataResult readData(MojoReadDataFlags flags, unsigned long numBytes,
> > BufferSource buffer);
> > 
> > MojoReadDataResult peekData(MojoReadDataFlags flags, unsigned long numBytes,
> > BufferSource buffer);
> > 
> > WDYT?
> 
> I am fine with this. +CC rockot for API discussion.

Seems reasonable to me.

[alokp, 2017-03-07 19:34:20]

This is ready for another round of review. I will add tests once everyone is
happy with the API.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:162: unsigned numBytes,
On 2017/03/07 16:01:44, jbroman wrote:
> Any reason for the disparity between read/write, where read requires the
caller
> to supply the number of bytes to be read, but write infers it from the size of
> the buffer source? I could see arguments for either approach.

My reasoning for not having numBytes on write was because to be really useful,
we would need to also add an offset. But since it is trivial to create a
DataView that supports both - offset and numBytes - it is not necessary.

I could get rid of numBytes from readData now that it is separate from
discardData if that would be more preferable.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:163:
ArrayBufferOrArrayBufferView& buffer,
On 2017/03/07 17:56:39, yzshen1 wrote:
> [just to double check] Is it because having |buffer| param (instead of making
it
> a return value) enables users to read data into a specified position of a
> pre-allocated, bigger buffer?

Right. I think a common usage pattern would be to reuse an ArrayBuffer to read
data as it becomes available. Creating an ArrayBuffer as done by the current
implementation is wasteful IMO.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:185: numBytes =
std::min(bufferLength, numBytes);
On 2017/03/07 17:56:39, yzshen1 wrote:
> On 2017/03/07 16:01:44, jbroman wrote:
> > Isn't it always an error on the part of the programmer to request |numBytes|
> > bytes and then supply a buffer smaller than |numBytes|? I'd expect an
> exception
> > to be thrown here, rather than silently reading less data.
> 
> We could return INVALID_ARGUMENT in this case, considering that we always use
> result code instead of exception. WDYT?

Done.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.h (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.h:46: void
writeData(ArrayBufferOrArrayBufferView&,
On 2017/03/07 16:01:44, jbroman wrote:
> nit: I think the ArrayBufferOrArrayBufferView parameter here and below can be
> const&.

Done.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:20: MojoWriteDataResult
writeData(BufferSource buffer, optional MojoWriteDataFlags flags);
On 2017/03/07 17:56:39, yzshen1 wrote:
> Is it the plan to add two-phase read/write in a follow-up change?

Do you mean MojoBegin[End]ReadData? If so, yes we can add in a later patch or
whenever needed. I could not find any current usage.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:21: MojoReadDataResult
readData(MojoReadDataFlags flags, optional unsigned long numBytes, optional
BufferSource buffer);
On 2017/03/07 18:09:51, Ken Rockot wrote:
> On 2017/03/07 at 17:56:39, yzshen1 wrote:
> > On 2017/03/07 05:48:58, alokp wrote:
> > > yzshen/jbroman: MojoReadData is pretty heavily overloaded. It supports
four
> > > operations - querying, discarding, reading, and peeking - via
> MojoReadDataFlags.
> > > 
> > > I have tried to fold all these four operations into a single readData API,
> but
> > > it probably makes sense to separate them into four different functions:
> > > 
> > > MojoReadDataResult queryData();
> > > 
> > > MojoReadDataResult discardData(MojoReadDataFlags flags, unsigned long
> numBytes);
> > > 
> > > MojoReadDataResult readData(MojoReadDataFlags flags, unsigned long
numBytes,
> > > BufferSource buffer);
> > > 
> > > MojoReadDataResult peekData(MojoReadDataFlags flags, unsigned long
numBytes,
> > > BufferSource buffer);
> > > 
> > > WDYT?
> > 
> > I am fine with this. +CC rockot for API discussion.
> 
> Seems reasonable to me.

Thanks for the feedback. I split readData into three functions - queryData,
discardData, and readData (also supports peekData). PTAL.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoReadDataFlags.idl (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoReadDataFlags.idl:5: dictionary
MojoReadDataFlags {
On 2017/03/07 16:01:44, jbroman wrote:
> super-nit: we'd usually call these "Options" dictionaries rather than "Flags",
> but this isn't really important as it's not exposed at all and is trivial to
> change later.

Done.

[jbroman, 2017-03-07 20:31:47]

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:162: unsigned numBytes,
On 2017/03/07 at 19:34:20, alokp wrote:
> On 2017/03/07 16:01:44, jbroman wrote:
> > Any reason for the disparity between read/write, where read requires the
caller
> > to supply the number of bytes to be read, but write infers it from the size
of
> > the buffer source? I could see arguments for either approach.
> 
> My reasoning for not having numBytes on write was because to be really useful,
we would need to also add an offset. But since it is trivial to create a
DataView that supports both - offset and numBytes - it is not necessary.
> 
> I could get rid of numBytes from readData now that it is separate from
discardData if that would be more preferable.

Mild preference for that (for simplicity, and consistent with the Java
bindings), but I don't feel strongly at all.

https://codereview.chromium.org/2732163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2732163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:18: // TODO(alokp): Create
DataPipeHandle, a subclass of MojoHandle
If you do this, wouldn't it be a MojoDataPipeProducerHandle and a
MojoDataPipeConsumerHandle? IIUC each handle can only be used for one set of
operations.

[jbroman, 2017-03-07 20:34:01]

aside from the layout test (which needs to be updated) and those nits, this
l-g-t-m

[alokp, 2017-03-07 20:49:32]

Thanks Jeremy.

I will wait for Yuzhu/Ken to sign off on the API before adding layout tests.

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2732163002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:162: unsigned numBytes,
On 2017/03/07 20:31:47, jbroman wrote:
> On 2017/03/07 at 19:34:20, alokp wrote:
> > On 2017/03/07 16:01:44, jbroman wrote:
> > > Any reason for the disparity between read/write, where read requires the
> caller
> > > to supply the number of bytes to be read, but write infers it from the
size
> of
> > > the buffer source? I could see arguments for either approach.
> > 
> > My reasoning for not having numBytes on write was because to be really
useful,
> we would need to also add an offset. But since it is trivial to create a
> DataView that supports both - offset and numBytes - it is not necessary.
> > 
> > I could get rid of numBytes from readData now that it is separate from
> discardData if that would be more preferable.
> 
> Mild preference for that (for simplicity, and consistent with the Java
> bindings), but I don't feel strongly at all.

Done.

https://codereview.chromium.org/2732163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2732163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:18: // TODO(alokp): Create
DataPipeHandle, a subclass of MojoHandle
On 2017/03/07 20:31:47, jbroman wrote:
> If you do this, wouldn't it be a MojoDataPipeProducerHandle and a
> MojoDataPipeConsumerHandle? IIUC each handle can only be used for one set of
> operations.

Ah yes - updated comment.

[yzshen1, 2017-03-07 22:14:00]

LGTM with one nit

https://codereview.chromium.org/2732163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2732163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:17: #include
"core/mojo/MojoReadMessageFlags.h"
Maybe we should also change it to "Options"?

[alokp, 2017-03-08 00:45:35]

Jeremy: Ready for your review.

https://codereview.chromium.org/2732163002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2732163002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:17: #include
"core/mojo/MojoReadMessageFlags.h"
On 2017/03/07 22:14:00, yzshen1 wrote:
> Maybe we should also change it to "Options"?

Yes - I will make a pass in another patch to make everything consistent.

[alokp, 2017-03-08 05:20:12]

The CQ bit was checked by alokp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 05:20:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 06:37:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 06:37:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jbroman, 2017-03-08 15:26:46]

lgtm

[alokp, 2017-03-08 15:39:39]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-03-08 15:39:39]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2732163002/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-08 15:40:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 17:01:47]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488987579218500,
"parent_rev": "deb62bfb3b4c7c2b78d6b6205766d9cf10ca39a0", "commit_rev":
"a04226e0012f12397a1f653dd264b7849112def2"}

[commit-bot: I haz the power, 2017-03-08 17:04:01]

Description was changed from

==========
Implements JS bindings for mojo data pipe.

BUG=647036
==========

to

==========
Implements JS bindings for mojo data pipe.

BUG=647036

Review-Url: https://codereview.chromium.org/2732163002
Cr-Commit-Position: refs/heads/master@{#455477}
Committed:
https://chromium.googlesource.com/chromium/src/+/a04226e0012f12397a1f653dd264...
==========

[commit-bot: I haz the power, 2017-03-08 17:04:02]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/a04226e0012f12397a1f653dd264...