Change mojo JS bindings to expose a handle object, which is Closed when garbage

Change mojo JS bindings to expose a handle object, which is Closed when garbage
collected, rather than a raw integer.

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

[abarth-chromium, 2014-03-29 01:56:17]

LGTM

https://codereview.chromium.org/214183003/diff/1/gin/test/gtest.cc
File gin/test/gtest.cc (right):

https://codereview.chromium.org/214183003/diff/1/gin/test/gtest.cc#newcode56
gin/test/gtest.cc:56: .SetMethod("collectGarbage",
v8::V8::LowMemoryNotification)
Did you mean to expose this function in two places?  I think the GC module is a
better approach.

https://codereview.chromium.org/214183003/diff/1/mojo/bindings/js/core.cc
File mojo/bindings/js/core.cc (right):

https://codereview.chromium.org/214183003/diff/1/mojo/bindings/js/core.cc#new...
mojo/bindings/js/core.cc:82: std::vector<MojoHandle> raw_handles;
Do you want to resize the vector to the final size so that there's only one
malloc?

https://codereview.chromium.org/214183003/diff/1/mojo/bindings/js/core.cc#new...
mojo/bindings/js/core.cc:128: std::vector<gin::Handle<HandleWrapper> > handles;
Ditto.

https://codereview.chromium.org/214183003/diff/1/mojo/bindings/js/core.cc#new...
mojo/bindings/js/core.cc:229: // object?
Yeah, at some point we should probably make these APIs more idiomatic for JS.

[Matt Perry, 2014-03-31 19:34:17]

https://codereview.chromium.org/214183003/diff/1/gin/test/gtest.cc
File gin/test/gtest.cc (right):

https://codereview.chromium.org/214183003/diff/1/gin/test/gtest.cc#newcode56
gin/test/gtest.cc:56: .SetMethod("collectGarbage",
v8::V8::LowMemoryNotification)
On 2014/03/29 01:56:17, abarth wrote:
> Did you mean to expose this function in two places?  I think the GC module is
a
> better approach.

Oops, thanks. I put it here first as a hack. Removed.

https://codereview.chromium.org/214183003/diff/1/mojo/bindings/js/core.cc
File mojo/bindings/js/core.cc (right):

https://codereview.chromium.org/214183003/diff/1/mojo/bindings/js/core.cc#new...
mojo/bindings/js/core.cc:82: std::vector<MojoHandle> raw_handles;
On 2014/03/29 01:56:17, abarth wrote:
> Do you want to resize the vector to the final size so that there's only one
> malloc?

Done.

https://codereview.chromium.org/214183003/diff/1/mojo/bindings/js/core.cc#new...
mojo/bindings/js/core.cc:128: std::vector<gin::Handle<HandleWrapper> > handles;
On 2014/03/29 01:56:17, abarth wrote:
> Ditto.

Done.

[Matt Perry, 2014-03-31 19:34:20]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-03-31 19:35:05]

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

[Matt Perry, 2014-03-31 20:03:00]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:16:22]

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

[commit-bot: I haz the power, 2014-03-31 20:17:18]

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

[Matt Perry, 2014-03-31 20:26:55]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-03-31 20:45:49]

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

[commit-bot: I haz the power, 2014-03-31 20:45:51]

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

[commit-bot: I haz the power, 2014-03-31 21:20:29]

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

[commit-bot: I haz the power, 2014-03-31 21:20:29]

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

[Matt Perry, 2014-03-31 23:21:06]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-03-31 23:23:19]

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

[commit-bot: I haz the power, 2014-04-01 00:47:45]

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

[commit-bot: I haz the power, 2014-04-01 00:47:46]

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

[Matt Perry, 2014-04-01 20:16:36]

I had to make some changes to fix a failure in the WebUI.EndToEnd test:
- The handle that we pass to the WebUI's "main" function is now converted to a
HandleWrapper. Ditto for MojoRunnerDelegate.
- Moved HandleWrapper to js/handle.h and changed the Converter template to use
it.

[Matt Perry, 2014-04-01 23:45:15]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-04-01 23:45:43]

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

[commit-bot: I haz the power, 2014-04-02 00:17:12]

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

[commit-bot: I haz the power, 2014-04-02 00:17:13]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Matt Perry, 2014-04-02 00:23:40]

+darin for content/renderer/web_ui_mojo_context_state.cc OWNERS.

[darin (slow to review), 2014-04-02 05:32:55]

LGTM


On Tue, Apr 1, 2014 at 5:23 PM, <mpcomplete@chromium.org> wrote:

> +darin for content/renderer/web_ui_mojo_context_state.cc OWNERS.
>
> https://codereview.chromium.org/214183003/
>

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-04-02 05:33:16]

LGTM

[Matt Perry, 2014-04-02 17:24:57]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-04-02 17:25:32]

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

[Matt Perry, 2014-04-02 22:03:12]

The CQ bit was checked by mpcomplete@chromium.org

[commit-bot: I haz the power, 2014-04-02 22:03:58]

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

[Matt Perry, 2014-04-02 22:04:48]

Two more small changes here:
- WriteMessage now invalidates any handles passed across the pipe, so we don't
attempt to Close them later.
- core.close() returns INVALID_ARGUMENT if called on a previously-closed handle.

These changes fix some crashes I saw in connection_unittests.js.

[Matt Perry, 2014-04-02 22:13:55]

The CQ bit was checked by mpcomplete@chromium.org

[abarth-chromium, 2014-04-02 22:57:55]

https://codereview.chromium.org/214183003/diff/140001/mojo/bindings/js/core.cc
File mojo/bindings/js/core.cc (right):

https://codereview.chromium.org/214183003/diff/140001/mojo/bindings/js/core.c...
mojo/bindings/js/core.cc:77: }
Is there a better idiom for this in C++?  For example, maybe we should be
calling the C++ version of MojoWriteMessage, which might do this work for us.

[Matt Perry, 2014-04-02 23:01:25]

https://codereview.chromium.org/214183003/diff/140001/mojo/bindings/js/core.cc
File mojo/bindings/js/core.cc (right):

https://codereview.chromium.org/214183003/diff/140001/mojo/bindings/js/core.c...
mojo/bindings/js/core.cc:77: }
On 2014/04/02 22:57:56, abarth wrote:
> Is there a better idiom for this in C++?  For example, maybe we should be
> calling the C++ version of MojoWriteMessage, which might do this work for us.

There's no C++ version that does this for us AFAICT. But there probably should
be.

[abarth-chromium, 2014-04-03 04:55:36]

On 2014/04/02 23:01:25, Matt Perry wrote:
> There's no C++ version that does this for us AFAICT. But there probably should
> be.

Okiedokes

[Sergey Berezin, 2014-04-03 15:46:34]

The CQ bit was unchecked by sergeyberezin@chromium.org

[Sergey Berezin, 2014-04-03 15:50:32]

The CQ bit was checked by sergeyberezin@chromium.org

[commit-bot: I haz the power, 2014-04-03 15:51:23]

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

[commit-bot: I haz the power, 2014-04-03 18:16:33]

Change committed as 261479

[Nico, 2014-05-05 16:01:43]

https://codereview.chromium.org/214183003/diff/140001/mojo/bindings/js/core.cc
File mojo/bindings/js/core.cc (right):

https://codereview.chromium.org/214183003/diff/140001/mojo/bindings/js/core.c...
mojo/bindings/js/core.cc:76: mojo::Handle _ MOJO_ALLOW_UNUSED =
handles[i]->release();
It's better to write this `ignore_result(handles[i]->release());`.