Mojo JS bindings: introduce concepts that are more similar to C++ bindings:

Mojo JS bindings: introduce concepts that are more similar to C++ bindings:

- InterfacePtrInfo
- InterfaceRequest
- InterfacePtr
- Binding

BUG=579646
Committed: https://crrev.com/5f3032e780d840cb59233978f457f76225336a3f
Cr-Commit-Position: refs/heads/master@{#436201}

[yzshen1, 2016-12-02 00:08:13]

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

[commit-bot: I haz the power, 2016-12-02 00:08:56]

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

[yzshen1, 2016-12-02 00:22:58]

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

[yzshen1, 2016-12-02 00:22:59]

Hi, Ken.

Would you please take a look? Thanks!

https://codereview.chromium.org/2549683002/diff/1/mojo/public/js/connection.js
File mojo/public/js/connection.js (right):

https://codereview.chromium.org/2549683002/diff/1/mojo/public/js/connection.j...
mojo/public/js/connection.js:15: var kProxyProperties =
Symbol("proxyProperties");
I moved the original contents in bindings.js to this file.
So there is no need to review this one carefully.

Eventually I will remove this entire file. The code is so confusing!!

https://codereview.chromium.org/2549683002/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/2549683002/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/js_templates/interface_definition.tmpl:5:
function {{interface.name}}Ptr() {
As you can see, I decided to create FooPtr which is a subclass of
InterfacePtrBase. The benefits are:
- shorter name when creating an interface pointer;
- both generic InterfacePtrBase methods and mojom interface methods are directly
exposed by FooPtr;
- if there is name collision between generic InterfacePtrBase methods and mojom
interface methods, people still can use something like
  InterfacePtrBase.prototype.isBound.call(obj)
to call the hidden methods. And we could provide syntax sugar for that, such as
ToInterfacePtrBase(obj).isBound().

One thing should be noted is that with this approach, we will eventually need a
FooAssociatedPtr subclass as well. I think code bloat shouldn't be an issue
given that eventually we will compile our generated JS code.

WDYT?

[commit-bot: I haz the power, 2016-12-02 01:18:08]

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

[commit-bot: I haz the power, 2016-12-02 01:18:09]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-12-02 02:42:40]

LGTM as-is, but I have an optional suggestion for your consideration:

Instead of a treating InterfacePtrBase as a superclass of the generated
InterfacePtrs, what if we just made it accessible via a "ptr" property?
Basically this is inverting the situation we didn't want, which was to make the
interface itself referenced by a property.

This has the nice property that if we extend the base interface later, we don't
have to worry about breaking existing client code. i.e. the only reserved
property becomes "ptr". WDYT?

[yzshen1, 2016-12-02 16:34:22]

On 2016/12/02 02:42:40, Ken Rockot wrote:
> LGTM as-is, but I have an optional suggestion for your consideration:
> 
> Instead of a treating InterfacePtrBase as a superclass of the generated
> InterfacePtrs, what if we just made it accessible via a "ptr" property?
> Basically this is inverting the situation we didn't want, which was to make
the
> interface itself referenced by a property.
> 
> This has the nice property that if we extend the base interface later, we
don't
> have to worry about breaking existing client code. i.e. the only reserved
> property becomes "ptr". WDYT?

The property that we don't have to worry about breaking client code when
extending the base does sound nice.
One minor thing is that a callsite fooPtr.ptr.isBound() has two "ptr"s in it. I
was thinking about something like "controller", because those methods are used
to set up/config a pointer, but that seems a little long:
    if (fooPtr.controller.isBound()) { ... }

Do you have other good choices in mind?

Thanks!

[Ken Rockot(use gerrit already), 2016-12-02 16:40:33]

On Fri, Dec 2, 2016 at 8:34 AM, <yzshen@chromium.org> wrote:

> On 2016/12/02 02:42:40, Ken Rockot wrote:
> > LGTM as-is, but I have an optional suggestion for your consideration:
> >
> > Instead of a treating InterfacePtrBase as a superclass of the generated
> > InterfacePtrs, what if we just made it accessible via a "ptr" property?
> > Basically this is inverting the situation we didn't want, which was to
> make
> the
> > interface itself referenced by a property.
> >
> > This has the nice property that if we extend the base interface later, we
> don't
> > have to worry about breaking existing client code. i.e. the only reserved
> > property becomes "ptr". WDYT?
>
> The property that we don't have to worry about breaking client code when
> extending the base does sound nice.
> One minor thing is that a callsite fooPtr.ptr.isBound() has two "ptr"s in
> it. I
> was thinking about something like "controller", because those methods are
> used
> to set up/config a pointer, but that seems a little long:
> if (fooPtr.controller.isBound()) { ... }
>

While I realize the tests here do this, I think it's atypical for proxy
symbol names to actually include "Ptr". Typically it's just a "foo" and not
a "fooPtr". Though maybe we do expect this pattern in JS?


> Do you have other good choices in mind?
>
> Thanks!
>
>
>
>
>
>
> https://codereview.chromium.org/2549683002/
>

-- 
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.

[yzshen1, 2016-12-02 16:45:13]

On 2016/12/02 16:34:22, yzshen1 wrote:
> On 2016/12/02 02:42:40, Ken Rockot wrote:
> > LGTM as-is, but I have an optional suggestion for your consideration:
> > 
> > Instead of a treating InterfacePtrBase as a superclass of the generated
> > InterfacePtrs, what if we just made it accessible via a "ptr" property?
> > Basically this is inverting the situation we didn't want, which was to make
> the
> > interface itself referenced by a property.
> > 
> > This has the nice property that if we extend the base interface later, we
> don't
> > have to worry about breaking existing client code. i.e. the only reserved
> > property becomes "ptr". WDYT?
> 
> The property that we don't have to worry about breaking client code when
> extending the base does sound nice.
> One minor thing is that a callsite fooPtr.ptr.isBound() has two "ptr"s in it.
I
> was thinking about something like "controller", because those methods are used
> to set up/config a pointer, but that seems a little long:
>     if (fooPtr.controller.isBound()) { ... }
> 
> Do you have other good choices in mind?
> 
> Thanks!

Btw, another thing that may worth mentioning: in C++ bindings a Binding object
is usually a member of the Impl object, in JS doing that creates circular
references. I have a comment in bindings.js talking about this, but I imagine a
C++ mojo user may skip the comment and rely on their C++ mojo experience and do
the wrong thing. If you have good ideas how to help users avoid such issues,
please let me know.

[yzshen1, 2016-12-02 17:14:04]

On 2016/12/02 16:40:33, Ken Rockot wrote:
> On Fri, Dec 2, 2016 at 8:34 AM, <mailto:yzshen@chromium.org> wrote:
> 
> > On 2016/12/02 02:42:40, Ken Rockot wrote:
> > > LGTM as-is, but I have an optional suggestion for your consideration:
> > >
> > > Instead of a treating InterfacePtrBase as a superclass of the generated
> > > InterfacePtrs, what if we just made it accessible via a "ptr" property?
> > > Basically this is inverting the situation we didn't want, which was to
> > make
> > the
> > > interface itself referenced by a property.
> > >
> > > This has the nice property that if we extend the base interface later, we
> > don't
> > > have to worry about breaking existing client code. i.e. the only reserved
> > > property becomes "ptr". WDYT?
> >
> > The property that we don't have to worry about breaking client code when
> > extending the base does sound nice.
> > One minor thing is that a callsite fooPtr.ptr.isBound() has two "ptr"s in
> > it. I
> > was thinking about something like "controller", because those methods are
> > used
> > to set up/config a pointer, but that seems a little long:
> > if (fooPtr.controller.isBound()) { ... }
> >
> 
> While I realize the tests here do this, I think it's atypical for proxy
> symbol names to actually include "Ptr". Typically it's just a "foo" and not
> a "fooPtr". Though maybe we do expect this pattern in JS?

I think you are right: I searched our C++ code and only ~5% usage have _ptr in
the name.

"mojom::[a-zA-Z]+Ptr\ [a-z]+_ptr[();,_] case:yes" : there are 36 results
"mojom::[a-zA-Z]+Ptr\ [a-z]+[();,_] case:yes" : there are 764 results

Okay. I am sold.
So only 5% usage actually have 
> 
> 
> > Do you have other good choices in mind?
> >
> > Thanks!
> >
> >
> >
> >
> >
> >
> > https://codereview.chromium.org/2549683002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-12-02 17:42:08]

Updated the CL as suggested. PTAL. Thanks!

[Ken Rockot(use gerrit already), 2016-12-02 18:17:52]

LGTM!

Options I can see for addressing the binding circular refs:

 - What we do now (restrict the impl)
 - Follow the disposable pattern for binding objects
 - Add native binding support to stash and retrieve weak refs

AFAICT JS is still unlikely to get builtin weak refs any time soon. It'd be kind
of weird to add them to the native mojo JS bindings surface, but still an
option.

[yzshen1, 2016-12-02 23:40:15]

On 2016/12/02 18:17:52, Ken Rockot wrote:
> LGTM!
> 
> Options I can see for addressing the binding circular refs:
> 
>  - What we do now (restrict the impl)
>  - Follow the disposable pattern for binding objects
>  - Add native binding support to stash and retrieve weak refs
> 
> AFAICT JS is still unlikely to get builtin weak refs any time soon. It'd be
kind
> of weird to add them to the native mojo JS bindings surface, but still an
> option.

I feel that the disposable pattern is pretty contagious (IIUC, a class
containing a disposable member should be made disposable). Maybe I haven't fully
understood the idea.
Adding native weak ref support does seem a little overkill. :)

Maybe I can land this as is, and try to convert some user code and see how it
works in real-world scenarios.
WDYT? Thanks!

[Ken Rockot(use gerrit already), 2016-12-02 23:41:23]

Oh, I definitely didn't mean that we should do anything more in this CL.
Still LGTM!

On Fri, Dec 2, 2016 at 3:40 PM, <yzshen@chromium.org> wrote:

> On 2016/12/02 18:17:52, Ken Rockot wrote:
> > LGTM!
> >
> > Options I can see for addressing the binding circular refs:
> >
> > - What we do now (restrict the impl)
> > - Follow the disposable pattern for binding objects
> > - Add native binding support to stash and retrieve weak refs
> >
> > AFAICT JS is still unlikely to get builtin weak refs any time soon. It'd
> be
> kind
> > of weird to add them to the native mojo JS bindings surface, but still an
> > option.
>
> I feel that the disposable pattern is pretty contagious (IIUC, a class
> containing a disposable member should be made disposable). Maybe I haven't
> fully
> understood the idea.
> Adding native weak ref support does seem a little overkill. :)
>
> Maybe I can land this as is, and try to convert some user code and see how
> it
> works in real-world scenarios.
> WDYT? Thanks!
>
>
>
>
> https://codereview.chromium.org/2549683002/
>

-- 
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.

[yzshen1, 2016-12-04 19:13:53]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-12-04 19:14:05]

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

[commit-bot: I haz the power, 2016-12-04 20:22:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480878833171380,
"parent_rev": "78dc39f36a1ace5c7239a887f1ddf7508d0a8252", "commit_rev":
"53a6bc33e6aefa1da555dcba5650ff88f505c0c7"}

[commit-bot: I haz the power, 2016-12-04 20:23:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-04 20:25:26]

Description was changed from

==========
Mojo JS bindings: introduce concepts that are more similar to C++ bindings:

- InterfacePtrInfo
- InterfaceRequest
- InterfacePtr
- Binding

BUG=579646
==========

to

==========
Mojo JS bindings: introduce concepts that are more similar to C++ bindings:

- InterfacePtrInfo
- InterfaceRequest
- InterfacePtr
- Binding

BUG=579646

Committed: https://crrev.com/5f3032e780d840cb59233978f457f76225336a3f
Cr-Commit-Position: refs/heads/master@{#436201}
==========

[commit-bot: I haz the power, 2016-12-04 20:25:27]

Patchset 2 (id:??) landed as
https://crrev.com/5f3032e780d840cb59233978f457f76225336a3f
Cr-Commit-Position: refs/heads/master@{#436201}