Issue 2400563002: Adds Mojo IDL.

alokp

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

4 years, 2 months ago (2016-10-07 00:36:20 UTC) #1

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-07 00:36:46 UTC) #2

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-07 01:08:51 UTC) #3

commit-bot: I haz the power

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

4 years, 2 months ago (2016-10-07 01:08:52 UTC) #4

alokp

Description was changed from ========== Adds Mojo IDL. ========== to ========== [WIP] Adds Mojo IDL. ...

4 years, 2 months ago (2016-10-07 22:16:59 UTC) #5

alokp

Description was changed from ========== [WIP] Adds Mojo IDL. BUG=647036 ========== to ========== [WIP] Adds ...

4 years, 2 months ago (2016-10-07 22:42:37 UTC) #6

Ken Rockot(use gerrit already)

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

4 years, 2 months ago (2016-10-07 23:22:26 UTC) #7

Ken Rockot(use gerrit already)

Cool! Thanks for looking at this. https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/Source/core/mojo/MojoInterfaceProvider.h File third_party/WebKit/Source/core/mojo/MojoInterfaceProvider.h (right): https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/Source/core/mojo/MojoInterfaceProvider.h#newcode1 third_party/WebKit/Source/core/mojo/MojoInterfaceProvider.h:1: // Copyright 2016 ...

4 years, 2 months ago (2016-10-07 23:22:27 UTC) #8

haraken

This looks like a right way to go overall, but I might want to see ...

4 years, 2 months ago (2016-10-08 01:30:29 UTC) #9

esprehn

esprehn@chromium.org changed reviewers: + esprehn@chromium.org

4 years, 2 months ago (2016-10-08 03:52:09 UTC) #10

alokp

haraken@: Thanks for the feedback. This patch simply replaces the gin bindings with that generated ...

4 years, 2 months ago (2016-10-10 16:41:22 UTC) #11

Ken Rockot(use gerrit already)

On 2016/10/10 at 16:41:22, alokp wrote: > haraken@: Thanks for the feedback. This patch simply ...

4 years, 2 months ago (2016-10-10 17:11:25 UTC) #12

On 2016/10/10 at 16:41:22, alokp wrote:
> haraken@: Thanks for the feedback. This patch simply replaces the gin bindings
with that generated using WebIDL, with the exact same API.
> 
> I will send out a design doc to iterate over the native bindings API and
figure out how clients would host and consume the public mojo-js API
(mojo/public/js). Is platform-architecture-dev@ the correct mailing list for
sending out the design doc?
> 
>
https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/mojo/MojoInterfaceProvider.h (right):
> 
>
https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/mojo/MojoInterfaceProvider.h:1: // Copyright
2016 The Chromium Authors. All rights reserved.
> On 2016/10/07 23:22:27, Ken Rockot wrote:
> > There can be many InterfaceProviders in the system, and InterfaceProvider
itself
> > is just a mojom type with generated bindings.
> > 
> > It might make sense to have some native bindings for exposing various
> > InterfaceProvider pipes to JS (e.g. mojo.getFrameInterfaceProviderHandle),
but
> > this should only provide a message pipe handle.
> > 
> > You can then use generated JS bindings for InterfaceProvider to bind that
handle
> > to an InterfaceProvider proxy object which will write messages to the pipe.
> 
> Good idea. In fact it might be more flexible have a dictionary of message-pipe
handles as an attribute on the native binding object.

I'm hesitant to stabilize any kind of specific pipe exposure API to JS at this
time. I think you should treat that as an implementation detail of your own
feature for now, i.e. have a single cast-specific binding which acquires your
primordial interface provider for cast interfaces.

> 
> That said I am trying to keep this API same as the one exposed by gin bindings
so that we can seamlessly transition to this one without changing the code in
mojo/public/js and gin bindings. Once we have transitioned I will make another
cleanup pass. Does that sound reasonable?

I don't understand why you'd port the InterfaceProvider stuff at all. Existing
code which gets an InterfaceProvider from the gin module will still work even if
the core bindings are moved off of gin. Since we're still going to be using gin
modules for generated bindings and non-native core JS in the near-to-medium
term, it seems like a separate issue from porting the native core library.

One thing you would also want to do is update the generated bindings and JS in
mojo/public/js to use your new IDL bindings calls, e.g. calling mojoCore.close
directly instead of calling ('mojo/public/js/core').close

https://codereview.chromium.org/2400563002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2400563002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:2555: registry->AddBuiltinModule(
I don't think this code should change yet. We should continue to expose the old
mojo/public/js/core module as it exists today, until the entire core API is
ported. I also don't think you want to use |registry| to expose your new
bindings object.

I'd rather we just stash the new core in a global object like window.mojoCore or
something similar (preferably just 'mojo' but that's already taken in layout
tests at the moment - can fix once the rest of the API is ported.)

Then JS contexts which are allowed to see mojo, like layout tests and your cast
client code, don't need to do any requires/defines/whatever to get the core API.
They can call mojoCore.createMessagePipe etc immediately.

alokp

> > Good idea. In fact it might be more flexible have a dictionary of ...

4 years, 2 months ago (2016-10-10 17:55:32 UTC) #13

> > Good idea. In fact it might be more flexible have a dictionary of
message-pipe
> handles as an attribute on the native binding object.
> 
> I'm hesitant to stabilize any kind of specific pipe exposure API to JS at this
> time. I think you should treat that as an implementation detail of your own
> feature for now, i.e. have a single cast-specific binding which acquires your
> primordial interface provider for cast interfaces.

We are not stabilizing anything in this patch. We already expose two
InterfaceProvider modules for acquiring message-pipe handles as
"content/public/renderer/frame_interfaces" and
"content/public/renderer/interfaces". This patch just replaces the
implementation for the InterfaceProvider module. Why was it necessary to make
this change in this patch? Please see below.

> > 
> > That said I am trying to keep this API same as the one exposed by gin
bindings
> so that we can seamlessly transition to this one without changing the code in
> mojo/public/js and gin bindings. Once we have transitioned I will make another
> cleanup pass. Does that sound reasonable?
> 
> I don't understand why you'd port the InterfaceProvider stuff at all. Existing
> code which gets an InterfaceProvider from the gin module will still work even
if
> the core bindings are moved off of gin. Since we're still going to be using
gin
> modules for generated bindings and non-native core JS in the near-to-medium
> term, it seems like a separate issue from porting the native core library.

Yeah that's exactly what I wanted to do initially (Patch Set 3). But it does not
work because of MojoHandle type mismatch between gin and blink.
InterfaceProviderJsWrapper::GetInterface returns gin handle, while the core
functions implemented in blink expect the MojoHandle in IDL. May be there is an
easier way to reconcile the two, but it seemed more straightforward (to me) to
just port InterfaceProvider. I would prefer to not have to port
InterfaceProvider in this patch if possible. I am open to suggestions on how to
do that.

> 
>
https://codereview.chromium.org/2400563002/diff/80001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/2400563002/diff/80001/content/renderer/render...
> content/renderer/render_frame_impl.cc:2555: registry->AddBuiltinModule(
> I don't think this code should change yet. We should continue to expose the
old
> mojo/public/js/core module as it exists today, until the entire core API is
> ported. I also don't think you want to use |registry| to expose your new
> bindings object.
> 
> I'd rather we just stash the new core in a global object like window.mojoCore
or
> something similar (preferably just 'mojo' but that's already taken in layout
> tests at the moment - can fix once the rest of the API is ported.)
> 
> Then JS contexts which are allowed to see mojo, like layout tests and your
cast
> client code, don't need to do any requires/defines/whatever to get the core
API.
> They can call mojoCore.createMessagePipe etc immediately.

Again - thats exactly what I was planning to do initially (see Patch Set 2) -
install a global "mojo" object. But then this patch becomes too large -
especially because I also need to port all other gin modules. To contain the
size of this patch, I decided to just remove the dependency on mojo/edk/js by
implementing the modules in blink instead of gin:Wrappable. That way I do not
need to touch mojo/public/js or generated bindings.

Sounds like I should send out a design doc that also includes the transition
plan to gather feedback :)

esprehn

I think we want this simpler, just add new interfaces: MojoHandle MojoInterfaceProvider close() should be ...

4 years, 2 months ago (2016-10-10 20:59:46 UTC) #14

Ken Rockot(use gerrit already)

On Mon, Oct 10, 2016 at 1:59 PM, <esprehn@chromium.org> wrote: > I think we want ...

4 years, 2 months ago (2016-10-10 21:05:30 UTC) #15

On Mon, Oct 10, 2016 at 1:59 PM, <esprehn@chromium.org> wrote:

> I think we want this simpler, just add new interfaces:
>
> MojoHandle
> MojoInterfaceProvider
>
> close() should be a method on MojoHandle, isHandle is "instanceof
> MojoHandle".
>

I agree with these.

We will however need somewhere to put other API calls, e.g.
createMessagePipe which creates two new MojoHandles.


>
>
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/Source/core/mojo/Mojo.idl
> File third_party/WebKit/Source/core/mojo/Mojo.idl (right):
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/Source/core/mojo/Mojo.idl#newcode9
> third_party/WebKit/Source/core/mojo/Mojo.idl:9: NoInterfaceObject,
> remove NoInterfaceObject on this and the other interfaces
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/Source/core/mojo/Mojo.idl#newcode36
> third_party/WebKit/Source/core/mojo/Mojo.idl:36: boolean isHandle(any
> value);
> Why not make handles be a real type and code can do instanceof
> MojoHandle instead? Having an isFoo() method is unusual.
>
> Your patch even has a MojoHandle type.
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/public/web/WebMojoBindings.h
> File third_party/WebKit/public/web/WebMojoBindings.h (right):
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/public/web/WebMojoBindings.h#newcode19
> third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static
> v8::Local<v8::Value> createInterfaceProvider(
> Instead of providing a new API, I think you just want an API on blink
> that takes an isolated world id and blink inserts the API for you.
>
> https://codereview.chromium.org/2400563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Ken Rockot(use gerrit already)

On Mon, Oct 10, 2016 at 1:59 PM, <esprehn@chromium.org> wrote: > I think we want ...

4 years, 2 months ago (2016-10-10 21:05:30 UTC) #16

On Mon, Oct 10, 2016 at 1:59 PM, <esprehn@chromium.org> wrote:

> I think we want this simpler, just add new interfaces:
>
> MojoHandle
> MojoInterfaceProvider
>
> close() should be a method on MojoHandle, isHandle is "instanceof
> MojoHandle".
>

I agree with these.

We will however need somewhere to put other API calls, e.g.
createMessagePipe which creates two new MojoHandles.


>
>
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/Source/core/mojo/Mojo.idl
> File third_party/WebKit/Source/core/mojo/Mojo.idl (right):
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/Source/core/mojo/Mojo.idl#newcode9
> third_party/WebKit/Source/core/mojo/Mojo.idl:9: NoInterfaceObject,
> remove NoInterfaceObject on this and the other interfaces
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/Source/core/mojo/Mojo.idl#newcode36
> third_party/WebKit/Source/core/mojo/Mojo.idl:36: boolean isHandle(any
> value);
> Why not make handles be a real type and code can do instanceof
> MojoHandle instead? Having an isFoo() method is unusual.
>
> Your patch even has a MojoHandle type.
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/public/web/WebMojoBindings.h
> File third_party/WebKit/public/web/WebMojoBindings.h (right):
>
> https://codereview.chromium.org/2400563002/diff/80001/
> third_party/WebKit/public/web/WebMojoBindings.h#newcode19
> third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static
> v8::Local<v8::Value> createInterfaceProvider(
> Instead of providing a new API, I think you just want an API on blink
> that takes an isolated world id and blink inserts the API for you.
>
> https://codereview.chromium.org/2400563002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

alokp

https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/public/web/WebMojoBindings.h File third_party/WebKit/public/web/WebMojoBindings.h (right): https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/public/web/WebMojoBindings.h#newcode19 third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static v8::Local<v8::Value> createInterfaceProvider( On 2016/10/10 20:59:46, esprehn wrote: ...

4 years, 2 months ago (2016-10-10 23:48:17 UTC) #17

esprehn

https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/public/web/WebMojoBindings.h File third_party/WebKit/public/web/WebMojoBindings.h (right): https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/public/web/WebMojoBindings.h#newcode19 third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static v8::Local<v8::Value> createInterfaceProvider( On 2016/10/10 at 23:48:17, alokp ...

4 years, 2 months ago (2016-10-10 23:54:46 UTC) #18

esprehn

https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/public/web/WebMojoBindings.h File third_party/WebKit/public/web/WebMojoBindings.h (right): https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/public/web/WebMojoBindings.h#newcode19 third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static v8::Local<v8::Value> createInterfaceProvider( Err you can take a ...

4 years, 2 months ago (2016-10-11 00:12:38 UTC) #19

alokp

I have addressed comments and reverted all changes outside blink to make it easier to ...

4 years, 2 months ago (2016-10-18 00:11:47 UTC) #20

I have addressed comments and reverted all changes outside blink to make it
easier to review. PTAL.

https://codereview.chromium.org/2400563002/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2400563002/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:2555: registry->AddBuiltinModule(
On 2016/10/10 17:11:25, Ken Rockot wrote:
> I don't think this code should change yet. We should continue to expose the
old
> mojo/public/js/core module as it exists today, until the entire core API is
> ported. I also don't think you want to use |registry| to expose your new
> bindings object.
> 
> I'd rather we just stash the new core in a global object like window.mojoCore
or
> something similar (preferably just 'mojo' but that's already taken in layout
> tests at the moment - can fix once the rest of the API is ported.)
> 
> Then JS contexts which are allowed to see mojo, like layout tests and your
cast
> client code, don't need to do any requires/defines/whatever to get the core
API.
> They can call mojoCore.createMessagePipe etc immediately.

Agreed. I have reverted the changes in this file for now.

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

https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/Mojo.idl:9: NoInterfaceObject,
On 2016/10/10 20:59:46, esprehn wrote:
> remove NoInterfaceObject on this and the other interfaces

Done.

https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/mojo/Mojo.idl:36: boolean isHandle(any value);
On 2016/10/10 20:59:46, esprehn wrote:
> Why not make handles be a real type and code can do instanceof MojoHandle
> instead? Having an isFoo() method is unusual.
> 
> Your patch even has a MojoHandle type.

Done.

https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebMojoBindings.h (right):

https://codereview.chromium.org/2400563002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static
v8::Local<v8::Value> createInterfaceProvider(
On 2016/10/11 00:12:38, esprehn wrote:
> Err you can take a v8::Context directly instead too, and handle the isolated
> world stuff out in content for now, which is what injectInternalsObject does.

Added a TODO to do this in a follow-up patch. Wrapping the core binding into a
gin module is the least disruptive way to transition.

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebMojoBindings.h (right):

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static
v8::Local<v8::Value> create(v8::Local<v8::Context>);
I have retained this API for now to make is easy to transition from the bindings
implemented in mojo/edk/js. Once the transition is complete, I will implement
the solution proposed by esprehn@.

Ken Rockot(use gerrit already)

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode47 third_party/WebKit/Source/core/mojo/Mojo.idl:47: MojoResult writeMessage(MojoMessagePipeHandle pipe, ArrayBuffer buffer, sequence<MojoHandle> handles, MojoWriteMessageFlags flags); ...

4 years, 2 months ago (2016-10-18 15:47:10 UTC) #21

alokp

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode47 third_party/WebKit/Source/core/mojo/Mojo.idl:47: MojoResult writeMessage(MojoMessagePipeHandle pipe, ArrayBuffer buffer, sequence<MojoHandle> handles, MojoWriteMessageFlags flags); ...

4 years, 2 months ago (2016-10-18 16:48:37 UTC) #22

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:47: MojoResult
writeMessage(MojoMessagePipeHandle pipe, ArrayBuffer buffer,
sequence<MojoHandle> handles, MojoWriteMessageFlags flags);
On 2016/10/18 15:47:10, Ken Rockot wrote:
> APIs that take ArrayBuffer are sad because then you can only use ArrayBuffer
> with them. I believe you want BufferSource instead, as this can be an
> ArrayBuffer, or a typed array, or a data view type, etc.

I used ArrayBuffer to keep writeMessage consistent with readMessage. I can use
BufferSource if that is preferred.

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:48: MojoReadMessageResult
readMessage(MojoMessagePipeHandle pipe, MojoReadMessageFlags flags);
On 2016/10/18 15:47:10, Ken Rockot wrote:
> Note that we also have new read/write message APIs, and these old ones should
be
> considered deprecated. The new APIs deal with a Message object type, and
message
> storage is allocated by the Mojo system on your behalf. This eliminates
> otherwise unavoidable redundant copies when using message pipes.
> 
> I think it's fine to only achieve parity with the existing JS bindings for
now,
> but it is probably worth supporting the new APIs as well before switching any
> code to use the new core interfaces.

Acknowledged.

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandleWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandleWatcher.cpp:63: MojoHandleWatcher*
watcher = reinterpret_cast<MojoHandleWatcher*>(context);
On 2016/10/18 15:47:10, Ken Rockot wrote:
> Please explicitly document here that |watcher| is guaranteed to remain alive
> through the extent of onHandleReady.

Done.

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandleWatcher.h (right):

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandleWatcher.h:35: WebTaskRunner*
m_taskRunner;
On 2016/10/18 15:47:10, Ken Rockot wrote:
> nit: WebTaskRunner* const to guarantee m_taskRunner never changes. Otherwise
> onHandleReady's thread-safety is unclear

Done.

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoScopedHandle.h (right):

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoScopedHandle.h:12: class
MojoScopedHandle {
On 2016/10/18 15:47:10, Ken Rockot wrote:
> Why can't you use mojo::ScopedHandle in mojo/public/cpp/system/handle.h? It
> seems you're just partially duplicating that.

It would add a dependency on mojo/public/cpp, which I have tried to avoid since
it is my understanding that we cannot use base/ here? Without base/ we cannot
use mojo::Watcher for example. IMHO the things that are duplicated here do not
warrant adding the dependency on cpp.

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebMojoBindings.h (right):

https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static
v8::Local<v8::Value> create(v8::Local<v8::Context>);
On 2016/10/18 15:47:10, Ken Rockot wrote:
> On 2016/10/18 at 00:11:47, alokp wrote:
> > I have retained this API for now to make is easy to transition from the
> bindings implemented in mojo/edk/js. Once the transition is complete, I will
> implement the solution proposed by esprehn@.
> 
> I don't really understand the TODO. What do gin or ES6 modules have to do with
> the core API? We can synchronously inject the Mojo interface now without
> interfering with any existing code.

We can inject it now but transitioning the existing code (especially
mojo/public/js) would require quite a few changes. It would be much easier to
register the object returned by "create" as a builtin module, exactly like
mojo/edk/js/core is registered. Once we have deprecated the edk module, we can
inject the core API object and update all consumers.

It also makes it easy for me to test this implementation against existing
clients without changing too much code:
https://codereview.chromium.org/2405093003/

Ken Rockot(use gerrit already)

On 2016/10/18 at 16:48:37, alokp wrote: > https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Source/core/mojo/Mojo.idl > File third_party/WebKit/Source/core/mojo/Mojo.idl (right): > > https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode47 ...

4 years, 2 months ago (2016-10-18 16:59:57 UTC) #23

On 2016/10/18 at 16:48:37, alokp wrote:
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/mojo/Mojo.idl (right):
> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/mojo/Mojo.idl:47: MojoResult
writeMessage(MojoMessagePipeHandle pipe, ArrayBuffer buffer,
sequence<MojoHandle> handles, MojoWriteMessageFlags flags);
> On 2016/10/18 15:47:10, Ken Rockot wrote:
> > APIs that take ArrayBuffer are sad because then you can only use ArrayBuffer
> > with them. I believe you want BufferSource instead, as this can be an
> > ArrayBuffer, or a typed array, or a data view type, etc.
> 
> I used ArrayBuffer to keep writeMessage consistent with readMessage. I can use
BufferSource if that is preferred.

Actually, I think you want a BufferSource for write and a DataView for read.
This would maximize the usability of both APIs.

> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/mojo/Mojo.idl:48: MojoReadMessageResult
readMessage(MojoMessagePipeHandle pipe, MojoReadMessageFlags flags);
> On 2016/10/18 15:47:10, Ken Rockot wrote:
> > Note that we also have new read/write message APIs, and these old ones
should be
> > considered deprecated. The new APIs deal with a Message object type, and
message
> > storage is allocated by the Mojo system on your behalf. This eliminates
> > otherwise unavoidable redundant copies when using message pipes.
> > 
> > I think it's fine to only achieve parity with the existing JS bindings for
now,
> > but it is probably worth supporting the new APIs as well before switching
any
> > code to use the new core interfaces.
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/mojo/MojoHandleWatcher.cpp (right):
> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/mojo/MojoHandleWatcher.cpp:63:
MojoHandleWatcher* watcher = reinterpret_cast<MojoHandleWatcher*>(context);
> On 2016/10/18 15:47:10, Ken Rockot wrote:
> > Please explicitly document here that |watcher| is guaranteed to remain alive
> > through the extent of onHandleReady.
> 
> Done.
> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/mojo/MojoHandleWatcher.h (right):
> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/mojo/MojoHandleWatcher.h:35: WebTaskRunner*
m_taskRunner;
> On 2016/10/18 15:47:10, Ken Rockot wrote:
> > nit: WebTaskRunner* const to guarantee m_taskRunner never changes. Otherwise
> > onHandleReady's thread-safety is unclear
> 
> Done.
> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/mojo/MojoScopedHandle.h (right):
> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/mojo/MojoScopedHandle.h:12: class
MojoScopedHandle {
> On 2016/10/18 15:47:10, Ken Rockot wrote:
> > Why can't you use mojo::ScopedHandle in mojo/public/cpp/system/handle.h? It
> > seems you're just partially duplicating that.
> 
> It would add a dependency on mojo/public/cpp, which I have tried to avoid
since it is my understanding that we cannot use base/ here? Without base/ we
cannot use mojo::Watcher for example. IMHO the things that are duplicated here
do not warrant adding the dependency on cpp.

Watcher makes sense because it needs to use a different kind of task runner.
There are not blanket restrictions against using all of base, and
mojo/public/cpp/system/handle.h specifically has no interesting base
dependencies (everything is allowed). Other things in mojo/public/cpp/system do,
but I really don't want two different types of scoped handles and I don't think
this is a good enough reason to duplicate it.

> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebMojoBindings.h (right):
> 
>
https://codereview.chromium.org/2400563002/diff/100001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebMojoBindings.h:19: BLINK_EXPORT static
v8::Local<v8::Value> create(v8::Local<v8::Context>);
> On 2016/10/18 15:47:10, Ken Rockot wrote:
> > On 2016/10/18 at 00:11:47, alokp wrote:
> > > I have retained this API for now to make is easy to transition from the
> > bindings implemented in mojo/edk/js. Once the transition is complete, I will
> > implement the solution proposed by esprehn@.
> > 
> > I don't really understand the TODO. What do gin or ES6 modules have to do
with
> > the core API? We can synchronously inject the Mojo interface now without
> > interfering with any existing code.
> 
> We can inject it now but transitioning the existing code (especially
mojo/public/js) would require quite a few changes. It would be much easier to
register the object returned by "create" as a builtin module, exactly like
mojo/edk/js/core is registered. Once we have deprecated the edk module, we can
inject the core API object and update all consumers.
> 
> It also makes it easy for me to test this implementation against existing
clients without changing too much code:
https://codereview.chromium.org/2405093003/

OK. Makes sense

alokp

On 2016/10/24 21:27:28, esprehn wrote: > Is this ready for another review? I just uploaded ...

4 years, 1 month ago (2016-10-24 22:30:00 UTC) #25

esprehn

Bunch of questions, pardon my mojo ignorance, just trying to understand it. :) https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Source/core/mojo/Mojo.cpp File ...

4 years, 1 month ago (2016-10-25 03:34:07 UTC) #26

alokp

Addressed a few comments. A bunch of blink/idl questions for you. https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Source/core/mojo/Mojo.cpp File third_party/WebKit/Source/core/mojo/Mojo.cpp (right): ...

4 years, 1 month ago (2016-10-25 05:01:38 UTC) #27

Addressed a few comments. A bunch of blink/idl questions for you.

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.cpp (right):

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.cpp:52: std::vector<::MojoHandle>
rawHandles(handles.size());
On 2016/10/25 03:34:06, esprehn wrote:
> Use Vector, I'd also suggest using inline capacity here since you can totally
> avoid the heap allocation.
> 
> Vector<::MojoHandle, 32> rawHandles(handles.size());
> 
> or something, how many handles is typical?

Used WTF::Vector. The number of handle really depends on the interface, but most
messages do not have any handle. Do you still recommend using a non-zero inline
capacity?

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.cpp:53: for (size_t i = 0; i <
handles.size(); ++i)
On 2016/10/25 03:34:06, esprehn wrote:
> for (auto& handle : handles)
>   rawHandles.append(handle.release().value))
> 
> ?

Done.

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.cpp:86: std::vector<::MojoHandle>
rawHandles(numHandles);
On 2016/10/25 03:34:06, esprehn wrote:
> Vector<> again, you probably also want inline capacity since this vector is
> wasted below when we convert to a HeapVector for making the objects.

Used Vector. Since most messages would not have any handle, wouldn't using a
non-zero inline-capacity be more expensive on average?

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:45: MojoCreateMessagePipeResult
createMessagePipe(MojoCreateMessagePipeOptions options);
On 2016/10/25 03:34:06, esprehn wrote:
> Lets make MojoCreateMessagePipeResult a real object

Sorry for my ignorance. By real object, do you mean an interface with read-only
attributes? Is that more efficient, since I do not need any member function?

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoCreateMessagePipeResult.idl
(right):

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoCreateMessagePipeResult.idl:5:
dictionary MojoCreateMessagePipeResult {
On 2016/10/25 03:34:06, esprehn wrote:
> Can we make this a real interface?

Same question as before. I do not need any member function. Is an interface more
efficient than a dictionary?

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoReadMessageResult.idl (right):

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoReadMessageResult.idl:5: dictionary
MojoReadMessageResult {
On 2016/10/25 03:34:06, esprehn wrote:
> ditto

ditto :)

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:25:
CrossThreadWeakPersistent<MojoWatcher> m_watcher;
On 2016/10/25 03:34:06, esprehn wrote:
> Can we just WTF::Bind() and createBaseCallback() instead?

Sure. But since you recommend using ExecutionContext TaskRunner, I would need to
create ExecutionContextTask using createCrossThreadTask?

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:35:
Platform::current()->currentThread()->getWebTaskRunner()->clone());
On 2016/10/25 03:34:06, esprehn wrote:
> I think you want to use the TaskRunner from the ExecutionContext instead? This
> bypasses the scheduler.

Yeah that should work. Is it safe to call ScriptState::getExecutionContext from
non-main thread? I would need to call it from MojoWatcher::onHandleReady, which
runs on IO thread.

Or do you recommend storing a pointer to ExecutionContext in MojoWatcher?

A related question: Do I need to observe life cycle of ExecutionContext to quit
calling the watch callback?

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:88: if
(watcher->m_taskRunner->runsTasksOnCurrentThread()) {
On 2016/10/25 03:34:06, esprehn wrote:
> Hmm, did it do this in the old bindings?

Yes - the old bindings use mojo::Watcher, which does this:
https://cs.chromium.org/chromium/src/mojo/public/cpp/system/watcher.cc?q=mojo...

That said, it is not really necessary here since this function is always called
on the IO thread. If you prefer, I can get rid of this if-block.

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.h (right):

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.h:33: friend class
V8MojoWatcher;
On 2016/10/25 03:34:06, esprehn wrote:
> Why friends?

V8MojoWatcher accesses m_callback to call setWrapperReference. Please see
V8MojoWatcherCustom.cpp.

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.h:40: static void
onHandleReady(uintptr_t,
On 2016/10/25 03:34:07, esprehn wrote:
> add argument names for primitive types

Done.

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.h:46: mojo::Handle m_handle;
On 2016/10/25 03:34:06, esprehn wrote:
> Hmm, you use mojo::ScopedHandle in Handle, but just mojo::Handle here, why? 

Because blink::MojoHandle owns mojo::Handle (closes the handle in destructor).
blink::MojoWatcher just watches the mojo::Handle for certain events.

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.idl (right):

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.idl:8: MojoResult cancel();
On 2016/10/25 03:34:07, esprehn wrote:
> This could also have a MojoHandle property?

Sure - if needed. The old bindings do not have this property.

Related question: It seems a more idiomatic way is to use "long" instead of an
object (similar to setInterval and clearInterval)? Would you recommend the watch
API like:
long watch(MojoHandle, MojoHandleSignals, MojoWatchCallback);
void cancelWatch(long);

Or the current one is fine? The reason I made MojoWatcher an object so as to
cancel the watch when MojoWatcher is garbage collected. But I am not sure why
most other watch APIs that I know of are implemented using an integral ID.

alokp

The latest patch uses ExecutionContext TaskRunner, which fixed the hang issue and also made a ...

4 years, 1 month ago (2016-10-25 17:18:18 UTC) #28

Ken Rockot(use gerrit already)

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Source/core/mojo/Mojo.cpp File third_party/WebKit/Source/core/mojo/Mojo.cpp (right): https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Source/core/mojo/Mojo.cpp#newcode52 third_party/WebKit/Source/core/mojo/Mojo.cpp:52: std::vector<::MojoHandle> rawHandles(handles.size()); On 2016/10/25 at 05:01:37, alokp wrote: > ...

4 years, 1 month ago (2016-10-25 18:11:11 UTC) #29

alokp

https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Source/core/mojo/Mojo.cpp File third_party/WebKit/Source/core/mojo/Mojo.cpp (right): https://codereview.chromium.org/2400563002/diff/200001/third_party/WebKit/Source/core/mojo/Mojo.cpp#newcode52 third_party/WebKit/Source/core/mojo/Mojo.cpp:52: std::vector<::MojoHandle> rawHandles(handles.size()); On 2016/10/25 18:11:11, Ken Rockot wrote: > ...

4 years, 1 month ago (2016-10-25 20:33:36 UTC) #30

alokp

If this looks reasonable I will start adding layout tests. Please let me know which ...

4 years, 1 month ago (2016-10-26 05:24:09 UTC) #31

alokp

On 2016/10/26 05:24:09, alokp wrote: > If this looks reasonable I will start adding layout ...

4 years, 1 month ago (2016-10-28 17:37:45 UTC) #32

esprehn

esprehn@chromium.org changed reviewers: + jbroman@chromium.org, reillyg@chromium.org

4 years ago (2016-12-20 03:10:05 UTC) #33

esprehn

jbroman@ and reillyg@ Could you look this over? I'm a bit overloaded and perhaps you ...

4 years ago (2016-12-20 03:10:05 UTC) #34

jbroman

Two high-level questions: - Is there a design doc for this? The two sentences in ...

4 years ago (2016-12-20 15:45:05 UTC) #35

alokp

Chromecast team wants to use mojo-js bindings in production code (I understand that they are ...

4 years ago (2016-12-20 18:37:35 UTC) #36

Reilly Grant (use Gerrit)

This change looks reasonable to me but I am probably not the best reviewer for ...

4 years ago (2016-12-20 20:37:32 UTC) #37

jbroman

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/BUILD.gn File third_party/WebKit/Source/core/mojo/BUILD.gn (right): https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/BUILD.gn#newcode1 third_party/WebKit/Source/core/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. ...

4 years ago (2016-12-20 23:21:58 UTC) #38

jbroman

A couple other API-ish questions, now that I've thought about the presence of MojoHandle::close. https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl ...

4 years ago (2016-12-21 18:09:48 UTC) #39

esprehn

+1 to making the API look like an idomatic JS api as much as possible ...

4 years ago (2016-12-21 21:32:44 UTC) #40

alokp

On 2016/12/21 21:32:44, esprehn wrote: > +1 to making the API look like an idomatic ...

4 years ago (2016-12-22 00:36:59 UTC) #41

alokp

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/BUILD.gn File third_party/WebKit/Source/core/mojo/BUILD.gn (right): https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/BUILD.gn#newcode1 third_party/WebKit/Source/core/mojo/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. ...

3 years, 11 months ago (2017-01-09 23:33:10 UTC) #42

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/BUILD.gn (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/BUILD.gn:1: # Copyright 2016 The Chromium
Authors. All rights reserved.
On 2016/12/20 23:21:57, jbroman wrote:
> Would you mind also having a README.md that explains what this directory is?
In
> particular, I'm concerned people might be confused and think it's the general
> way to use Mojo from Blink. It'd be nice to clear up that it's just the native
> part of JS bindings to Mojo.

Done.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.cpp (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.cpp:59:
rawHandles.append(handle->release().value());
On 2016/12/20 23:21:57, jbroman wrote:
> I don't think this does what you want. This creates a vector with
> |handles.size()| zero-initialized handles followed by |handles.size()|
released
> from |handles|. I think you want to do something like below: either using a
for
> loop over the indices, or (my personal preference) std::transform from
> <algorithm>.

good catch - done.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.cpp:68:
DCHECK(buffer.isArrayBufferView());
On 2016/12/20 23:21:57, jbroman wrote:
> For what it's worth, this is already DCHECKed inside getAsArrayBufferView.

Removed redundant DCHECK

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.cpp:75: rawHandles.size(), flags);
On 2016/12/20 23:21:57, jbroman wrote:
> Here and elsewhere: this exposes access to pass arbitrary parameters to the
Mojo
> EDK. I am assuming this is an interface that will only be used in contexts
> running only trusted script? If not, has security team signed off on allowing
> whatever scripts this interface is available in to send arbitrary IPC messages
> with arbitrary flags?

This will only be used for trusted script. I would still be curious to know
about the security implications. IIUC the service side closes the pipe upon
receiving invalid messages.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.h (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.h:80: ArrayBufferOrArrayBufferView&,
On 2016/12/20 23:21:57, jbroman wrote:
> const ArrayBufferOrArrayBufferView&

Done.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo {
On 2016/12/20 23:21:57, jbroman wrote:
> Should all of these interfaces have [NoInterfaceObject] to avoid their
> interfaces being installed on the global object? If I understand correctly,
they
> will get access to it by being given an instance of Mojo (and can reach
> constants from there).

We do need to install MojoHandle interface so that we can do "handle instanceOf
MojoHandle". I agree that this particular interface can have NoInterfaceObject,
but we may still want to install it for consistency and not have to pass the
instance around just to access the constants?

Anyways, I did use NoInterfaceObject in the initial versions of the patch.
esprehn@ asked me to remove those. Elliot?

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:43: [CallWith=ScriptState]
MojoWatcher watch(MojoHandle handle, MojoHandleSignals signals,
MojoWatchCallback callback);
On 2016/12/21 18:09:48, jbroman wrote:
> Most of these look like they could be interface methods on MojoHandle. Is it
an
> intentional choice not to do that?
> 
> handle.watch(signals, function() { ... });
> 
> seems a little more idiomatic than
> 
> mojo.watch(handle, signals, function() { ... });
> 
> Also, how stable will we be making this API? If it needs to be stable because
it
> interacts with code not shipped with Chromium, we might want to consider some
of
> the usual web API lessons. For example, might this (or other) functions need
> more arguments in the future? If so, we might want to have a dictionary
argument
> right now.
> 
> If that's the case, then this will also lock in the MojoResult constants etc.

Good idea. Moved watch, read/writeMessage to MojoHandle. Also created dictionary
arguments.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:46: MojoResult
writeMessage(MojoHandle pipe, BufferSource buffer, sequence<MojoHandle> handles,
MojoWriteMessageFlags flags);
On 2016/12/21 18:09:48, jbroman wrote:
> We can probably reduce type confusion by making a derived interface for
> MojoMessagePipeHandle (like the C++ library does), and having the method take
> that instead of MojoHandle. If there are cases where we'll have some
MojoHandle
> of unknown type and need to expose it to script, then maybe it's not worth the
> hassle.

Type of MojoHandle is not available when reading a message at the core layer
(Note that MojoReadMessage only provides an array of raw handles). Type becomes
available only after message deserialization at which point C++ library simply
wraps the raw handle into a MojoMessagePipeHandle.

With my limited JS foo, I could not figure out how to convert a JS MojoHandle
type into MojoMessagePipeHandle. I agree that it would be much nicer if we could
do this.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandle.h (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandle.h:19: virtual ~MojoHandle();
On 2016/12/20 23:21:57, jbroman wrote:
> nit: Why the virtual destructor? This is a final class.

Done.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:69:
watcher->m_taskRunner->postTask(
On 2016/12/20 23:21:57, jbroman wrote:
> Even though this reference to ExecutionContext is thread-safe,
ExecutionContext
> itself is not.

Hmm I see multiple other instances of cross-thread postTask. For example:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Messa...

> In addition, though watcher is guaranteed to still exist, the ExecutionContext
> is not (at least, not obviously), since it does not hold a strong reference. I
> think you want to hold std::unique_ptr<WebTaskRunner> instead, which should be
> thread-safe.

I used WebTaskRunner in earlier versions of this patch, but I ran into an issue
where layout tests using mojo did not terminate. MojoWatcher would continue to
fire ready callbacks. Elliot suggested using the task runner from
ExecutionContext because WebTaskRunner bypasses the scheduler. Making the change
fixed those layout tests as well. Is there a way to extract a thread-safe task
runner from ExecutionContext?

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:82:
m_callback->call(m_scriptState, this, result);
On 2016/12/20 23:21:57, jbroman wrote:
> I don't think m_scriptState is needed for this (and thus, anything in this
> class) anymore.

Done.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.h (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.h:44: Member<MojoWatchCallback>
m_callback;
On 2016/12/20 23:21:57, jbroman wrote:
> This should be a TraceWrapperMember, because if TraceWrappables is enabled (it
> was just turned on at tip of tree), you'll need to use that instead of the
> deprecated visitDOMWrapper.
> 
> See
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

Done.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:23: [CallWith=ScriptState]
MojoWatcher watch(MojoHandleSignals signals, MojoWatchCallback callback);
Should this return a MojoWatch or an integer that can be cancelled using
MojoHandle::cancelWatch? The latter seems to be more idiomatic.

alokp

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

3 years, 11 months ago (2017-01-10 00:34:49 UTC) #43

jbroman

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.cpp File third_party/WebKit/Source/core/mojo/Mojo.cpp (right): https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.cpp#newcode75 third_party/WebKit/Source/core/mojo/Mojo.cpp:75: rawHandles.size(), flags); On 2017/01/09 at 23:33:09, alokp wrote: > ...

3 years, 11 months ago (2017-01-10 20:46:31 UTC) #44

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.cpp (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.cpp:75: rawHandles.size(), flags);
On 2017/01/09 at 23:33:09, alokp wrote:
> On 2016/12/20 23:21:57, jbroman wrote:
> > Here and elsewhere: this exposes access to pass arbitrary parameters to the
Mojo
> > EDK. I am assuming this is an interface that will only be used in contexts
> > running only trusted script? If not, has security team signed off on
allowing
> > whatever scripts this interface is available in to send arbitrary IPC
messages
> > with arbitrary flags?
> 
> This will only be used for trusted script. I would still be curious to know
about the security implications. IIUC the service side closes the pipe upon
receiving invalid messages.

I'm not a security engineer, but yes, I think that will protect the service
side. Escaping from JavaScript to compromise the renderer would still be bad.
I'm concerned about whether these core Mojo methods could corrupt state inside
the process if given invalid arguments, and whether that might possible allow
the script to compromise the renderer. A compromised renderer is still fairly
powerful (e.g. it may hold some cross-origin cookies).

Generally our C++ API methods don't assume an adversarial caller, but calls from
JS script contexts (notably those that may have untrusted script running in
them) must be treated with greater suspicion.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo {
On 2017/01/09 at 23:33:09, alokp wrote:
> On 2016/12/20 23:21:57, jbroman wrote:
> > Should all of these interfaces have [NoInterfaceObject] to avoid their
> > interfaces being installed on the global object? If I understand correctly,
they
> > will get access to it by being given an instance of Mojo (and can reach
> > constants from there).
> 
> We do need to install MojoHandle interface so that we can do "handle
instanceOf MojoHandle". I agree that this particular interface can have
NoInterfaceObject, but we may still want to install it for consistency and not
have to pass the instance around just to access the constants?
> 
> Anyways, I did use NoInterfaceObject in the initial versions of the patch.
esprehn@ asked me to remove those. Elliot?

1. My main concern here is that I do not want all Blink script contexts to have
the Mojo family of objects exposed, which is what will happen at present (you
probably have a test failure in
virtual/stable/webexposed/global-interface-listing.html at present).
2. My secondary concern is that in those contexts where we do intend to use it,
it's not clear to me whether we want the Mojo stuff visible on the global (lest
cast receiver apps be tempted to issue their own Mojo calls).

Possibly we could address (1) by applying the infrastructure built for origin
trials to conditionally expose this.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:46: MojoResult
writeMessage(MojoHandle pipe, BufferSource buffer, sequence<MojoHandle> handles,
MojoWriteMessageFlags flags);
On 2017/01/09 at 23:33:09, alokp wrote:
> On 2016/12/21 18:09:48, jbroman wrote:
> > We can probably reduce type confusion by making a derived interface for
> > MojoMessagePipeHandle (like the C++ library does), and having the method
take
> > that instead of MojoHandle. If there are cases where we'll have some
MojoHandle
> > of unknown type and need to expose it to script, then maybe it's not worth
the
> > hassle.
> 
> Type of MojoHandle is not available when reading a message at the core layer
(Note that MojoReadMessage only provides an array of raw handles). Type becomes
available only after message deserialization at which point C++ library simply
wraps the raw handle into a MojoMessagePipeHandle.
> 
> With my limited JS foo, I could not figure out how to convert a JS MojoHandle
type into MojoMessagePipeHandle. I agree that it would be much nicer if we could
do this.

interface MojoMessagePipeHandle : MojoHandle {};

Ideally you'd be able to do things like:
var someHandle = result.handles[0];
console.log(someHandle instanceof MojoMessagePipeHandle); // true

But if we can't tell when we wrap a handle what the right wrapper type for it is
(e.g. when handles are received from a pipe). Blink objects normally know what
their type is when they create their wrappers, but if that's not the case here
then we'd need to add the equivalent of an unsafe cast, which IMHO is worse than
just having it validated after being passed to an API call.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:69:
watcher->m_taskRunner->postTask(
On 2017/01/09 at 23:33:10, alokp wrote:
> On 2016/12/20 23:21:57, jbroman wrote:
> > Even though this reference to ExecutionContext is thread-safe,
ExecutionContext
> > itself is not.
> 
> Hmm I see multiple other instances of cross-thread postTask. For example:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Messa...

I believe that is probably safe, but for subtle reasons, because of the use of a
lock on the Chromium side:

- If the MessagePort or its context are destroyed, MessagePort::close will call
WebMessagePortChannelImpl::setClient (with nullptr) which acquires its lock.
- MessagePort::messageAvailable is only called off-thread by
WebMessagePortChannelImpl::OnMessage, which holds the same lock.
- It happens that there don't seem to be any other methods that mutate that
state on the main thread at present.

Thus it is sequenced so that the destruction of the execution context and the
processing of the message do not occur simultaneously.

If you do (as I think you should in any event, see below) stop the watch when
the context is destroyed (using the lock that I believe Mojo has internally),
then this might be safe. But I think it's unnecessarily precarious, and
ExecutionContext does not make a general promise that ExecutionContext::postTask
doesn't mutate any other main thread state. WebTaskRunner does have that
contract.

(esprehn may disagree with me; I haven't discussed this with him.)

> > In addition, though watcher is guaranteed to still exist, the
ExecutionContext
> > is not (at least, not obviously), since it does not hold a strong reference.
I
> > think you want to hold std::unique_ptr<WebTaskRunner> instead, which should
be
> > thread-safe.
> 
> I used WebTaskRunner in earlier versions of this patch, but I ran into an
issue where layout tests using mojo did not terminate. MojoWatcher would
continue to fire ready callbacks. Elliot suggested using the task runner from
ExecutionContext because WebTaskRunner bypasses the scheduler. Making the change
fixed those layout tests as well. Is there a way to extract a thread-safe task
runner from ExecutionContext?

This object should clean itself up when the context is destroyed, by subclassing
ContextLifecycleObserver (unless it's changed due to haraken's recent changes).
At that point it can cancel itself, which should deal with this.

I'm not sure what was meant by "bypassing the scheduler". There may be task
runners that do, but if you get one of the categorized task runners from
TaskRunnerHelper, those tasks should be managed by the scheduler.

Something like this should work, I believe:

// on the main thread, when the MojoWatcher is created
std::unique_ptr<WebTaskRunner> taskRunner = TaskRunnerHelper::get(TaskType::Mojo
/* or one of the existing ones */, scriptState->getExecutionContext())->clone();

This will create a WebTaskRunner which this object owns, but which shares the
same (thread-safe) task queue as the one owned from the main thread.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoCreateMessagePipeOptions.idl
(right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoCreateMessagePipeOptions.idl:6:
MojoCreateMessagePipeOptionsFlags flags;
The webby way would be for the particular flags to be booleans in the
dictionary. As there currently are none, this can go away (and in fact
MojoCreateMessagePipeOptions can go away entirely, as it could be added as
"optional MojoCreateMessagePipeOptions" in the future if any options come into
existence, without breaking compatibility).

That is, callers today could look like:

var { result, handle0, handle1 } = mojo.createMessagePipe();

and in the future, that might become: mojo.createMessagePipe({ extraWidePipes:
true }) or whatever

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:31: return
mojo::CloseRaw(m_handle.release());
Can this fail (in a case where the result will be useful on the JS side)?

Most places seem to DCHECK that this is MOJO_RESULT_OK. In that case, maybe this
would work:

void MojoHandle::close() {
  m_handle.reset();
}

Otherwise, maybe a comment explaining this would be helpful.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:23: [CallWith=ScriptState]
MojoWatcher watch(MojoHandleSignals signals, MojoWatchCallback callback);
On 2017/01/09 at 23:33:10, alokp wrote:
> Should this return a MojoWatch or an integer that can be cancelled using
MojoHandle::cancelWatch? The latter seems to be more idiomatic.

This could be done with a numbered identifier like setTimeout has (as you
mention), but personally I actually prefer objects for this, rather than having
to maintain a context-wide map.

JavaScript-land would also typically use a dictionary instead of flags for
signals, like MutationObserverInit, maybe:

dictionary MojoWatcherInit {
  optional boolean readable = false;
  optional boolean writable = false;
  optional boolean peerClosed = false;
};

with uses looking like:
handle.watch({ readable: true, writable: true }, result => { ... });

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoReadMessageOptions.idl (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoReadMessageOptions.idl:6:
MojoReadMessageFlags flags;
In JavaScript-land, it would be more idiomatic to have the dictionary be an
optional argument (our bindings will take care of constructing a default
dictionary in this case), and have this have specific fields that correspond to
the flags:

boolean mayDiscard = false;

so that use looks like:

var result = handle.readMessage();
var resultThatMayDiscard = handle.readMessage({ mayDiscard: false });

Doing so reduces the need for numeric constants.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:41: cancel();
Hold on, what stops this from being cancelled the next time GC happens? It's
unusual in JS to expect visible effects from finalization of objects (you're
usually permitted to just drop references to things you don't care about).

If I write:

handle.watch(signals, result => {
  // ...
});

and I'm not interested in cancelling the watch ever (well, more likely, until
context shutdown)

I think this object probably needs to be ActiveScriptWrappable so that it's kept
alive while the context is alive and it has pending activity (which is probably
"the watch is not cancelled").

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:60: MojoHandleSignalsState,
Does script not need to know which signals are ready? Is it expected that this
will be added in the future, or is it never needed?

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl:8:
MojoWriteMessageFlags flags;
Similarly, it would be idiomatic to have these flags expanded as separate
booleans (except that there currently are none, so you can just remove it until
there are).

alokp

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.cpp File third_party/WebKit/Source/core/mojo/Mojo.cpp (right): https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.cpp#newcode75 third_party/WebKit/Source/core/mojo/Mojo.cpp:75: rawHandles.size(), flags); On 2017/01/10 20:46:30, jbroman wrote: > On ...

3 years, 11 months ago (2017-01-11 21:53:02 UTC) #45

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.cpp (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.cpp:75: rawHandles.size(), flags);
On 2017/01/10 20:46:30, jbroman wrote:
> On 2017/01/09 at 23:33:09, alokp wrote:
> > On 2016/12/20 23:21:57, jbroman wrote:
> > > Here and elsewhere: this exposes access to pass arbitrary parameters to
the
> Mojo
> > > EDK. I am assuming this is an interface that will only be used in contexts
> > > running only trusted script? If not, has security team signed off on
> allowing
> > > whatever scripts this interface is available in to send arbitrary IPC
> messages
> > > with arbitrary flags?
> > 
> > This will only be used for trusted script. I would still be curious to know
> about the security implications. IIUC the service side closes the pipe upon
> receiving invalid messages.
> 
> I'm not a security engineer, but yes, I think that will protect the service
> side. Escaping from JavaScript to compromise the renderer would still be bad.
> I'm concerned about whether these core Mojo methods could corrupt state inside
> the process if given invalid arguments, and whether that might possible allow
> the script to compromise the renderer. A compromised renderer is still fairly
> powerful (e.g. it may hold some cross-origin cookies).
> 
> Generally our C++ API methods don't assume an adversarial caller, but calls
from
> JS script contexts (notably those that may have untrusted script running in
> them) must be treated with greater suspicion.

Acknowledged. I will add tsepez@ as reviewer once this patch is ready for
security review.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo {
On 2017/01/10 20:46:30, jbroman wrote:
> On 2017/01/09 at 23:33:09, alokp wrote:
> > On 2016/12/20 23:21:57, jbroman wrote:
> > > Should all of these interfaces have [NoInterfaceObject] to avoid their
> > > interfaces being installed on the global object? If I understand
correctly,
> they
> > > will get access to it by being given an instance of Mojo (and can reach
> > > constants from there).
> > 
> > We do need to install MojoHandle interface so that we can do "handle
> instanceOf MojoHandle". I agree that this particular interface can have
> NoInterfaceObject, but we may still want to install it for consistency and not
> have to pass the instance around just to access the constants?
> > 
> > Anyways, I did use NoInterfaceObject in the initial versions of the patch.
> esprehn@ asked me to remove those. Elliot?
> 
> 1. My main concern here is that I do not want all Blink script contexts to
have
> the Mojo family of objects exposed, which is what will happen at present (you
> probably have a test failure in
> virtual/stable/webexposed/global-interface-listing.html at present).

Is this concern about cluttering the context with unnecessary interfaces or
something else? We are not unconditionally injecting a Mojo instance into every
context.

> 2. My secondary concern is that in those contexts where we do intend to use
it,
> it's not clear to me whether we want the Mojo stuff visible on the global
(lest
> cast receiver apps be tempted to issue their own Mojo calls).

Cast Apps will not consume the mojo APIs directly. They will use Cast SDK, which
in turn will internally use Mojo. Pardon my ignorance - I am not sure what you
mean by this comment. Can you elaborate?

> Possibly we could address (1) by applying the infrastructure built for origin
> trials to conditionally expose this.

Can you point me to this infrastructure? I am happy to use it if that is the
recommended approach.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:46: MojoResult
writeMessage(MojoHandle pipe, BufferSource buffer, sequence<MojoHandle> handles,
MojoWriteMessageFlags flags);
On 2017/01/10 20:46:30, jbroman wrote:
> On 2017/01/09 at 23:33:09, alokp wrote:
> > On 2016/12/21 18:09:48, jbroman wrote:
> > > We can probably reduce type confusion by making a derived interface for
> > > MojoMessagePipeHandle (like the C++ library does), and having the method
> take
> > > that instead of MojoHandle. If there are cases where we'll have some
> MojoHandle
> > > of unknown type and need to expose it to script, then maybe it's not worth
> the
> > > hassle.
> > 
> > Type of MojoHandle is not available when reading a message at the core layer
> (Note that MojoReadMessage only provides an array of raw handles). Type
becomes
> available only after message deserialization at which point C++ library simply
> wraps the raw handle into a MojoMessagePipeHandle.
> > 
> > With my limited JS foo, I could not figure out how to convert a JS
MojoHandle
> type into MojoMessagePipeHandle. I agree that it would be much nicer if we
could
> do this.
> 
> interface MojoMessagePipeHandle : MojoHandle {};
> 
> Ideally you'd be able to do things like:
> var someHandle = result.handles[0];
> console.log(someHandle instanceof MojoMessagePipeHandle); // true
> 
> But if we can't tell when we wrap a handle what the right wrapper type for it
is
> (e.g. when handles are received from a pipe). Blink objects normally know what
> their type is when they create their wrappers, but if that's not the case here
> then we'd need to add the equivalent of an unsafe cast, which IMHO is worse
than
> just having it validated after being passed to an API call.

We do not know the type of handle when they are read from a pipe, so I guess we
have to live with a single MojoHandle type. MOJO_RESULT_INVALID_ARGUMENT will be
returned if read/writeMessage is called on the handle of wrong type.

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:69:
watcher->m_taskRunner->postTask(
On 2017/01/10 20:46:30, jbroman wrote:
> On 2017/01/09 at 23:33:10, alokp wrote:
> > On 2016/12/20 23:21:57, jbroman wrote:
> > > Even though this reference to ExecutionContext is thread-safe,
> ExecutionContext
> > > itself is not.
> > 
> > Hmm I see multiple other instances of cross-thread postTask. For example:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Messa...
> 
> I believe that is probably safe, but for subtle reasons, because of the use of
a
> lock on the Chromium side:
> 
> - If the MessagePort or its context are destroyed, MessagePort::close will
call
> WebMessagePortChannelImpl::setClient (with nullptr) which acquires its lock.
> - MessagePort::messageAvailable is only called off-thread by
> WebMessagePortChannelImpl::OnMessage, which holds the same lock.
> - It happens that there don't seem to be any other methods that mutate that
> state on the main thread at present.
> 
> Thus it is sequenced so that the destruction of the execution context and the
> processing of the message do not occur simultaneously.
> 
> If you do (as I think you should in any event, see below) stop the watch when
> the context is destroyed (using the lock that I believe Mojo has internally),
> then this might be safe. But I think it's unnecessarily precarious, and
> ExecutionContext does not make a general promise that
ExecutionContext::postTask
> doesn't mutate any other main thread state. WebTaskRunner does have that
> contract.
> 
> (esprehn may disagree with me; I haven't discussed this with him.)
> 
> > > In addition, though watcher is guaranteed to still exist, the
> ExecutionContext
> > > is not (at least, not obviously), since it does not hold a strong
reference.
> I
> > > think you want to hold std::unique_ptr<WebTaskRunner> instead, which
should
> be
> > > thread-safe.
> > 
> > I used WebTaskRunner in earlier versions of this patch, but I ran into an
> issue where layout tests using mojo did not terminate. MojoWatcher would
> continue to fire ready callbacks. Elliot suggested using the task runner from
> ExecutionContext because WebTaskRunner bypasses the scheduler. Making the
change
> fixed those layout tests as well. Is there a way to extract a thread-safe task
> runner from ExecutionContext?
> 
> This object should clean itself up when the context is destroyed, by
subclassing
> ContextLifecycleObserver (unless it's changed due to haraken's recent
changes).
> At that point it can cancel itself, which should deal with this.
> 
> 
> I'm not sure what was meant by "bypassing the scheduler". There may be task
> runners that do, but if you get one of the categorized task runners from
> TaskRunnerHelper, those tasks should be managed by the scheduler.
> 
> Something like this should work, I believe:
> 
> // on the main thread, when the MojoWatcher is created
> std::unique_ptr<WebTaskRunner> taskRunner =
TaskRunnerHelper::get(TaskType::Mojo
> /* or one of the existing ones */,
scriptState->getExecutionContext())->clone();
> 
> This will create a WebTaskRunner which this object owns, but which shares the
> same (thread-safe) task queue as the one owned from the main thread.

Thanks for the code snippet. It works. Done.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoCreateMessagePipeOptions.idl
(right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoCreateMessagePipeOptions.idl:6:
MojoCreateMessagePipeOptionsFlags flags;
On 2017/01/10 20:46:30, jbroman wrote:
> The webby way would be for the particular flags to be booleans in the
> dictionary. As there currently are none, this can go away (and in fact
> MojoCreateMessagePipeOptions can go away entirely, as it could be added as
> "optional MojoCreateMessagePipeOptions" in the future if any options come into
> existence, without breaking compatibility).
> 
> That is, callers today could look like:
> 
> var { result, handle0, handle1 } = mojo.createMessagePipe();
> 
> and in the future, that might become: mojo.createMessagePipe({ extraWidePipes:
> true }) or whatever

Done.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandle.cpp (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandle.cpp:31: return
mojo::CloseRaw(m_handle.release());
On 2017/01/10 20:46:30, jbroman wrote:
> Can this fail (in a case where the result will be useful on the JS side)?
> 
> Most places seem to DCHECK that this is MOJO_RESULT_OK. In that case, maybe
this
> would work:
> 
> void MojoHandle::close() {
>   m_handle.reset();
> }
> 
> Otherwise, maybe a comment explaining this would be helpful.

I followed the cpp API - mojo::Handle::Close - and got rid of the return value.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:23: [CallWith=ScriptState]
MojoWatcher watch(MojoHandleSignals signals, MojoWatchCallback callback);
On 2017/01/10 20:46:31, jbroman wrote:
> On 2017/01/09 at 23:33:10, alokp wrote:
> > Should this return a MojoWatch or an integer that can be cancelled using
> MojoHandle::cancelWatch? The latter seems to be more idiomatic.
> 
> This could be done with a numbered identifier like setTimeout has (as you
> mention), but personally I actually prefer objects for this, rather than
having
> to maintain a context-wide map.
> 
> JavaScript-land would also typically use a dictionary instead of flags for
> signals, like MutationObserverInit, maybe:
> 
> dictionary MojoWatcherInit {
>   optional boolean readable = false;
>   optional boolean writable = false;
>   optional boolean peerClosed = false;
> };
> 
> with uses looking like:
> handle.watch({ readable: true, writable: true }, result => { ... });

Done.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoReadMessageOptions.idl (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoReadMessageOptions.idl:6:
MojoReadMessageFlags flags;
On 2017/01/10 20:46:31, jbroman wrote:
> In JavaScript-land, it would be more idiomatic to have the dictionary be an
> optional argument (our bindings will take care of constructing a default
> dictionary in this case), and have this have specific fields that correspond
to
> the flags:
> 
> boolean mayDiscard = false;
> 
> so that use looks like:
> 
> var result = handle.readMessage();
> var resultThatMayDiscard = handle.readMessage({ mayDiscard: false });
> 
> Doing so reduces the need for numeric constants.

Done.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:41: cancel();
On 2017/01/10 20:46:31, jbroman wrote:
> Hold on, what stops this from being cancelled the next time GC happens? It's
> unusual in JS to expect visible effects from finalization of objects (you're
> usually permitted to just drop references to things you don't care about).
> 
> If I write:
> 
> handle.watch(signals, result => {
>   // ...
> });
> 
> and I'm not interested in cancelling the watch ever (well, more likely, until
> context shutdown)
> 
> I think this object probably needs to be ActiveScriptWrappable so that it's
kept
> alive while the context is alive and it has pending activity (which is
probably
> "the watch is not cancelled").

Done.

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:60: MojoHandleSignalsState,
On 2017/01/10 20:46:31, jbroman wrote:
> Does script not need to know which signals are ready? Is it expected that this
> will be added in the future, or is it never needed?

C API currently does not specify the signals for which callback is fired. The
current usage also only specifies one of the two signals - readable or writable.
Whether this will change in future is a question for mojo team. yzshen/rockot?

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl:8:
MojoWriteMessageFlags flags;
On 2017/01/10 20:46:31, jbroman wrote:
> Similarly, it would be idiomatic to have these flags expanded as separate
> booleans (except that there currently are none, so you can just remove it
until
> there are).

Done.

jbroman

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode13 third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo { On 2017/01/11 at 21:53:01, alokp wrote: ...

3 years, 11 months ago (2017-01-11 23:00:01 UTC) #46

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo {
On 2017/01/11 at 21:53:01, alokp wrote:
> On 2017/01/10 20:46:30, jbroman wrote:
> > On 2017/01/09 at 23:33:09, alokp wrote:
> > > On 2016/12/20 23:21:57, jbroman wrote:
> > > > Should all of these interfaces have [NoInterfaceObject] to avoid their
> > > > interfaces being installed on the global object? If I understand
correctly,
> > they
> > > > will get access to it by being given an instance of Mojo (and can reach
> > > > constants from there).
> > > 
> > > We do need to install MojoHandle interface so that we can do "handle
> > instanceOf MojoHandle". I agree that this particular interface can have
> > NoInterfaceObject, but we may still want to install it for consistency and
not
> > have to pass the instance around just to access the constants?
> > > 
> > > Anyways, I did use NoInterfaceObject in the initial versions of the patch.
> > esprehn@ asked me to remove those. Elliot?
> > 
> > 1. My main concern here is that I do not want all Blink script contexts to
have
> > the Mojo family of objects exposed, which is what will happen at present
(you
> > probably have a test failure in
> > virtual/stable/webexposed/global-interface-listing.html at present).
> 
> Is this concern about cluttering the context with unnecessary interfaces or
something else? We are not unconditionally injecting a Mojo instance into every
context.

Right, I don't want the interface to be visible in contexts where we don't
intend to inject a Mojo object. If NoInterfaceObject is not specified, then by
default each interface has a function added to the global object, which is
visible to script. (e.g. in this case, it would be "window.Mojo"). I do not want
this, or any of the others, to be visible to the open web.

> > 2. My secondary concern is that in those contexts where we do intend to use
it,
> > it's not clear to me whether we want the Mojo stuff visible on the global
(lest
> > cast receiver apps be tempted to issue their own Mojo calls).
> 
> Cast Apps will not consume the mojo APIs directly. They will use Cast SDK,
which in turn will internally use Mojo. Pardon my ignorance - I am not sure what
you mean by this comment. Can you elaborate?

I guess I'm worried that some cast developer will spot the Mojo things on the
global, and be tempted to use it to send their own messages (e.g. if there's
something possible via the Mojo service but not exposed through the Cast SDK).
The harder we make it, the more obvious it will be to the author that they
should not be doing this. I'm not familiar enough with our relationship with
these developers, but I know that on the Web and on Android developers love to
use "private" APIs.

Something like this might be possible:

var savedCreateMessagePipe = Mojo.prototype.createMessagePipe;
Mojo.prototype.createMessagePipe = function(...args) {
  window.secretMojoObject = this;
  return savedCreateMessagePipe.call(this, args);
};
// after cast SDK creates at least one message pipe, author script has gained
access to the |mojo| object

If the interface objects aren't exposed, it becomes harder (though still not
necessarily impossible) to get a handle on these objects.

> > Possibly we could address (1) by applying the infrastructure built for
origin
> > trials to conditionally expose this.
> 
> Can you point me to this infrastructure? I am happy to use it if that is the
recommended approach.

[cc iclelland]
I think the OriginTrialEnabled= IDL attribute is the start of how it works for
origin trials. Ian would know how this infrastructure works generally, and if
what the current best way to conditionally expose something from WebIDL to JS
contexts.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:17: MojoWatcher*
MojoWatcher::create(mojo::Handle handle,
What if !handle.is_valid() to begin with? (i.e. we were passed an invalid
handle).

It looks like this can happen if I close a handle, and then call watch on it.
(Aside: Ordinarily using something in an invalid state would throw an exception;
I haven't thought through the merits of exposing MojoResult vs throwing
exceptions for this case. It sounds like not all non-OK results are
exception-worthy, so maybe sticking with MojoResult is fine.)

Consider this case:

var handle; // a MojoHandle of some kind
handle.close();
handle.watch({ readable: true }, function(result) {
  console.log(result);
});

As written now, whether or not we call the callback with MOJO_INVALID_ARGUMENT
depends on whether we run garbage-collection between when watch is called and
when the callback was scheduled below. Since the watcher has no pending
activity, the context won't keep it alive. Since there's no reference from
script, that won't keep it alive. And since you're using wrapWeakPersistent,
that also won't keep it alive.

If we don't handle this case another way, I think the postTask should use
wrapPersistent(this), to ensure the callback is consistently delivered. (In the
case where the callback is called from Mojo rather than from here, I'm convinced
the difference isn't observable.) An alternative might be to invoke the callback
synchronously, but I assume the choice to be async is intentional.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:59: if (m_handle.is_valid())
{
nit: I mildly prefer early return, but I don't feel strongly.

if (!m_handle.is_valid())
  return MOJO_RESULT_OK;

MojoResult result = MojoCancelWatch(...);
m_handle = mojo::Handle();
return result;

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:69: }
m_callback must be traced here as well

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:76: return
m_handle.is_valid() && getExecutionContext();
nit: the getExecutionContext() check is implied and happens elsewhere

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:102: cancel();
Is this the right thing to do here? mojo/public/c/system/functions.h lists
several cases where it would be illegal to cancel a watch, like if MojoWatch
returns MOJO_RESULT_CANCELLED (or its callback receives that value).

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.h (right):

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.h:39: // ScriptWrappable
nit: ActiveScriptWrappable

alokp

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode13 third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo { > > Right, I don't want ...

3 years, 11 months ago (2017-01-12 01:20:26 UTC) #47

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo {
> 
> Right, I don't want the interface to be visible in contexts where we don't
> intend to inject a Mojo object. If NoInterfaceObject is not specified, then by
> default each interface has a function added to the global object, which is
> visible to script. (e.g. in this case, it would be "window.Mojo"). I do not
want
> this, or any of the others, to be visible to the open web.

OK - Agreed.

> I guess I'm worried that some cast developer will spot the Mojo things on the
> global, and be tempted to use it to send their own messages (e.g. if there's
> something possible via the Mojo service but not exposed through the Cast SDK).
> The harder we make it, the more obvious it will be to the author that they
> should not be doing this. I'm not familiar enough with our relationship with
> these developers, but I know that on the Web and on Android developers love to
> use "private" APIs.
> 

This has not been a problem so far. But agreed that hiding as much as possible
is better. 

> [cc iclelland]
> I think the OriginTrialEnabled= IDL attribute is the start of how it works for
> origin trials. Ian would know how this infrastructure works generally, and if
> what the current best way to conditionally expose something from WebIDL to JS
> contexts.

OK. Looking into OriginTrialEnabled...

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:17: MojoWatcher*
MojoWatcher::create(mojo::Handle handle,
On 2017/01/11 23:00:00, jbroman wrote:
> What if !handle.is_valid() to begin with? (i.e. we were passed an invalid
> handle).
> 
> It looks like this can happen if I close a handle, and then call watch on it.
> (Aside: Ordinarily using something in an invalid state would throw an
exception;
> I haven't thought through the merits of exposing MojoResult vs throwing
> exceptions for this case. It sounds like not all non-OK results are
> exception-worthy, so maybe sticking with MojoResult is fine.)
> 
> Consider this case:
> 
> var handle; // a MojoHandle of some kind
> handle.close();
> handle.watch({ readable: true }, function(result) {
>   console.log(result);
> });
> 
> As written now, whether or not we call the callback with MOJO_INVALID_ARGUMENT
> depends on whether we run garbage-collection between when watch is called and
> when the callback was scheduled below. Since the watcher has no pending
> activity, the context won't keep it alive. Since there's no reference from
> script, that won't keep it alive. And since you're using wrapWeakPersistent,
> that also won't keep it alive.
> 
> If we don't handle this case another way, I think the postTask should use
> wrapPersistent(this), to ensure the callback is consistently delivered. (In
the
> case where the callback is called from Mojo rather than from here, I'm
convinced
> the difference isn't observable.) An alternative might be to invoke the
callback
> synchronously, but I assume the choice to be async is intentional.

Great catch.

I have TODOs about using exception in a couple places including here (see line
34). The other ones are in MojoCreateMessagePipeResult.idl and
MojoReadMessageResult.idl. Do you recommend removing those TODOs and sticking
with MojoResult for now? If so, using wrapPersistent here would be simplest -
Done.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:59: if (m_handle.is_valid())
{
On 2017/01/11 23:00:00, jbroman wrote:
> nit: I mildly prefer early return, but I don't feel strongly.
> 
> if (!m_handle.is_valid())
>   return MOJO_RESULT_OK;
> 
> MojoResult result = MojoCancelWatch(...);
> m_handle = mojo::Handle();
> return result;

Done.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:69: }
On 2017/01/11 23:00:01, jbroman wrote:
> m_callback must be traced here as well

Done.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:76: return
m_handle.is_valid() && getExecutionContext();
On 2017/01/11 23:00:01, jbroman wrote:
> nit: the getExecutionContext() check is implied and happens elsewhere
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/...

Done.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:102: cancel();
On 2017/01/11 23:00:01, jbroman wrote:
> Is this the right thing to do here? mojo/public/c/system/functions.h lists
> several cases where it would be illegal to cancel a watch, like if MojoWatch
> returns MOJO_RESULT_CANCELLED (or its callback receives that value).

Fixed.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.h (right):

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.h:39: // ScriptWrappable
On 2017/01/11 23:00:01, jbroman wrote:
> nit: ActiveScriptWrappable

Done.

esprehn

You don't want to use NoInterfaceObject, this is a real API that's exposed already, just ...

3 years, 11 months ago (2017-01-12 01:30:28 UTC) #48

jbroman

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode13 third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo { On 2017/01/12 at 01:20:26, alokp wrote: ...

3 years, 11 months ago (2017-01-12 02:21:10 UTC) #49

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo {
On 2017/01/12 at 01:20:26, alokp wrote:
> > 
> > Right, I don't want the interface to be visible in contexts where we don't
> > intend to inject a Mojo object. If NoInterfaceObject is not specified, then
by
> > default each interface has a function added to the global object, which is
> > visible to script. (e.g. in this case, it would be "window.Mojo"). I do not
want
> > this, or any of the others, to be visible to the open web.
> 
> OK - Agreed.
> 
> > I guess I'm worried that some cast developer will spot the Mojo things on
the
> > global, and be tempted to use it to send their own messages (e.g. if there's
> > something possible via the Mojo service but not exposed through the Cast
SDK).
> > The harder we make it, the more obvious it will be to the author that they
> > should not be doing this. I'm not familiar enough with our relationship with
> > these developers, but I know that on the Web and on Android developers love
to
> > use "private" APIs.
> > 
> 
> This has not been a problem so far. But agreed that hiding as much as possible
is better. 

OK, it sounds like esprehn sees this as a "real API" that need not be hidden. In
that case,
you don't need [NoInterfaceObject], but you do need to ensure that we don't
expose the
interface object in contexts except cast receiver apps.

> > [cc iclelland]
> > I think the OriginTrialEnabled= IDL attribute is the start of how it works
for
> > origin trials. Ian would know how this infrastructure works generally, and
if
> > what the current best way to conditionally expose something from WebIDL to
JS
> > contexts.
> 
> OK. Looking into OriginTrialEnabled...

To expand on my comment earlier, origin trials and feature policy are both paths
that
can conditionally enable/disable features at runtime, per document. See, e.g.,
ConditionalFeatures.cpp and ConditionalFeaturesForModules.cpp.

iclelland@ would know what the status of the conditional feature code is, and
how we
can use it here. (Previously we had RuntimeEnabled=, but it was a per-process
setting,
whereas now we can have per-frame granularity.)

yzshen1

Apologize for late reply! https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right): https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode60 third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:60: MojoHandleSignalsState, On 2017/01/11 21:53:02, alokp ...

3 years, 11 months ago (2017-01-12 23:38:35 UTC) #50

Apologize for late reply!

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:60: MojoHandleSignalsState,
On 2017/01/11 21:53:02, alokp wrote:
> On 2017/01/10 20:46:31, jbroman wrote:
> > Does script not need to know which signals are ready? Is it expected that
this
> > will be added in the future, or is it never needed?
> 
> C API currently does not specify the signals for which callback is fired. The
> current usage also only specifies one of the two signals - readable or
writable.
> Whether this will change in future is a question for mojo team. yzshen/rockot?

I don't think it is super useful. I am fine not having it for now.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.h (right):

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.h:22: static const MojoResult kResultOk
= MOJO_HANDLE_SIGNAL_NONE;
Why having these constant definitions? They don't seem to be used anywhere?
Besides, assigning MOJO_HANDLE_SIGNAL_NONE to kResultOk is probably wrong.

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult
writeMessage(MojoWriteMessageOptions options);
Does it make sense to do:
- introduce subclass MojoMessagePipeHandle and move these methods there;
- introduce down-cast methods in MojoHandle such as "asMessagePipe".

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl (right):

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl:5: dictionary
MojoWriteMessageOptions {
IMO, it makes sense to have separate parameters instead of a single dictionary:
- API like MojoWriteMessage is unlikely to change. If we need any major change
to the paramters, we will have a new method instead.
- Calling method with a dictionary is more verbose. For example:
    handle.writeMessage({buffer: someBuffer, handles: someHandles});
  versus
    handle.writeMessage(someBuffer, someHandles);

WDYT?

Ken Rockot(use gerrit already)

On Thu, Jan 12, 2017 at 3:38 PM, <yzshen@chromium.org> wrote: > Apologize for late reply! ...

3 years, 11 months ago (2017-01-12 23:42:21 UTC) #51

On Thu, Jan 12, 2017 at 3:38 PM, <yzshen@chromium.org> wrote:

> Apologize for late reply!
>
>
> https://codereview.chromium.org/2400563002/diff/350001/
> third_party/WebKit/Source/core/mojo/MojoWatcher.cpp
> File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):
>
> https://codereview.chromium.org/2400563002/diff/350001/
> third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode60
> third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:60:
> MojoHandleSignalsState,
> On 2017/01/11 21:53:02, alokp wrote:
> > On 2017/01/10 20:46:31, jbroman wrote:
> > > Does script not need to know which signals are ready? Is it expected
> that this
> > > will be added in the future, or is it never needed?
> >
> > C API currently does not specify the signals for which callback is
> fired. The
> > current usage also only specifies one of the two signals - readable or
> writable.
> > Whether this will change in future is a question for mojo team.
> yzshen/rockot?
>
> I don't think it is super useful. I am fine not having it for now.
>
> https://codereview.chromium.org/2400563002/diff/390001/
> third_party/WebKit/Source/core/mojo/Mojo.h
> File third_party/WebKit/Source/core/mojo/Mojo.h (right):
>
> https://codereview.chromium.org/2400563002/diff/390001/
> third_party/WebKit/Source/core/mojo/Mojo.h#newcode22
> third_party/WebKit/Source/core/mojo/Mojo.h:22: static const MojoResult
> kResultOk = MOJO_HANDLE_SIGNAL_NONE;
> Why having these constant definitions? They don't seem to be used
> anywhere?
> Besides, assigning MOJO_HANDLE_SIGNAL_NONE to kResultOk is probably
> wrong.
>
> https://codereview.chromium.org/2400563002/diff/410001/
> third_party/WebKit/Source/core/mojo/MojoHandle.idl
> File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):
>
> https://codereview.chromium.org/2400563002/diff/410001/
> third_party/WebKit/Source/core/mojo/MojoHandle.idl#newcode10
> third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult
> writeMessage(MojoWriteMessageOptions options);
> Does it make sense to do:
> - introduce subclass MojoMessagePipeHandle and move these methods there;
> - introduce down-cast methods in MojoHandle such as "asMessagePipe".
>

or as a potentially more idiomatic alternative to the second suggestion, a
MojoMessagePipeHandle constructor which takes a MojoHandle argument.

We also discussed offline the possibility of adding a core C API function
to query the handle type, so that we could do runtime safety checks for
such downcasting.


> https://codereview.chromium.org/2400563002/diff/410001/
> third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl
> File third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl
> (right):
>
> https://codereview.chromium.org/2400563002/diff/410001/
> third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl#newcode5
> third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl:5:
> dictionary MojoWriteMessageOptions {
> IMO, it makes sense to have separate parameters instead of a single
> dictionary:
> - API like MojoWriteMessage is unlikely to change. If we need any major
> change to the paramters, we will have a new method instead.
> - Calling method with a dictionary is more verbose. For example:
> handle.writeMessage({buffer: someBuffer, handles: someHandles});
> versus
> handle.writeMessage(someBuffer, someHandles);
>
> WDYT?
>
> https://codereview.chromium.org/2400563002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

Ken Rockot(use gerrit already)

On Thu, Jan 12, 2017 at 3:38 PM, <yzshen@chromium.org> wrote: > Apologize for late reply! ...

3 years, 11 months ago (2017-01-12 23:42:21 UTC) #52

On Thu, Jan 12, 2017 at 3:38 PM, <yzshen@chromium.org> wrote:

> Apologize for late reply!
>
>
> https://codereview.chromium.org/2400563002/diff/350001/
> third_party/WebKit/Source/core/mojo/MojoWatcher.cpp
> File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):
>
> https://codereview.chromium.org/2400563002/diff/350001/
> third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode60
> third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:60:
> MojoHandleSignalsState,
> On 2017/01/11 21:53:02, alokp wrote:
> > On 2017/01/10 20:46:31, jbroman wrote:
> > > Does script not need to know which signals are ready? Is it expected
> that this
> > > will be added in the future, or is it never needed?
> >
> > C API currently does not specify the signals for which callback is
> fired. The
> > current usage also only specifies one of the two signals - readable or
> writable.
> > Whether this will change in future is a question for mojo team.
> yzshen/rockot?
>
> I don't think it is super useful. I am fine not having it for now.
>
> https://codereview.chromium.org/2400563002/diff/390001/
> third_party/WebKit/Source/core/mojo/Mojo.h
> File third_party/WebKit/Source/core/mojo/Mojo.h (right):
>
> https://codereview.chromium.org/2400563002/diff/390001/
> third_party/WebKit/Source/core/mojo/Mojo.h#newcode22
> third_party/WebKit/Source/core/mojo/Mojo.h:22: static const MojoResult
> kResultOk = MOJO_HANDLE_SIGNAL_NONE;
> Why having these constant definitions? They don't seem to be used
> anywhere?
> Besides, assigning MOJO_HANDLE_SIGNAL_NONE to kResultOk is probably
> wrong.
>
> https://codereview.chromium.org/2400563002/diff/410001/
> third_party/WebKit/Source/core/mojo/MojoHandle.idl
> File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):
>
> https://codereview.chromium.org/2400563002/diff/410001/
> third_party/WebKit/Source/core/mojo/MojoHandle.idl#newcode10
> third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult
> writeMessage(MojoWriteMessageOptions options);
> Does it make sense to do:
> - introduce subclass MojoMessagePipeHandle and move these methods there;
> - introduce down-cast methods in MojoHandle such as "asMessagePipe".
>

or as a potentially more idiomatic alternative to the second suggestion, a
MojoMessagePipeHandle constructor which takes a MojoHandle argument.

We also discussed offline the possibility of adding a core C API function
to query the handle type, so that we could do runtime safety checks for
such downcasting.


> https://codereview.chromium.org/2400563002/diff/410001/
> third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl
> File third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl
> (right):
>
> https://codereview.chromium.org/2400563002/diff/410001/
> third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl#newcode5
> third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl:5:
> dictionary MojoWriteMessageOptions {
> IMO, it makes sense to have separate parameters instead of a single
> dictionary:
> - API like MojoWriteMessage is unlikely to change. If we need any major
> change to the paramters, we will have a new method instead.
> - Calling method with a dictionary is more verbose. For example:
> handle.writeMessage({buffer: someBuffer, handles: someHandles});
> versus
> handle.writeMessage(someBuffer, someHandles);
>
> WDYT?
>
> https://codereview.chromium.org/2400563002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

esprehn

On 2017/01/12 at 02:21:10, jbroman wrote: > https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl > File third_party/WebKit/Source/core/mojo/Mojo.idl (right): > > https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode13 ...

3 years, 11 months ago (2017-01-13 01:01:26 UTC) #53

On 2017/01/12 at 02:21:10, jbroman wrote:
>
https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/mojo/Mojo.idl (right):
> 
>
https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo {
> On 2017/01/12 at 01:20:26, alokp wrote:
> > > 
> > > Right, I don't want the interface to be visible in contexts where we don't
> > > intend to inject a Mojo object. If NoInterfaceObject is not specified,
then by
> > > default each interface has a function added to the global object, which is
> > > visible to script. (e.g. in this case, it would be "window.Mojo"). I do
not want
> > > this, or any of the others, to be visible to the open web.
> > 
> > OK - Agreed.
> > 
> > > I guess I'm worried that some cast developer will spot the Mojo things on
the
> > > global, and be tempted to use it to send their own messages (e.g. if
there's
> > > something possible via the Mojo service but not exposed through the Cast
SDK).
> > > The harder we make it, the more obvious it will be to the author that they
> > > should not be doing this. I'm not familiar enough with our relationship
with
> > > these developers, but I know that on the Web and on Android developers
love to
> > > use "private" APIs.
> > > 
> > 
> > This has not been a problem so far. But agreed that hiding as much as
possible is better. 
> 
> OK, it sounds like esprehn sees this as a "real API" that need not be hidden.
In that case,
> you don't need [NoInterfaceObject], but you do need to ensure that we don't
expose the
> interface object in contexts except cast receiver apps.

Sorry to be more clear, NoInterfaceObject means the interface literally does not
exist, you end up with a plain object and no type or .constructor:
https://www.w3.org/TR/WebIDL-1/#NoInterfaceObject

The spec says you should never use it for anything except for mixin situations
(the spec calls these "supplemental interfaces").

So using it here isn't correct per spec and also doesn't have the behavior you
want.

Separately we already expose some mojo API to JS through some Gin bindings, so
cast can already reach them. This API shouldn't be leaking any new features to
cast. I'm not sure it's worth trying to hide it, though we can brainstorm ideas
how to do that, it's quite hard since the mojo generated JS still needs to get
to it somehow.

alokp

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode13 third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo { > you don't need [NoInterfaceObject], but ...

3 years, 11 months ago (2017-01-13 01:07:56 UTC) #54

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.idl (right):

https://codereview.chromium.org/2400563002/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.idl:13: interface Mojo {
> you don't need [NoInterfaceObject], but you do need to ensure that we don't
> expose the
> interface object in contexts except cast receiver apps.

Not sure if it makes any difference. But we also need to enable mojo-js bindings
for layout tests, webui, and headless. See MojoBindingsType:
https://cs.chromium.org/chromium/src/content/renderer/mojo_bindings_controlle...

Anyways I added origin trials but I am still missing things since I do not know
how to enable the bindings for layout tests for example.

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/Mojo.h (right):

https://codereview.chromium.org/2400563002/diff/390001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/Mojo.h:22: static const MojoResult kResultOk
= MOJO_HANDLE_SIGNAL_NONE;
On 2017/01/12 23:38:35, yzshen1 wrote:
> Why having these constant definitions? They don't seem to be used anywhere?
> Besides, assigning MOJO_HANDLE_SIGNAL_NONE to kResultOk is probably wrong.

These are defined to keep the values of various error codes defined in Mojo.idl
in sync with that in Mojo C API. MOJO_HANDLE_SIGNAL_NONE is a typo, which has
been fixed in the new patch.

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoHandle.idl (right):

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult
writeMessage(MojoWriteMessageOptions options);
On 2017/01/12 23:38:35, yzshen1 wrote:
> Does it make sense to do:
> - introduce subclass MojoMessagePipeHandle and move these methods there;
> - introduce down-cast methods in MojoHandle such as "asMessagePipe".

Good Idea. I will prefer to do it in a followup patch though to make it easier
to port over current usage without significant changes.

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl (right):

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl:5: dictionary
MojoWriteMessageOptions {
On 2017/01/12 23:38:35, yzshen1 wrote:
> IMO, it makes sense to have separate parameters instead of a single
dictionary:
> - API like MojoWriteMessage is unlikely to change. If we need any major change
> to the paramters, we will have a new method instead.
> - Calling method with a dictionary is more verbose. For example:
>     handle.writeMessage({buffer: someBuffer, handles: someHandles});
>   versus
>     handle.writeMessage(someBuffer, someHandles);
> 
> WDYT?
> 
> 

I do not have strong opinions on this. I guess this is a question for jbroman@
who initially suggested adding the dictionary.

alokp

alokp@chromium.org changed reviewers: + iclelland@chromium.org

3 years, 11 months ago (2017-01-13 01:13:06 UTC) #55

alokp

iclelland@: Could you please provide pointers on how to enable an origin-trial feature for say ...

3 years, 11 months ago (2017-01-13 01:13:07 UTC) #56

jbroman

> https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Source/core/mojo/MojoHandle.idl#newcode10 > third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult writeMessage(MojoWriteMessageOptions options); > On 2017/01/12 23:38:35, yzshen1 wrote: > > ...

3 years, 11 months ago (2017-01-13 19:25:40 UTC) #57

jbroman

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl File third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl (right): https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl#newcode5 third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl:5: dictionary MojoWriteMessageOptions { On 2017/01/13 at 01:07:56, alokp wrote: ...

3 years, 11 months ago (2017-01-13 19:25:59 UTC) #58

alokp

> Separately we already expose some mojo API to JS through some Gin bindings, so ...

3 years, 11 months ago (2017-01-13 20:10:34 UTC) #59

Ken Rockot(use gerrit already)

I'm not entirely sure where the origin trials suggestion came from, but they don't seem ...

3 years, 11 months ago (2017-01-13 20:40:34 UTC) #60

Ken Rockot(use gerrit already)

I'm not entirely sure where the origin trials suggestion came from, but they don't seem ...

3 years, 11 months ago (2017-01-13 20:40:34 UTC) #61

yzshen1

On 2017/01/13 19:25:40, jbroman wrote: > > > https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Source/core/mojo/MojoHandle.idl#newcode10 > > third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult > writeMessage(MojoWriteMessageOptions ...

3 years, 11 months ago (2017-01-13 20:49:25 UTC) #62

On 2017/01/13 19:25:40, jbroman wrote:
> >
>
https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult
> writeMessage(MojoWriteMessageOptions options);
> > On 2017/01/12 23:38:35, yzshen1 wrote:
> > > Does it make sense to do:
> > > - introduce subclass MojoMessagePipeHandle and move these methods there;
> > > - introduce down-cast methods in MojoHandle such as "asMessagePipe".
> > 
> > Good Idea. I will prefer to do it in a followup patch though to make it
easier
> to port over current usage without significant changes.
> 
> For what it's worth, I'm not aware of any JavaScript APIs that have a "cast"
> operation like this.
> 
> Usually, things just have their derived interface (if you get a child of the
> body element, for instance, you don't have to cast it to HTMLDivElement, it
> already is one). From an API perspective, this is nicest (you can just say
> "handle instanceof MojoMessagePipeHandle" to check, and exactly the right
> methods will exist). But this does mean that you'd have to check the type of
> each handle (unless you know for sure what it is) in order to create a wrapper
> for it, and I don't know how expensive that is to know in Mojo-land.

I think it makes sense.

As Ken said, we will need a new mojo C API to query handle type. It would
involve a lock operation and handle table lookup. I think the cost should be
okay (but Ken could correct me if I am wrong).

> 
> Failing that, I think the next pattern I've seen is throwing an exception (or
> otherwise reporting an error) if the wrong kind of thing is used.
> 
> It'll also be a little finicky to have such an API, because only one object
can
> be the sole owner of the ScopedMessagePipeHandle. So you probably end up
> creating two ScriptWrappables for every message pipe handle, then: one that's
a
> MojoHandle and owns the handle, and one that's a MojoMessagePipeHandle which
> only has indirect access to the handle via the MojoHandle (unless you want to
> invalidate the original object when you cast, which would be weird). But then
> you have two JS objects that represent the same logical object, which is
> unfortunate.

Ken Rockot(use gerrit already)

On Jan 13, 2017 12:49 PM, <yzshen@chromium.org> wrote: On 2017/01/13 19:25:40, jbroman wrote: > > ...

3 years, 11 months ago (2017-01-13 20:52:58 UTC) #63

On Jan 13, 2017 12:49 PM, <yzshen@chromium.org> wrote:

On 2017/01/13 19:25:40, jbroman wrote:
> >
>
https://codereview.chromium.org/2400563002/diff/410001/
third_party/WebKit/Source/core/mojo/MojoHandle.idl#newcode10
> > third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult
> writeMessage(MojoWriteMessageOptions options);
> > On 2017/01/12 23:38:35, yzshen1 wrote:
> > > Does it make sense to do:
> > > - introduce subclass MojoMessagePipeHandle and move these methods
there;
> > > - introduce down-cast methods in MojoHandle such as "asMessagePipe".
> >
> > Good Idea. I will prefer to do it in a followup patch though to make it
easier
> to port over current usage without significant changes.
>
> For what it's worth, I'm not aware of any JavaScript APIs that have a
"cast"
> operation like this.
>
> Usually, things just have their derived interface (if you get a child of
the
> body element, for instance, you don't have to cast it to HTMLDivElement,
it
> already is one). From an API perspective, this is nicest (you can just say
> "handle instanceof MojoMessagePipeHandle" to check, and exactly the right
> methods will exist). But this does mean that you'd have to check the type
of
> each handle (unless you know for sure what it is) in order to create a
wrapper
> for it, and I don't know how expensive that is to know in Mojo-land.

I think it makes sense.

As Ken said, we will need a new mojo C API to query handle type. It would
involve a lock operation and handle table lookup. I think the cost should be
okay (but Ken could correct me if I am wrong).


Should be fine and SGTM



>
> Failing that, I think the next pattern I've seen is throwing an exception
(or
> otherwise reporting an error) if the wrong kind of thing is used.
>
> It'll also be a little finicky to have such an API, because only one
object
can
> be the sole owner of the ScopedMessagePipeHandle. So you probably end up
> creating two ScriptWrappables for every message pipe handle, then: one
that's
a
> MojoHandle and owns the handle, and one that's a MojoMessagePipeHandle
which
> only has indirect access to the handle via the MojoHandle (unless you
want to
> invalidate the original object when you cast, which would be weird). But
then
> you have two JS objects that represent the same logical object, which is
> unfortunate.



https://codereview.chromium.org/2400563002/

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

Ken Rockot(use gerrit already)

On Jan 13, 2017 12:49 PM, <yzshen@chromium.org> wrote: On 2017/01/13 19:25:40, jbroman wrote: > > ...

3 years, 11 months ago (2017-01-13 20:52:59 UTC) #64

On Jan 13, 2017 12:49 PM, <yzshen@chromium.org> wrote:

On 2017/01/13 19:25:40, jbroman wrote:
> >
>
https://codereview.chromium.org/2400563002/diff/410001/
third_party/WebKit/Source/core/mojo/MojoHandle.idl#newcode10
> > third_party/WebKit/Source/core/mojo/MojoHandle.idl:10: MojoResult
> writeMessage(MojoWriteMessageOptions options);
> > On 2017/01/12 23:38:35, yzshen1 wrote:
> > > Does it make sense to do:
> > > - introduce subclass MojoMessagePipeHandle and move these methods
there;
> > > - introduce down-cast methods in MojoHandle such as "asMessagePipe".
> >
> > Good Idea. I will prefer to do it in a followup patch though to make it
easier
> to port over current usage without significant changes.
>
> For what it's worth, I'm not aware of any JavaScript APIs that have a
"cast"
> operation like this.
>
> Usually, things just have their derived interface (if you get a child of
the
> body element, for instance, you don't have to cast it to HTMLDivElement,
it
> already is one). From an API perspective, this is nicest (you can just say
> "handle instanceof MojoMessagePipeHandle" to check, and exactly the right
> methods will exist). But this does mean that you'd have to check the type
of
> each handle (unless you know for sure what it is) in order to create a
wrapper
> for it, and I don't know how expensive that is to know in Mojo-land.

I think it makes sense.

As Ken said, we will need a new mojo C API to query handle type. It would
involve a lock operation and handle table lookup. I think the cost should be
okay (but Ken could correct me if I am wrong).


Should be fine and SGTM



>
> Failing that, I think the next pattern I've seen is throwing an exception
(or
> otherwise reporting an error) if the wrong kind of thing is used.
>
> It'll also be a little finicky to have such an API, because only one
object
can
> be the sole owner of the ScopedMessagePipeHandle. So you probably end up
> creating two ScriptWrappables for every message pipe handle, then: one
that's
a
> MojoHandle and owns the handle, and one that's a MojoMessagePipeHandle
which
> only has indirect access to the handle via the MojoHandle (unless you
want to
> invalidate the original object when you cast, which would be weird). But
then
> you have two JS objects that represent the same logical object, which is
> unfortunate.



https://codereview.chromium.org/2400563002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

esprehn

Yeah I think it's being used as a hack here to turn it on. We ...

3 years, 11 months ago (2017-01-13 22:04:33 UTC) #65

alokp

Thanks esprehn@. I have addressed all other comments in the latest patch. I will wait ...

3 years, 11 months ago (2017-01-13 23:40:31 UTC) #66

Thanks esprehn@.

I have addressed all other comments in the latest patch. I will wait for
jbroman@ to weigh in on SecureContext before looking into it.

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl (right):

https://codereview.chromium.org/2400563002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWriteMessageOptions.idl:5: dictionary
MojoWriteMessageOptions {
On 2017/01/13 19:25:59, jbroman wrote:
> On 2017/01/13 at 01:07:56, alokp wrote:
> > On 2017/01/12 23:38:35, yzshen1 wrote:
> > > IMO, it makes sense to have separate parameters instead of a single
> dictionary:
> > > - API like MojoWriteMessage is unlikely to change. If we need any major
> change
> > > to the paramters, we will have a new method instead.
> > > - Calling method with a dictionary is more verbose. For example:
> > >     handle.writeMessage({buffer: someBuffer, handles: someHandles});
> > >   versus
> > >     handle.writeMessage(someBuffer, someHandles);
> > > 
> > > WDYT?
> > > 
> > > 
> > 
> > I do not have strong opinions on this. I guess this is a question for
jbroman@
> who initially suggested adding the dictionary.
> 
> I don't believe I insisted on a dictionary for these particular parameters. I
> mostly want:
> - flags to be in a dictionary instead of numeric constants
> - APIs to be designed in such a way that we could add more arguments if needed
> in the future.
> 
> |buffer| and |handles| seem like core enough arguments that they can live
> outside the dictionary. Since there are no other options right now, the
> dictionary can go away (and can be added as a trailing optional argument if we
> do need more options in the future).
> i.e., if in the future there _are_ write flags, then we would be able to add a
> dictionary, with syntax like:
> 
> handle.writeMessage(someBuffer, someHandles, { highPriority: true });  // or
> whatever options may be added

The new patch removes MojoWriteMessageOptions. It also renames
MojoReadMessageOptions -> MojoReadMessageFlags and MojoWatchOptions ->
MojoHandleSignals to be consistent with C API.

jbroman

On 2017/01/13 at 23:40:31, alokp wrote: > I have addressed all other comments in the ...

3 years, 11 months ago (2017-01-14 05:16:30 UTC) #67

alokp

> I _am_ suggesting that the interfaces must be conditionally enabled (certainly > if we ...

3 years, 11 months ago (2017-01-14 22:49:02 UTC) #68

jbroman

On 2017/01/14 at 22:49:02, alokp wrote: > > I _am_ suggesting that the interfaces must ...

3 years, 11 months ago (2017-01-15 05:42:54 UTC) #69

iclelland

On 2017/01/15 05:42:54, jbroman wrote: > On 2017/01/14 at 22:49:02, alokp wrote: > > > ...

3 years, 11 months ago (2017-01-17 14:26:47 UTC) #70

On 2017/01/15 05:42:54, jbroman wrote:
> On 2017/01/14 at 22:49:02, alokp wrote:
> > > I _am_ suggesting that the interfaces must be conditionally enabled
> (certainly
> > > if we are going to expose the constructors on the global object).
> > 
> > There is no need or plan to expose constructors for any of the Mojo
> interfaces. We only plan to inject an instance of Mojo into specific contexts
> for layout tests, webui, headless, and cast.
> 
> The WebIDL bindings expose a JavaScript function object (the "interface
object")
> as a property of the global, which is the constructor of objects of that
> interface, even if you don't declare a constructor in WebIDL (invoking the
> function will throw), unless it is disabled or the interface has
> [NoInterfaceObject].
> 
> > So the question is whether it is worth hiding the interfaces (which cannot
be
> used by themselves without the mojo instance or any data/pipe handle) from
> contexts that do not need it.
> 
> In my view, the interface object should not be visible to unblessed web page
> contexts in Chrome (desktop/mobile).
> 
> RuntimeEnabled could be used to make it controllable at a per-process level
> (which would at least restrict it to Chromecast), but I suspect that we do
want
> to control this per-frame (IIUC it's possible for a cast receiver app to embed
> an <iframe>, which should presumably not have access to special APIs). I'll
ask
> iclelland@ to comment on how the per-frame mechanism used by origin trials
could
> be adapted.

Agreed that origin trials itself isn't likely to be of much use here -- I am
working on a per-frame replacement for RuntimeEnableFeatures; the early design
proposal is in
https://docs.google.com/document/d/1PHkCEyfUKwhoJEiRwcio9JWU5xQ1OuvIRb8enSbfY...
.  The rough idea there is that we end up with something like
ConditionalFeatures::<someFeature>Enabled(LocalFrame*) for features which
require per-frame context, and ConditionalFeatures::<someFeature>Enabled() for
those that don't -- like current runtime-enabled features. The implementation of
those methods would depend on the feature -- whether it's controlled by feature
policy, finch, flags, origin trials, or something else.

For origin trials in IDL bindings, we delay the installation of members until
the object containing them is first accessed (when its prototype or constructor
is first requested through PerContextData). At that point,
PerContextData::constructorForType() will call into ConditionalFeatures.cpp to
see whether any additional members need to be installed for the given type. --
this only happens once per type, on first access.

Actually delaying the installation, and generation of
V8<type>::install<feature>() methods is all done by the bindings generator,
triggered by the presence of either the "OriginTrialEnabled" or
"FeaturePolicyEnabled" extended attributes in IDL. We could make this a generic
"Feature" extended attribute, rather than try to add another attribute name for
this case. In that case, setting any "Feature=<something>" attribute in IDL will
make the member  conditional on context. At that point, you would just add a
couple of lines to installConditionalFeaturesCore() to check whether the feature
should be installed, and actually call the generated install<Feature> method.

alokp

iclelland@: Thanks for your reply. > Actually delaying the installation, and generation of > V8<type>::install<feature>() ...

3 years, 11 months ago (2017-01-17 23:02:09 UTC) #71

jbroman

On 2017/01/17 at 23:02:09, alokp wrote: > iclelland@: Thanks for your reply. > > > ...

3 years, 11 months ago (2017-01-17 23:03:21 UTC) #72

alokp

https://codereview.chromium.org/2400563002/diff/490001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/490001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode29 third_party/WebKit/Source/core/mojo/Mojo.idl:29: static MojoCreateMessagePipeResult createMessagePipe(); Elliot: When I declare this function ...

3 years, 11 months ago (2017-01-20 23:11:57 UTC) #73

jbroman

https://codereview.chromium.org/2400563002/diff/490001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/490001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode29 third_party/WebKit/Source/core/mojo/Mojo.idl:29: static MojoCreateMessagePipeResult createMessagePipe(); On 2017/01/20 at 23:11:56, alokp wrote: ...

3 years, 11 months ago (2017-01-24 21:21:04 UTC) #74

jbroman

https://codereview.chromium.org/2400563002/diff/490001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/490001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode29 third_party/WebKit/Source/core/mojo/Mojo.idl:29: static MojoCreateMessagePipeResult createMessagePipe(); On 2017/01/24 at 21:21:03, jbroman wrote: ...

3 years, 11 months ago (2017-01-24 21:25:51 UTC) #75

alokp

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

3 years, 10 months ago (2017-01-27 01:22:03 UTC) #76

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 01:23:36 UTC) #77

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 01:50:16 UTC) #78

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/310972) linux_chromium_tsan_rel_ng on ...

3 years, 10 months ago (2017-01-27 01:50:17 UTC) #79

alokp

Description was changed from ========== [WIP] Adds Mojo IDL. BUG=647036 ========== to ========== Adds Mojo ...

3 years, 10 months ago (2017-01-27 07:28:51 UTC) #80

alokp

I have added a bunch of layout tests and also fixed a few issues found ...

3 years, 10 months ago (2017-01-27 07:34:29 UTC) #81

alokp

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

3 years, 10 months ago (2017-01-27 07:40:14 UTC) #82

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 07:40:28 UTC) #83

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 09:40:10 UTC) #84

commit-bot: I haz the power

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_ng/builds/372231)

3 years, 10 months ago (2017-01-27 09:40:11 UTC) #85

jbroman

LGTM You'll need to rebaseline the non-stable interface listings, and I'd suggest having a Mojo ...

3 years, 10 months ago (2017-01-27 20:35:16 UTC) #86

LGTM

You'll need to rebaseline the non-stable interface listings, and I'd suggest
having a Mojo person (e.g. rockot, yzshen) double-check that all of the
particulars about invoking the Mojo API are correct.

Nice layout tests -- very easy to follow.

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/mojo/message-pipe.html (right):

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/message-pipe.html:13: read: (handle) => {
super-nit: while you're being fancy with JavaScript, you can even do this:

return {
  read(handle) {
    // function body here
  },
  write(handle) {
    // function body here
  }
};

if you want to

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/message-pipe.html:19: dataOut.every((value,
index) => { assert_equals(value, dataIn[index]); });
nit: isn't this (and the previous line) what assert_array_equals does?

assert_array_equals(new Uint8Array(buffer), dataIn);

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/message-pipe.html:36: let {result, handle0,
handle1} = Mojo.createMessagePipe();
nit: either check result or remove it from the destructing here? ({handle0,
handle1} will do)

As it happens this also removes the variable name collision that caused the
block below. (You could also use "var" which is more loose than var, and allows
redeclaring the variable. I don't mind either way.)

Same elsewhere below.

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:43: nullptr, result));
nit: It seems more consistent, if you're trying to make it look the same as if
an error happened later, to still pass the MojoWatcher as the |this| object
(i.e. wrapPersistent(this)), rather than nullptr. Not a big deal.

alokp

rockot/yzshen: Could you please review if my understanding/usage of mojo API is correct? Thanks! https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/LayoutTests/mojo/message-pipe.html ...

3 years, 10 months ago (2017-01-27 23:54:47 UTC) #87

rockot/yzshen: Could you please review if my understanding/usage of mojo API is
correct? Thanks!

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/mojo/message-pipe.html (right):

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/message-pipe.html:13: read: (handle) => {
On 2017/01/27 20:35:16, jbroman wrote:
> super-nit: while you're being fancy with JavaScript, you can even do this:
> 
> return {
>   read(handle) {
>     // function body here
>   },
>   write(handle) {
>     // function body here
>   }
> };
> 
> if you want to

Nice. Done.

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/message-pipe.html:19: dataOut.every((value,
index) => { assert_equals(value, dataIn[index]); });
On 2017/01/27 20:35:16, jbroman wrote:
> nit: isn't this (and the previous line) what assert_array_equals does?
> 
> assert_array_equals(new Uint8Array(buffer), dataIn);

I was not sure if assert_array_equals works on typed-arrays as well. Done.

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/mojo/message-pipe.html:36: let {result, handle0,
handle1} = Mojo.createMessagePipe();
On 2017/01/27 20:35:16, jbroman wrote:
> nit: either check result or remove it from the destructing here? ({handle0,
> handle1} will do)
> 
> As it happens this also removes the variable name collision that caused the
> block below. (You could also use "var" which is more loose than var, and
allows
> redeclaring the variable. I don't mind either way.)
> 
> Same elsewhere below.

I thought it was necessary to extract all keys. Removed result, which is being
checked in "create pipe" already - no need to test the same thing several times.

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):

https://codereview.chromium.org/2400563002/diff/570001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:43: nullptr, result));
On 2017/01/27 20:35:16, jbroman wrote:
> nit: It seems more consistent, if you're trying to make it look the same as if
> an error happened later, to still pass the MojoWatcher as the |this| object
> (i.e. wrapPersistent(this)), rather than nullptr. Not a big deal.

Good idea - done.

Ken Rockot(use gerrit already)

Very sorry for the delay. Everything looks great, just one question about watcher. https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp File ...

3 years, 10 months ago (2017-01-31 16:33:16 UTC) #89

jbroman

https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right): https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode107 third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:107: // It is safe to assume the MojoWatcher still ...

3 years, 10 months ago (2017-01-31 17:55:50 UTC) #90

yzshen1

LGTM https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right): https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode107 third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:107: // It is safe to assume the MojoWatcher ...

3 years, 10 months ago (2017-01-31 17:57:33 UTC) #91

alokp

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org, yzshen@chromium.org Link to ...

3 years, 10 months ago (2017-01-31 18:42:48 UTC) #94

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-31 18:43:51 UTC) #95

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-31 18:55:51 UTC) #96

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-31 18:55:53 UTC) #97

Ken Rockot(use gerrit already)

On 2017/01/31 at 17:57:33, yzshen wrote: > LGTM > > https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp > File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right): ...

3 years, 10 months ago (2017-01-31 19:27:05 UTC) #99

On 2017/01/31 at 17:57:33, yzshen wrote:
> LGTM
> 
>
https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):
> 
>
https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:107: // It is safe to
assume the MojoWatcher still exists because this
> On 2017/01/31 16:33:16, Ken Rockot wrote:
> > If a MojoWatcher is garbage-collected during the lifetime of its context
without
> > anyone having explicitly called cancel() from JS, how does the watch get
> > cancelled? I don't see any guarantees that this object stays alive, or that
> > MojoCancelWatch is necessarily called before this object is destroyed. Does
it
> > have something to do with the V8 custom wrapper thing which I don't
understand?
> > :)
> 
> IIUC (but I don't know much about v8 stuff), MojoWatcher is never
garbage-collected as long as m_handle is valid?
> Because m_handle is set only when MojoWatch succeeds, it should get an
onHandleReady call eventually.

The problem is that MojoWatch only takes a raw function pointer and a uintptr_t
context (a raw pointer to |this| in this case). As long as the MojoHandle is
alive and able to receive notifications, the Mojo system layer will invoke
onHandleReady when a relevant event occurs. This will happen until
MojoCancelWatch is explicitly called with the same handle and the same context.

I do not see any guarantee that MojoCancelWatch is called before ~MojoWatcher.
If JS code explicitly calls cancel() or the ExecutionContext is destroyed,
MojoCancelWatch is called. If the MojoHandle is closed, the watch is implicitly
cancelled and MojoCancelWatch doesn't need to be called.

But if all JS references to the MojoWatcher go away, cancel() hasn't been
explicitly called, the ExecutionContext is still alive, and the MojoHandle
remains open, this means the next notification on that handle will still invoke
onHandleReady with a context which now points to a deleted MojoWatcher.

Of course the MojoWatcher won't be GCed as long as a weakPersistent is bound to
a pending task, but AFAICT that is only the case in the period between the
constructor and the first onHandleReady (in some cases), and the period between
any onHandleReady and its corresponding runReadyCallback execytion. Unless I'm
missing something.

Ken Rockot(use gerrit already)

On 2017/01/31 at 19:27:05, Ken Rockot wrote: > On 2017/01/31 at 17:57:33, yzshen wrote: > ...

3 years, 10 months ago (2017-01-31 20:31:41 UTC) #100

On 2017/01/31 at 19:27:05, Ken Rockot wrote:
> On 2017/01/31 at 17:57:33, yzshen wrote:
> > LGTM
> > 
> >
https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right):
> > 
> >
https://codereview.chromium.org/2400563002/diff/590001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:107: // It is safe to
assume the MojoWatcher still exists because this
> > On 2017/01/31 16:33:16, Ken Rockot wrote:
> > > If a MojoWatcher is garbage-collected during the lifetime of its context
without
> > > anyone having explicitly called cancel() from JS, how does the watch get
> > > cancelled? I don't see any guarantees that this object stays alive, or
that
> > > MojoCancelWatch is necessarily called before this object is destroyed.
Does it
> > > have something to do with the V8 custom wrapper thing which I don't
understand?
> > > :)
> > 
> > IIUC (but I don't know much about v8 stuff), MojoWatcher is never
garbage-collected as long as m_handle is valid?
> > Because m_handle is set only when MojoWatch succeeds, it should get an
onHandleReady call eventually.
> 
> The problem is that MojoWatch only takes a raw function pointer and a
uintptr_t context (a raw pointer to |this| in this case). As long as the
MojoHandle is alive and able to receive notifications, the Mojo system layer
will invoke onHandleReady when a relevant event occurs. This will happen until
MojoCancelWatch is explicitly called with the same handle and the same context.
> 
> I do not see any guarantee that MojoCancelWatch is called before ~MojoWatcher.
If JS code explicitly calls cancel() or the ExecutionContext is destroyed,
MojoCancelWatch is called. If the MojoHandle is closed, the watch is implicitly
cancelled and MojoCancelWatch doesn't need to be called.
> 
> But if all JS references to the MojoWatcher go away, cancel() hasn't been
explicitly called, the ExecutionContext is still alive, and the MojoHandle
remains open, this means the next notification on that handle will still invoke
onHandleReady with a context which now points to a deleted MojoWatcher.
> 
> Of course the MojoWatcher won't be GCed as long as a weakPersistent is bound
to a pending task, but AFAICT that is only the case in the period between the
constructor and the first onHandleReady (in some cases), and the period between
any onHandleReady and its corresponding runReadyCallback execytion. Unless I'm
missing something.

Ah, indeed I was missing something. Yuzhu explained ActiveScriptWrappable to me
off-thread. LGTM

alokp

alokp@chromium.org changed reviewers: + haraken@chromium.org

3 years, 10 months ago (2017-02-08 00:49:37 UTC) #101

haraken

Would you add more explanation to the CL description? Where do we have tests? https://codereview.chromium.org/2400563002/diff/610001/third_party/WebKit/Source/bindings/core/v8/custom/V8MojoWatcherCustom.cpp ...

3 years, 10 months ago (2017-02-08 02:27:29 UTC) #103

alokp

Description was changed from ========== Adds Mojo IDL. BUG=647036 ========== to ========== Implements JS bindings ...

3 years, 10 months ago (2017-02-08 05:02:31 UTC) #104

alokp

Added CL description. The patch already contains LayoutTests. Are you looking for any other kind ...

3 years, 10 months ago (2017-02-08 05:19:19 UTC) #105

alokp

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

3 years, 10 months ago (2017-02-08 05:23:42 UTC) #106

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 05:23:58 UTC) #107

haraken

LGTM with comments. https://codereview.chromium.org/2400563002/diff/630001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/630001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode5 third_party/WebKit/Source/core/mojo/Mojo.idl:5: typedef unsigned long MojoResult; Add a ...

3 years, 10 months ago (2017-02-08 06:32:10 UTC) #108

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 09:41:11 UTC) #109

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-08 09:41:16 UTC) #110

jbroman

https://codereview.chromium.org/2400563002/diff/630001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right): https://codereview.chromium.org/2400563002/diff/630001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode40 third_party/WebKit/Source/core/mojo/MojoWatcher.cpp:40: context->postTask( On 2017/02/08 at 06:32:10, haraken wrote: > We're ...

3 years, 10 months ago (2017-02-08 13:20:04 UTC) #111

alokp

https://codereview.chromium.org/2400563002/diff/630001/third_party/WebKit/Source/core/mojo/Mojo.idl File third_party/WebKit/Source/core/mojo/Mojo.idl (right): https://codereview.chromium.org/2400563002/diff/630001/third_party/WebKit/Source/core/mojo/Mojo.idl#newcode5 third_party/WebKit/Source/core/mojo/Mojo.idl:5: typedef unsigned long MojoResult; On 2017/02/08 06:32:09, haraken wrote: ...

3 years, 10 months ago (2017-02-08 17:34:41 UTC) #112

alokp

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, jbroman@chromium.org, yzshen@chromium.org, haraken@chromium.org ...

3 years, 10 months ago (2017-02-08 17:35:03 UTC) #114

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-08 17:35:34 UTC) #115

haraken

On 2017/02/08 13:20:04, jbroman wrote: > https://codereview.chromium.org/2400563002/diff/630001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp > File third_party/WebKit/Source/core/mojo/MojoWatcher.cpp (right): > > https://codereview.chromium.org/2400563002/diff/630001/third_party/WebKit/Source/core/mojo/MojoWatcher.cpp#newcode40 > ...

3 years, 10 months ago (2017-02-08 17:56:53 UTC) #116

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 650001, "attempt_start_ts": 1486575302300900, "parent_rev": "888b44e14b7afac674d55e305c8dd89a393cbdac", "commit_rev": "a97e1315d39a13e0c239615e93ffc28fd868c911"}

3 years, 10 months ago (2017-02-08 19:55:06 UTC) #117

commit-bot: I haz the power

Description was changed from ========== Implements JS bindings for mojo core module. Mojo core module ...

3 years, 10 months ago (2017-02-08 19:55:45 UTC) #118

commit-bot: I haz the power

3 years, 10 months ago (2017-02-08 19:55:48 UTC) #119

Message was sent while issue was closed.

Committed patchset #34 (id:650001) as
https://chromium.googlesource.com/chromium/src/+/a97e1315d39a13e0c239615e93ff...

Issue 2400563002: Adds Mojo IDL. (Closed)

Description

Patch Set 1 #

Patch Set 2 : install works #

Patch Set 3 : avoids touching module system #

Patch Set 4 : implements InterfaceProvider #

Patch Set 5 : fixes modulename typo #

Patch Set 6 : implements messagepipe #

Patch Set 7 : addressed comments #

Patch Set 8 : rebase #

Patch Set 9 : rebase #

Patch Set 10 : rebase #

Patch Set 11 : fixed (most) tests #

Patch Set 12 : addressed comments #

Patch Set 13 : Uses ExecutionContext TaskRunner #

Patch Set 14 : rebase #

Patch Set 15 : uses inline capacity #

Patch Set 16 : rebase #

Patch Set 17 : rebase #

Patch Set 18 : rebase #

Patch Set 19 : addressed comments #

Patch Set 20 : rebase #

Patch Set 21 : addressed comments #

Patch Set 22 : addressed comments #

Patch Set 23 : origin trials #

Patch Set 24 : cleanup #

Patch Set 25 : injects mojo object #

Patch Set 26 : static createMessagePipe #

Patch Set 27 : moar tests #

Patch Set 28 : fixes tests #

Patch Set 29 : more tests #

Patch Set 30 : fixes typo #

Patch Set 31 : addressed comments #

Patch Set 32 : rebaseline #

Patch Set 33 : removes visitDOMWrapper #

Patch Set 34 : uses WebTaskRunner #

Messages