Implements JS bindings for mojo shared buffer.

Implements JS bindings for mojo shared buffer.

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

[alokp, 2017-02-27 19:50:44]

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

[alokp, 2017-02-27 19:50:45]

This is a WIP. I ran into a few issues while implementing bindings for shared
buffer, so I have left a few questions for you guys in the patch.

Could you please look? Thanks!

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:105: // TODO(alokp): Check if
we need to track all mappings so that they can be
yzshen@: The current js-binding implementation does not track the various
mappings for a shared buffer. But I think we should. Otherwise the mappings
would keep holding on to invalid memory when the handle gets closed. I am
assuming that shared buffer associated with a handle gets destroyed on
MojoClose?

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp:14: // TODO(alokp): We
need to create an ArrayBuffer that does not own data.
jbroman@: I could not find a way to create an array buffer that does not own the
underlying data. We need an External v8::ArrayBuffer here:
https://cs.chromium.org/chromium/src/v8/include/v8.h?dr&l=4059

Is there a way to create such an array buffer in blink?

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp:19: // TODO(alokp):
Neuter m_buffer.
jbroman@: Is Neutering the correct thing to do here?

[jbroman, 2017-02-27 20:14:44]

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp:14: // TODO(alokp): We
need to create an ArrayBuffer that does not own data.
On 2017/02/27 at 19:50:45, alokp wrote:
> jbroman@: I could not find a way to create an array buffer that does not own
the underlying data. We need an External v8::ArrayBuffer here:
> https://cs.chromium.org/chromium/src/v8/include/v8.h?dr&l=4059
> 
> Is there a way to create such an array buffer in blink?

blink::DOMArrayBufferBase doesn't presently support further externalizing it. I
believe it currently assumes that all array buffers are in-process, and
allocated by PartitionAlloc (in the "array buffer" partition).

While you could in principle create a v8 array buffer backed by one (there's an
overload of New for that), that would be problematic because at present, Blink
assumes that all externalized buffers were externalized by it. Thus
V8ArrayBuffer::toImpl (or V8SharedArrayBuffer::toImpl) would have a CHECK
failure.

If you want to be able to expose a Mojo-allocated buffer in this way, you'll
have to either teach Blink to accept that there are externalized V8 buffers that
aren't DOMArrayBufferBase, or more likely, teach DOMArrayBufferBase that it
could have a buffer that comes from Mojo as opposed to from PartitionAlloc.

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp:19: // TODO(alokp):
Neuter m_buffer.
On 2017/02/27 at 19:50:45, alokp wrote:
> jbroman@: Is Neutering the correct thing to do here?

Yes, probably.

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoSharedBuffer.idl (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.idl:8: readonly attribute
ArrayBuffer buffer;
Can this be mapped in multiple threads/processes at once? (i.e., do we need to
think about whether SharedArrayBuffer is necessary here, and what promises V8
does or does not make about when writes are flushed?)

[alokp, 2017-02-27 20:30:35]

Thanks for the quick response!

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp:14: // TODO(alokp): We
need to create an ArrayBuffer that does not own data.
On 2017/02/27 20:14:44, jbroman wrote:
> On 2017/02/27 at 19:50:45, alokp wrote:
> > jbroman@: I could not find a way to create an array buffer that does not own
> the underlying data. We need an External v8::ArrayBuffer here:
> > https://cs.chromium.org/chromium/src/v8/include/v8.h?dr&l=4059
> > 
> > Is there a way to create such an array buffer in blink?
> 
> blink::DOMArrayBufferBase doesn't presently support further externalizing it.
I
> believe it currently assumes that all array buffers are in-process, and
> allocated by PartitionAlloc (in the "array buffer" partition).
> 
> While you could in principle create a v8 array buffer backed by one (there's
an
> overload of New for that), that would be problematic because at present, Blink
> assumes that all externalized buffers were externalized by it. Thus
> V8ArrayBuffer::toImpl (or V8SharedArrayBuffer::toImpl) would have a CHECK
> failure.
> 
> If you want to be able to expose a Mojo-allocated buffer in this way, you'll
> have to either teach Blink to accept that there are externalized V8 buffers
that
> aren't DOMArrayBufferBase, or more likely, teach DOMArrayBufferBase that it
> could have a buffer that comes from Mojo as opposed to from PartitionAlloc.

I think the latter - teaching DOMArrayBufferBase about external buffers - might
be easier. I will look into adding this support.

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoSharedBuffer.idl (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.idl:8: readonly attribute
ArrayBuffer buffer;
On 2017/02/27 20:14:44, jbroman wrote:
> Can this be mapped in multiple threads/processes at once? (i.e., do we need to
> think about whether SharedArrayBuffer is necessary here, and what promises V8
> does or does not make about when writes are flushed?)

Now that I think about it, I named this interface wrong. It should instead be
called MojoMappedBuffer. It does not represent the shared buffer but a memory
region in the shared buffer.

I do not think SharedArrayBuffer is applicable here. Mojo core layer should take
care of read/write synchronization between multiple mappings into a common
buffer. yzshen@: Is my understanding correct?

[yzshen1, 2017-02-27 21:05:27]

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:105: // TODO(alokp): Check if
we need to track all mappings so that they can be
On 2017/02/27 19:50:45, alokp wrote:
> yzshen@: The current js-binding implementation does not track the various
> mappings for a shared buffer. But I think we should. Otherwise the mappings
> would keep holding on to invalid memory when the handle gets closed. I am
> assuming that shared buffer associated with a handle gets destroyed on
> MojoClose?

The lifetime of mappings is not tied to that of the mojo handle. Actually mojo
keeps a duplicated shared memory handle for each mapping internally.

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoSharedBuffer.idl (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.idl:8: readonly attribute
ArrayBuffer buffer;
On 2017/02/27 20:30:34, alokp wrote:
> On 2017/02/27 20:14:44, jbroman wrote:
> > Can this be mapped in multiple threads/processes at once? (i.e., do we need
to
> > think about whether SharedArrayBuffer is necessary here, and what promises
V8
> > does or does not make about when writes are flushed?)
> 
> Now that I think about it, I named this interface wrong. It should instead be
> called MojoMappedBuffer. It does not represent the shared buffer but a memory
> region in the shared buffer.
> 
> I do not think SharedArrayBuffer is applicable here. Mojo core layer should
take
> care of read/write synchronization between multiple mappings into a common
> buffer. yzshen@: Is my understanding correct?

I don't think mojo provides any extra read/write synchronization on top of OS
shared memory API.

[alokp, 2017-02-28 00:22:15]

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp (right):

https://codereview.chromium.org/2720873002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/mojo/MojoSharedBuffer.cpp:14: // TODO(alokp): We
need to create an ArrayBuffer that does not own data.
On 2017/02/27 20:30:34, alokp wrote:
> On 2017/02/27 20:14:44, jbroman wrote:
> > On 2017/02/27 at 19:50:45, alokp wrote:
> > > jbroman@: I could not find a way to create an array buffer that does not
own
> > the underlying data. We need an External v8::ArrayBuffer here:
> > > https://cs.chromium.org/chromium/src/v8/include/v8.h?dr&l=4059
> > > 
> > > Is there a way to create such an array buffer in blink?
> > 
> > blink::DOMArrayBufferBase doesn't presently support further externalizing
it.
> I
> > believe it currently assumes that all array buffers are in-process, and
> > allocated by PartitionAlloc (in the "array buffer" partition).
> > 
> > While you could in principle create a v8 array buffer backed by one (there's
> an
> > overload of New for that), that would be problematic because at present,
Blink
> > assumes that all externalized buffers were externalized by it. Thus
> > V8ArrayBuffer::toImpl (or V8SharedArrayBuffer::toImpl) would have a CHECK
> > failure.
> > 
> > If you want to be able to expose a Mojo-allocated buffer in this way, you'll
> > have to either teach Blink to accept that there are externalized V8 buffers
> that
> > aren't DOMArrayBufferBase, or more likely, teach DOMArrayBufferBase that it
> > could have a buffer that comes from Mojo as opposed to from PartitionAlloc.
> 
> I think the latter - teaching DOMArrayBufferBase about external buffers -
might
> be easier. I will look into adding this support.

A quick patch to support externalized ArrayBuffer:
https://codereview.chromium.org/2719883004/

[alokp, 2017-03-01 19:43:34]

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

[alokp, 2017-03-01 19:43:49]

This is ready for review. PTAL.

[commit-bot: I haz the power, 2017-03-01 19:44:21]

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-01 21:51:27]

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

[commit-bot: I haz the power, 2017-03-01 21:51:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yzshen1, 2017-03-01 23:11:47]

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/mojo/shared-buffer.html (right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/shared-buffer.html:24: let {result, handle:
handle1} = handle0.clone();
Clone is not a great name in that it sounds like a generic handle operation. I
think it is more clear/consistent to name it duplicateSharedBufferHandle().

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoCreateSharedBufferResult.idl
(right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoCreateSharedBufferResult.idl:6: required
MojoResult result;
nit: I notice that in some files the indent is 4-space while in the others it is
2-space.

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

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.h:42: void mapBuffer(unsigned
offset, unsigned numBytes, MojoMapBufferResult&);
Do we also need a way to explicitly release the mapping, instead of waiting for
the JS object to be garbage-collected? Just like we have a close() method for
mojo handle.

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.h:43: void clone(const
MojoCloneBufferHandleFlags&, MojoCreateSharedBufferResult&);
Does it make sense to make the names more consistent with the C API:
duplicateBufferHandle / MojoDuplicateBufferHandleFlags?

[jbroman, 2017-03-01 23:29:37]

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/mojo/shared-buffer.html (right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/shared-buffer.html:42: let {buffer: buffer1}
= handle1.mapBuffer(0, kBufferSize);
nit: Consider verifying that handle1 != handle0 and buffer1 != buffer0.

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoCloneBufferHandleFlags.idl (right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoCloneBufferHandleFlags.idl:6: boolean
readOnly = false;
I'd missed the first time through that these can be read-only. Since we don't
have any support for a "read-only" array buffer, the resulting ArrayBuffer (and
any views into it) will still seem writable. I assume we'll get a renderer crash
(due to memory access violation) if script ever does write it.

mojo/edk/js/ already has this issue, so I assume you're aware of/okay with it.
(Naturally this means that any script that has access to a mapped buffer must be
extremely careful.)

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

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.h:42: void mapBuffer(unsigned
offset, unsigned numBytes, MojoMapBufferResult&);
On 2017/03/01 at 23:11:46, yzshen1 wrote:
> Do we also need a way to explicitly release the mapping, instead of waiting
for the JS object to be garbage-collected? Just like we have a close() method
for mojo handle.

+1, this is probably a good thing to have. (I thought it was there in a previous
CL; I didn't mean that it had to be removed. Just that GC should suffice if
script doesn't unmap it first.)

[yzshen1, 2017-03-01 23:33:08]

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoCloneBufferHandleFlags.idl (right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoCloneBufferHandleFlags.idl:6: boolean
readOnly = false;
On 2017/03/01 23:29:37, jbroman wrote:
> I'd missed the first time through that these can be read-only. Since we don't
> have any support for a "read-only" array buffer, the resulting ArrayBuffer
(and
> any views into it) will still seem writable. I assume we'll get a renderer
crash
> (due to memory access violation) if script ever does write it.
> 
> mojo/edk/js/ already has this issue, so I assume you're aware of/okay with it.
> (Naturally this means that any script that has access to a mapped buffer must
be
> extremely careful.)

Alok and I chatted about this. Because at the moment only some special contexts
have access to the Mojo APIs (WebUI, layout tests, etc), it should be okay. In
the future if we want to expose this to arbitrary web pages, we will need to
carefully evaluate its security implication, I think.

[alokp, 2017-03-01 23:58:35]

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/mojo/shared-buffer.html (right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/shared-buffer.html:24: let {result, handle:
handle1} = handle0.clone();
On 2017/03/01 23:11:46, yzshen1 wrote:
> Clone is not a great name in that it sounds like a generic handle operation. I
> think it is more clear/consistent to name it duplicateSharedBufferHandle().

I was following the C++ API:
https://cs.chromium.org/chromium/src/mojo/public/cpp/system/buffer.h?l=64

If you like "duplicate" better than "clone", I am fine with it.

The reason we do not have the suffix "SharedBufferHandle" is because the plan is
to move these member functions into MojoSharedBufferHandle, a subclass of
MojoHandle, which makes the suffix redundant.

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/shared-buffer.html:42: let {buffer: buffer1}
= handle1.mapBuffer(0, kBufferSize);
On 2017/03/01 23:29:37, jbroman wrote:
> nit: Consider verifying that handle1 != handle0 and buffer1 != buffer0.

Done. Added "handle1 != handle0" checks to clone tests and "buffer1 != buffer0"
to RW tests.

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoCreateSharedBufferResult.idl
(right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoCreateSharedBufferResult.idl:6: required
MojoResult result;
On 2017/03/01 23:11:46, yzshen1 wrote:
> nit: I notice that in some files the indent is 4-space while in the others it
is
> 2-space.

jbroman: What is the convention here? I remember reading somewhere that idl
files need to have 4 space indents?

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

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.h:42: void mapBuffer(unsigned
offset, unsigned numBytes, MojoMapBufferResult&);
On 2017/03/01 23:29:37, jbroman wrote:
> On 2017/03/01 at 23:11:46, yzshen1 wrote:
> > Do we also need a way to explicitly release the mapping, instead of waiting
> for the JS object to be garbage-collected? Just like we have a close() method
> for mojo handle.
> 
> +1, this is probably a good thing to have. (I thought it was there in a
previous
> CL; I didn't mean that it had to be removed. Just that GC should suffice if
> script doesn't unmap it first.)

I am not sure what the usecase for unmap would be though. Handle::close makes
sense because this is also a mechanism to trigger a connection error on the
other end at a predictable time.

Mapped shared buffer OTOH owns its own ArrayBuffer. Unmap is purely a resource
destruction mechanism with no side effect. Adding Unmap also makes the API
clunky because you need to create wrapper for ArrayBuffer.

That said, I do not feel strongly about this. I can be easily convinced
otherwise.

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.h:43: void clone(const
MojoCloneBufferHandleFlags&, MojoCreateSharedBufferResult&);
On 2017/03/01 23:11:46, yzshen1 wrote:
> Does it make sense to make the names more consistent with the C API:
> duplicateBufferHandle / MojoDuplicateBufferHandleFlags?

Please see my other comment. I am happy to change it to duplicate if you like it
better.

[yzshen1, 2017-03-02 17:41:07]

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

[yzshen1, 2017-03-02 17:41:08]

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/mojo/shared-buffer.html (right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/mojo/shared-buffer.html:24: let {result, handle:
handle1} = handle0.clone();
On 2017/03/01 23:58:34, alokp wrote:
> On 2017/03/01 23:11:46, yzshen1 wrote:
> > Clone is not a great name in that it sounds like a generic handle operation.
I
> > think it is more clear/consistent to name it duplicateSharedBufferHandle().
> 
> I was following the C++ API:
> https://cs.chromium.org/chromium/src/mojo/public/cpp/system/buffer.h?l=64
> 
> If you like "duplicate" better than "clone", I am fine with it.
> 
> The reason we do not have the suffix "SharedBufferHandle" is because the plan
is
> to move these member functions into MojoSharedBufferHandle, a subclass of
> MojoHandle, which makes the suffix redundant.

I would vote for "duplicate" to be consistent with the C API. (And it makes
sense to me to change the C++ API to duplicate as well.) +CC rockot for
bikeshedding

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

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoHandle.h:42: void mapBuffer(unsigned
offset, unsigned numBytes, MojoMapBufferResult&);
On 2017/03/01 23:58:34, alokp wrote:
> On 2017/03/01 23:29:37, jbroman wrote:
> > On 2017/03/01 at 23:11:46, yzshen1 wrote:
> > > Do we also need a way to explicitly release the mapping, instead of
waiting
> > for the JS object to be garbage-collected? Just like we have a close()
method
> > for mojo handle.
> > 
> > +1, this is probably a good thing to have. (I thought it was there in a
> previous
> > CL; I didn't mean that it had to be removed. Just that GC should suffice if
> > script doesn't unmap it first.)
> 
> I am not sure what the usecase for unmap would be though. Handle::close makes
> sense because this is also a mechanism to trigger a connection error on the
> other end at a predictable time.
> 
> Mapped shared buffer OTOH owns its own ArrayBuffer. Unmap is purely a resource
> destruction mechanism with no side effect. Adding Unmap also makes the API
> clunky because you need to create wrapper for ArrayBuffer.
> 
> That said, I do not feel strongly about this. I can be easily convinced
> otherwise.

Because the mappings holds the underlying shared memory alive, it seems nice to
me to be able to explicitly release the resource if the user wants to.

I don't feel very strongly either. I am fine to add it later when/if we see the
need.

[Ken Rockot(use gerrit already), 2017-03-02 18:03:15]

On 2017/03/02 at 17:41:08, yzshen wrote:
>
https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/mojo/shared-buffer.html (right):
> 
>
https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/mojo/shared-buffer.html:24: let {result,
handle: handle1} = handle0.clone();
> On 2017/03/01 23:58:34, alokp wrote:
> > On 2017/03/01 23:11:46, yzshen1 wrote:
> > > Clone is not a great name in that it sounds like a generic handle
operation. I
> > > think it is more clear/consistent to name it
duplicateSharedBufferHandle().
> > 
> > I was following the C++ API:
> > https://cs.chromium.org/chromium/src/mojo/public/cpp/system/buffer.h?l=64
> > 
> > If you like "duplicate" better than "clone", I am fine with it.
> > 
> > The reason we do not have the suffix "SharedBufferHandle" is because the
plan is
> > to move these member functions into MojoSharedBufferHandle, a subclass of
> > MojoHandle, which makes the suffix redundant.
> 
> I would vote for "duplicate" to be consistent with the C API. (And it makes
sense to me to change the C++ API to duplicate as well.) +CC rockot for
bikeshedding
> 

+1 for "duplicate". That verb also mirrors the SharedBufferHandle API in //base
which seems like a nice property to me.

>
https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/mojo/MojoHandle.h (right):
> 
>
https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/mojo/MojoHandle.h:42: void mapBuffer(unsigned
offset, unsigned numBytes, MojoMapBufferResult&);
> On 2017/03/01 23:58:34, alokp wrote:
> > On 2017/03/01 23:29:37, jbroman wrote:
> > > On 2017/03/01 at 23:11:46, yzshen1 wrote:
> > > > Do we also need a way to explicitly release the mapping, instead of
waiting
> > > for the JS object to be garbage-collected? Just like we have a close()
method
> > > for mojo handle.
> > > 
> > > +1, this is probably a good thing to have. (I thought it was there in a
> > previous
> > > CL; I didn't mean that it had to be removed. Just that GC should suffice
if
> > > script doesn't unmap it first.)
> > 
> > I am not sure what the usecase for unmap would be though. Handle::close
makes
> > sense because this is also a mechanism to trigger a connection error on the
> > other end at a predictable time.
> > 
> > Mapped shared buffer OTOH owns its own ArrayBuffer. Unmap is purely a
resource
> > destruction mechanism with no side effect. Adding Unmap also makes the API
> > clunky because you need to create wrapper for ArrayBuffer.
> > 
> > That said, I do not feel strongly about this. I can be easily convinced
> > otherwise.
> 
> Because the mappings holds the underlying shared memory alive, it seems nice
to me to be able to explicitly release the resource if the user wants to.
> 
> I don't feel very strongly either. I am fine to add it later when/if we see
the need.

[alokp, 2017-03-02 18:55:32]

The new patch makes the JS API consistent with C API. 

Is everyone OK with adding unmapBuffer when needed?

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/mojo/MojoCreateSharedBufferResult.idl
(right):

https://codereview.chromium.org/2720873002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/MojoCreateSharedBufferResult.idl:6: required
MojoResult result;
On 2017/03/01 23:58:34, alokp wrote:
> On 2017/03/01 23:11:46, yzshen1 wrote:
> > nit: I notice that in some files the indent is 4-space while in the others
it
> is
> > 2-space.
> 
> jbroman: What is the convention here? I remember reading somewhere that idl
> files need to have 4 space indents?

I found the page which specifies 4-space indents and fixed this patch to make
the indenting consistent.
https://www.chromium.org/blink/webidl#TOC-Style

[jbroman, 2017-03-02 20:50:47]

lgtm

I guess I'm okay with adding an unmap later, if you'd really prefer that.

It seems to me like these buffers are likely to be big enough that script may
want to be a good citizen and free them eagerly, rather than potentially having
a large number of shared buffers mapped at once while we wait for GC. It may
also create GC pressure that makes us do a collection when we don't really have
much to do on the actual heap. But admittedly that could be dealt with later.

(For reference, we do have things like ImageBitmap.close which exist to eagerly
drop that kind of large resource -- though most web APIs don't have explicit
free-ing APIs.)

https://codereview.chromium.org/2720873002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/mojo/MojoDuplicateBufferHandleOptionsFlags.idl
(right):

https://codereview.chromium.org/2720873002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoDuplicateBufferHandleOptionsFlags.idl:5:
dictionary MojoDuplicateBufferHandleOptionsFlags {
nit: I'd just inline this into MojoDuplicateBufferHandleOptions. {readOnly:
true} seems like it's just as useful as {flags: {readOnly: true}}, but it's
shorter, simpler and faster. I see the fact that they're nested in Mojo as an
artifact of C vs JS API style.

[yzshen1, 2017-03-02 20:55:34]

LGTM

[alokp, 2017-03-02 21:22:47]

Jeremy: Thanks for the ImageBitmap reference. I did not know about that. I have
added a TODO for unmap for now. I will revisit it.

https://codereview.chromium.org/2720873002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/mojo/MojoDuplicateBufferHandleOptionsFlags.idl
(right):

https://codereview.chromium.org/2720873002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoDuplicateBufferHandleOptionsFlags.idl:5:
dictionary MojoDuplicateBufferHandleOptionsFlags {
On 2017/03/02 20:50:47, jbroman wrote:
> nit: I'd just inline this into MojoDuplicateBufferHandleOptions. {readOnly:
> true} seems like it's just as useful as {flags: {readOnly: true}}, but it's
> shorter, simpler and faster. I see the fact that they're nested in Mojo as an
> artifact of C vs JS API style.

Done.

[alokp, 2017-03-02 23:47:49]

The CQ bit was checked by alokp@chromium.org

[alokp, 2017-03-02 23:47:49]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2720873002/#ps200001
(title: "rebaseline2")

[commit-bot: I haz the power, 2017-03-02 23:48:25]

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-03 01:24:58]

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

[commit-bot: I haz the power, 2017-03-03 01:25:00]

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

[alokp, 2017-03-03 01:35:34]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2017-03-03 01:36:07]

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-03 03:28:25]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1488504934343840,
"parent_rev": "6c9e50b3b25885f6e7c638a7edab2a0c731c46a8", "commit_rev":
"b07ce7263e703b1eddda72bf6c233710809ecf6b"}

[commit-bot: I haz the power, 2017-03-03 03:29:07]

Description was changed from

==========
Implements JS bindings for mojo shared buffer.

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

to

==========
Implements JS bindings for mojo shared buffer.

BUG=647036

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

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

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/b07ce7263e703b1eddda72bf6c23...