Mojo: Use Promises for request/response calls in the JavaScript bindings.

Mojo: Use Promises for request/response calls in the JavaScript bindings.

BUG=359812
R=abarth@chromium.org

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

[Matt Perry, 2014-04-04 00:42:20]

Here's what it looks like when we use Promises for the request/response stuff.
I'm not totally convinced it's a win here, but maybe in more complex examples it
will be better.

I also had to jump through some hoops to get the unittest working. Promises
don't execute until the stack unwinds, so I had to ensure our mock asyncWait
triggered in a deferred task.

[abarth-chromium, 2014-04-04 05:28:09]

On 2014/04/04 00:42:20, Matt Perry wrote:
> Here's what it looks like when we use Promises for the request/response stuff.
> I'm not totally convinced it's a win here, but maybe in more complex examples
it
> will be better.

Promises are the right JavaScript idiom to use for this situation.  There isn't
a big win when used in isolation like this, but when you build up a bigger
system, they make it easier to keep things straight.

> I also had to jump through some hoops to get the unittest working. Promises
> don't execute until the stack unwinds, so I had to ensure our mock asyncWait
> triggered in a deferred task.

The unit testing does seem a bit ugly, but that's probably the right trade-off
to make the system more programmable.

LGTM

[darin (slow to review), 2014-04-04 05:34:18]

I haven't reviewed the code, but one of my goals with using ES promises
here was to use it as a way to report connection errors. In the C++
bindings, we do this via the ErrorHandler interface. Instead of mapping
that interface to JS, we should just dispatch an exception through the
promise interface. (Sorry if you have already done so in this CL!)

-Darin


On Thu, Apr 3, 2014 at 10:28 PM, <abarth@chromium.org> wrote:

> On 2014/04/04 00:42:20, Matt Perry wrote:
>
>> Here's what it looks like when we use Promises for the request/response
>> stuff.
>> I'm not totally convinced it's a win here, but maybe in more complex
>> examples
>>
> it
>
>> will be better.
>>
>
> Promises are the right JavaScript idiom to use for this situation.  There
> isn't
> a big win when used in isolation like this, but when you build up a bigger
> system, they make it easier to keep things straight.
>
>
>  I also had to jump through some hoops to get the unittest working.
>> Promises
>> don't execute until the stack unwinds, so I had to ensure our mock
>> asyncWait
>> triggered in a deferred task.
>>
>
> The unit testing does seem a bit ugly, but that's probably the right
> trade-off
> to make the system more programmable.
>
> LGTM
>
> https://codereview.chromium.org/223043006/
>

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

[Matt Perry, 2014-04-04 18:40:14]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-04-04 18:40:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/223043006/80001

[Matt Perry, 2014-04-04 18:40:50]

No error handling with this CL, but I can look at that next.

On 2014/04/04 05:34:18, darin wrote:
> I haven't reviewed the code, but one of my goals with using ES promises
> here was to use it as a way to report connection errors. In the C++
> bindings, we do this via the ErrorHandler interface. Instead of mapping
> that interface to JS, we should just dispatch an exception through the
> promise interface. (Sorry if you have already done so in this CL!)
> 
> -Darin
> 
> 
> On Thu, Apr 3, 2014 at 10:28 PM, <mailto:abarth@chromium.org> wrote:
> 
> > On 2014/04/04 00:42:20, Matt Perry wrote:
> >
> >> Here's what it looks like when we use Promises for the request/response
> >> stuff.
> >> I'm not totally convinced it's a win here, but maybe in more complex
> >> examples
> >>
> > it
> >
> >> will be better.
> >>
> >
> > Promises are the right JavaScript idiom to use for this situation.  There
> > isn't
> > a big win when used in isolation like this, but when you build up a bigger
> > system, they make it easier to keep things straight.
> >
> >
> >  I also had to jump through some hoops to get the unittest working.
> >> Promises
> >> don't execute until the stack unwinds, so I had to ensure our mock
> >> asyncWait
> >> triggered in a deferred task.
> >>
> >
> > The unit testing does seem a bit ugly, but that's probably the right
> > trade-off
> > to make the system more programmable.
> >
> > LGTM
> >
> > https://codereview.chromium.org/223043006/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-04-04 21:23:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/223043006/80001

[commit-bot: I haz the power, 2014-04-04 21:24:21]

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

[commit-bot: I haz the power, 2014-04-04 21:24:21]

Failed to apply patch for
mojo/public/bindings/generators/js_templates/interface_definition.tmpl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  A         mojo/public/bindings
  Created missing directory mojo/public/bindings.
  A         mojo/public/bindings/generators
  Created missing directory mojo/public/bindings/generators.
  A         mojo/public/bindings/generators/js_templates
  Created missing directory mojo/public/bindings/generators/js_templates.
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: mojo/public/bindings/generators/js_templates/interface_definition.tmpl
  |diff --git
a/mojo/public/bindings/generators/js_templates/interface_definition.tmpl
b/mojo/public/bindings/generators/js_templates/interface_definition.tmpl
  |index
fc77059e76ce136790ada9529ba66248916b9a1e..3a42ae5618afe81fa5144f6486315edca057a71d
100644
  |--- a/mojo/public/bindings/generators/js_templates/interface_definition.tmpl
  |+++ b/mojo/public/bindings/generators/js_templates/interface_definition.tmpl
  --------------------------
  No file to patch.  Skipping patch.
  3 out of 3 hunks ignored

Patch:      
mojo/public/bindings/generators/js_templates/interface_definition.tmpl
Index: mojo/public/bindings/generators/js_templates/interface_definition.tmpl
diff --git
a/mojo/public/bindings/generators/js_templates/interface_definition.tmpl
b/mojo/public/bindings/generators/js_templates/interface_definition.tmpl
index
fc77059e76ce136790ada9529ba66248916b9a1e..3a42ae5618afe81fa5144f6486315edca057a71d
100644
--- a/mojo/public/bindings/generators/js_templates/interface_definition.tmpl
+++ b/mojo/public/bindings/generators/js_templates/interface_definition.tmpl
@@ -11,9 +11,6 @@
 {%- for parameter in method.parameters -%}
 {{parameter.name}}{% if not loop.last %}, {% endif %}
 {%- endfor -%}
-{%- if method.response_parameters != None -%}
-, closure
-{%- endif -%}
 ) {
     var params = new {{interface.name}}_{{method.name}}_Params();
 {%- for parameter in method.parameters %}
@@ -28,21 +25,20 @@
     var message = builder.finish();
     this.receiver_.accept(message);
 {%- else %}
-    var builder = new codec.MessageWithRequestIDBuilder(
-        k{{interface.name}}_{{method.name}}_Name,
-        codec.align({{interface.name}}_{{method.name}}_Params.encodedSize),
-        codec.kMessageExpectsResponse, 0);
-    builder.encodeStruct({{interface.name}}_{{method.name}}_Params, params);
-    var message = builder.finish();
-    this.receiver_.acceptWithResponder(message, { accept: function(message) {
-      var reader = new codec.MessageReader(message);
-      var responseParams =
-         
reader.decodeStruct({{interface.name}}_{{method.name}}_ResponseParams);
-      closure(
-{%- for parameter in method.response_parameters -%}
-responseParams.{{parameter.name}}{% if not loop.last %}, {% endif %}
-{%- endfor -%});
-    }});
+    return new Promise(function(resolve, reject) {
+      var builder = new codec.MessageWithRequestIDBuilder(
+          k{{interface.name}}_{{method.name}}_Name,
+          codec.align({{interface.name}}_{{method.name}}_Params.encodedSize),
+          codec.kMessageExpectsResponse, 0);
+      builder.encodeStruct({{interface.name}}_{{method.name}}_Params, params);
+      var message = builder.finish();
+      this.receiver_.acceptWithResponder(message, { accept: function(message) {
+        var reader = new codec.MessageReader(message);
+        var responseParams =
+           
reader.decodeStruct({{interface.name}}_{{method.name}}_ResponseParams);
+        resolve(responseParams);
+      }});
+    }.bind(this));
 {%- endif %}
   };
 {%- endfor %}
@@ -79,15 +75,12 @@ params.{{parameter.name}}{% if not loop.last %}, {% endif %}
       var params =
reader.decodeStruct({{interface.name}}_{{method.name}}_Params);
       this.{{method.name|stylize_method}}(
 {%- for parameter in method.parameters -%}
-params.{{parameter.name}},
-{%- endfor %} function (
-{%- for parameter in method.response_parameters -%}
-{{parameter.name}}{% if not loop.last %}, {% endif -%}
-{%- endfor -%}) {
+params.{{parameter.name}}{% if not loop.last %}, {% endif -%}
+{%- endfor %}).then(function(response) {
         var responseParams =
             new {{interface.name}}_{{method.name}}_ResponseParams();
 {%-     for parameter in method.response_parameters %}
-        responseParams.{{parameter.name}} = {{parameter.name}};
+        responseParams.{{parameter.name}} = response.{{parameter.name}};
 {%-     endfor %}
         var builder = new codec.MessageWithRequestIDBuilder(
             k{{interface.name}}_{{method.name}}_Name,

[Matt Perry, 2014-04-04 21:27:00]

The CQ bit was checked by mpcomplete@chromium.org

[Matt Perry, 2014-04-04 21:27:01]

The CQ bit was unchecked by mpcomplete@chromium.org

[Matt Perry, 2014-04-04 21:27:02]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-04-04 21:39:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/223043006/100001

[commit-bot: I haz the power, 2014-04-04 22:06:22]

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

[commit-bot: I haz the power, 2014-04-04 22:06:22]

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

[Matt Perry, 2014-04-04 22:36:49]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-04-04 22:37:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpcomplete@chromium.org/223043006/100001

[commit-bot: I haz the power, 2014-04-04 22:56:56]

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

[commit-bot: I haz the power, 2014-04-04 22:56:57]

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

[Matt Perry, 2014-04-05 00:37:34]

Committed patchset #6 manually as r261936 (presubmit successful).