Mojo: add javascript bindings for request/response

Mojo: add javascript bindings for request/response

If a mojom interface method specifies response parameters, then we'll generate javascript bindings that provide an additional callback parameter used to pass the response parameters. This mirrors the C++ implementation.

A future improvement will likely be to use promises instead, especially now that they are enabled on trunk.

As part of this CL, connector.js was also made consistent with connector.cc. It starts reading the message pipe immediately, handles write failures in a similar fashion, etc.

The Connection type from connector.js is factored out into a separate file as it now holds a Router object. router.js mirrors router.cc. The Connection type is really the mirror of the C++ RemotePtr<S> type.

Some basic request/response tests were added to connection_unittests.js. We'll probably want more testing.

R=abarth@chromium.org, mpcomplete@chromium.org

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

[abarth-chromium, 2014-03-26 16:45:11]

https://codereview.chromium.org/207503004/diff/120001/mojo/public/bindings/js...
File mojo/public/bindings/js/connector.js (right):

https://codereview.chromium.org/207503004/diff/120001/mojo/public/bindings/js...
mojo/public/bindings/js/connector.js:13: this.drop_writes_ = false;
dropWrites_ ?

https://codereview.chromium.org/207503004/diff/120001/mojo/public/bindings/js...
mojo/public/bindings/js/connector.js:56: drop_writes_ = true;
this.dropWrites_ ?

[darin (slow to review), 2014-03-26 17:12:29]

Thanks. I have to get used to writing JS code ;-)


On Wed, Mar 26, 2014 at 9:45 AM, <abarth@chromium.org> wrote:

>
> https://codereview.chromium.org/207503004/diff/120001/
> mojo/public/bindings/js/connector.js
> File mojo/public/bindings/js/connector.js (right):
>
> https://codereview.chromium.org/207503004/diff/120001/
> mojo/public/bindings/js/connector.js#newcode13
> mojo/public/bindings/js/connector.js:13: this.drop_writes_ = false;
> dropWrites_ ?
>
> https://codereview.chromium.org/207503004/diff/120001/
> mojo/public/bindings/js/connector.js#newcode56
> mojo/public/bindings/js/connector.js:56: drop_writes_ = true;
> this.dropWrites_ ?
>
> https://codereview.chromium.org/207503004/
>

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

[Matt Perry, 2014-03-26 18:19:43]

LGTM, small nits below

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
File mojo/public/bindings/generators/js_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
mojo/public/bindings/generators/js_templates/interface_definition.tmpl:57: {%-
if method.response_parameters == None %}
is the idea that if the method has response_parameters, then acceptWithResponder
would be called instead? if so, maybe we should handle the case and raise/print
an error?

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
File mojo/public/bindings/js/router.js (right):

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
mojo/public/bindings/js/router.js:32: if (requestID == 0)
why not just start nextRequestId at 1? or are you worried about wraparound? I
think JS ints have 52 bits of precision (it's just a double), so that'd take a
while.

[abarth-chromium, 2014-03-26 18:26:32]

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
File mojo/public/bindings/generators/js_templates/interface_definition.tmpl
(right):

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
mojo/public/bindings/generators/js_templates/interface_definition.tmpl:15: ,
closure
Why not use promises instead of callbacks?  This JS environment should have
Promises enabled now.

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
mojo/public/bindings/generators/js_templates/interface_definition.tmpl:39: var
response_params =
responseParams

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
mojo/public/bindings/generators/js_templates/interface_definition.tmpl:87: var
response_params =
responseParams

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
File mojo/public/bindings/js/codec.js (right):

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
mojo/public/bindings/js/codec.js:355: encoder.write32(3);  // num_fields.
Should we make this a named constant?

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
File mojo/public/bindings/js/connector.js (right):

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
mojo/public/bindings/js/connector.js:56: dropWrites_ = true;
Sorry for being unclear.  You need to write "this." if you want to store this
value in a field of this object.  As written, this line of code should throw an
exception in the unit test environment because we run in strict mode.  Are we
missing test coverage for this line?

[abarth-chromium, 2014-03-26 19:05:45]

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
File mojo/public/bindings/js/router.js (right):

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
mojo/public/bindings/js/router.js:23: };
Should we drop the responders_ too?

[darin (slow to review), 2014-03-27 00:19:49]

Thanks for the feedback guys. I mentioned in the CL description that my
plan is to follow this up with a promises based solution. I think that'll
mostly just be additive to the current CL. Of course it'll change the API,
but we have no consumers at this point.

-Darin


On Wed, Mar 26, 2014 at 11:26 AM, <abarth@chromium.org> wrote:

>
> https://codereview.chromium.org/207503004/diff/170001/
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl
> File
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl
> (right):
>
> https://codereview.chromium.org/207503004/diff/170001/
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl#
> newcode15
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl:15:
> , closure
> Why not use promises instead of callbacks?  This JS environment should
> have Promises enabled now.
>
> https://codereview.chromium.org/207503004/diff/170001/
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl#
> newcode39
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl:39:
> var response_params =
> responseParams
>
> https://codereview.chromium.org/207503004/diff/170001/
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl#
> newcode87
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl:87:
> var response_params =
> responseParams
>
> https://codereview.chromium.org/207503004/diff/170001/
> mojo/public/bindings/js/codec.js
> File mojo/public/bindings/js/codec.js (right):
>
> https://codereview.chromium.org/207503004/diff/170001/
> mojo/public/bindings/js/codec.js#newcode355
> mojo/public/bindings/js/codec.js:355: encoder.write32(3);  //
> num_fields.
> Should we make this a named constant?
>
> https://codereview.chromium.org/207503004/diff/170001/
> mojo/public/bindings/js/connector.js
> File mojo/public/bindings/js/connector.js (right):
>
> https://codereview.chromium.org/207503004/diff/170001/
> mojo/public/bindings/js/connector.js#newcode56
> mojo/public/bindings/js/connector.js:56: dropWrites_ = true;
> Sorry for being unclear.  You need to write "this." if you want to store
> this value in a field of this object.  As written, this line of code
> should throw an exception in the unit test environment because we run in
> strict mode.  Are we missing test coverage for this line?
>
> https://codereview.chromium.org/207503004/
>

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

[abarth-chromium, 2014-03-27 03:42:14]

On 2014/03/27 00:19:49, darin wrote:
> Thanks for the feedback guys. I mentioned in the CL description that my
> plan is to follow this up with a promises based solution. I think that'll
> mostly just be additive to the current CL. Of course it'll change the API,
> but we have no consumers at this point.

Oh, I'm not sure how I missed that.  LGTM

[darin (slow to review), 2014-03-27 05:06:08]

On 2014/03/26 18:19:43, Matt Perry wrote:
> LGTM, small nits below
> 
>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
> File mojo/public/bindings/generators/js_templates/interface_definition.tmpl
> (right):
> 
>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl:57: {%-
> if method.response_parameters == None %}
> is the idea that if the method has response_parameters, then
acceptWithResponder
> would be called instead?

Yes

> if so, maybe we should handle the case and raise/print
> an error?

The current design of the C++ bindings is also to silently return
false for unhandled messages. The reason we don't assert here is
because a client might be sending messages that an old implementation
of the service doesn't know about. It isn't a bug in the service to
not know about newer message types. That said, we could add
assertions that the messages we do know about aren't passed to the
wrong function (accept versus acceptWithResponder). Hmm...


>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
> File mojo/public/bindings/js/router.js (right):
> 
>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
> mojo/public/bindings/js/router.js:32: if (requestID == 0)
> why not just start nextRequestId at 1? or are you worried about wraparound? I
> think JS ints have 52 bits of precision (it's just a double), so that'd take a
> while.

Yup. The C++ code has the same wraparound protection (but for a 64-bit integer
in that case). I agree it'll be a rare event ;-)

[darin (slow to review), 2014-03-27 05:11:10]

On 2014/03/26 18:26:32, abarth wrote:
...
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl:39: var
> response_params =
> responseParams

Done

https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/ge...
> mojo/public/bindings/generators/js_templates/interface_definition.tmpl:87: var
> response_params =
> responseParams
> 

Done

>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
> File mojo/public/bindings/js/codec.js (right):
> 
>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
> mojo/public/bindings/js/codec.js:355: encoder.write32(3);  // num_fields.
> Should we make this a named constant?

Good question. In this case, there are exactly 3 lines that follow corresponding
to each of the 3 fields. Ditto for the MessageBuilder case which has 2 fields.
It seems pretty clear to see this literal followed by the corresponding number
of setters. Moving it to a constant defined far from this spot might actually
work against readability.


>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
> File mojo/public/bindings/js/connector.js (right):
> 
>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
> mojo/public/bindings/js/connector.js:56: dropWrites_ = true;
> Sorry for being unclear.  You need to write "this." if you want to store this
> value in a field of this object.  As written, this line of code should throw
an
> exception in the unit test environment because we run in strict mode.  Are we
> missing test coverage for this line?

Good catch. Yes, we do not have test coverage here. I should add that. I should
be
able to replicate the C++ test for this case.

[darin (slow to review), 2014-03-27 05:15:22]

On 2014/03/26 19:05:45, abarth wrote:
>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
> File mojo/public/bindings/js/router.js (right):
> 
>
https://codereview.chromium.org/207503004/diff/170001/mojo/public/bindings/js...
> mojo/public/bindings/js/router.js:23: };
> Should we drop the responders_ too?

Good idea.

[darin (slow to review), 2014-03-27 07:56:32]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-03-27 07:57:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/207503004/210001

[commit-bot: I haz the power, 2014-03-27 09:22:54]

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

[commit-bot: I haz the power, 2014-03-27 09:22:55]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[darin (slow to review), 2014-03-27 17:10:54]

The CQ bit was checked by darin@chromium.org

[commit-bot: I haz the power, 2014-03-27 17:11:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/darin@chromium.org/207503004/210001

[darin (slow to review), 2014-03-27 18:13:33]

Committed patchset #9 manually as r259932 (presubmit successful).

[tkent, 2014-03-28 00:41:08]

On 2014/03/27 18:13:33, darin wrote:
> Committed patchset #9 manually as r259932 (presubmit successful).

Let me revert this.  As win_rel try bot noticed, this broke
WebUIMojoTest.EndToEnd test.

[tkent, 2014-03-28 00:42:33]

A revert of this CL has been created in
https://codereview.chromium.org/215883004/ by tkent@chromium.org.

The reason for reverting is: broke WebUIMojoTest.EndToEnd test.
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/355...
.

[darin (slow to review), 2014-03-28 00:48:52]

Hmm... an exception was already added to disable that test. The proper fix
is in the CQ.
On Mar 27, 2014 5:42 PM, <tkent@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/215883004/ by tkent@chromium.org.
>
> The reason for reverting is: broke WebUIMojoTest.EndToEnd test.
> http://build.chromium.org/p/chromium.webkit/builders/
> Linux%20Tests/builds/35528/steps/content_browsertests/logs/EndToEnd
> .
>
> https://codereview.chromium.org/207503004/
>

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

[darin (slow to review), 2014-03-28 05:50:59]

On 2014/03/28 00:48:52, darin wrote:
> Hmm... an exception was already added to disable that test. The proper fix
> is in the CQ.
> On Mar 27, 2014 5:42 PM, <mailto:tkent@chromium.org> wrote:
> 
> > A revert of this CL has been created in
> > https://codereview.chromium.org/215883004/ by mailto:tkent@chromium.org.
> >
> > The reason for reverting is: broke WebUIMojoTest.EndToEnd test.
> > http://build.chromium.org/p/chromium.webkit/builders/
> > Linux%20Tests/builds/35528/steps/content_browsertests/logs/EndToEnd
> > .
> >
> > https://codereview.chromium.org/207503004/

Ah, but the exception was only made on Windows :-/