Implement Mojo message codec in JavaScript

Implement Mojo message codec in JavaScript

This CL contains an encoder and decoder for the Mojo message IPC format.  I've
tested the encoder and decoder against each other, but I haven't yet tested them
against the C++ implementation.

I haven't tried to tune the encoder or decoder for performance. IMHO, we should
work on that after getting correctness.

The C++ code for JSTest.codec is a bit verbose because the C++ code need to
load all the dependencies for codec_unittests.js. A better approach is to teach
the system how to load dependencies from the file system, but I've left that for
a future CL.

BUG=317398
R=aa@chromium.org

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

[abarth-chromium, 2013-11-12 02:10:20]

Here's an early work-in-progress of the bindings serialization library in
JavaScript.  I'd appreciate early feedback about the approach, but there's no
need to review in detail at this time.

(I also think I've got then endianness backwards, but I'm waiting for a test to
tell me that.)

[abarth-chromium, 2013-11-12 23:56:51]

I've uploaded a new work-in-progress that gets all the way up to encoding and
decoding the Foo and Bar structs from sample_service.

There are some problem spots involving allocating new records during encoding
and encoding handles, but things mostly seem to be shaping up.

[Aaron Boodman, 2013-11-13 19:32:36]

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
File mojo/public/bindings/js/codec.js (right):

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:7: function store8(memory, ptr, val) {
What is intended to be the public API out of all these identifiers?The rest of
them should be hidden in a closure.

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:63: return size + (kAlignment - (size %
kAlignment)) % kAlignment;
I think the "% kAlignment" at the end is unnecessary?

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:68: function encodePointer(memory, ptr,
offsetPtr) {
Weird to use both 'ptr' and 'pointer'. Google style would be to use 'pointer'
everywhere.

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:124: // buffer using v8::String::NewFromUtf8.
There are javascript implementations of utf8! I wrote one once!

[abarth-chromium, 2013-11-13 19:41:51]

Thanks for taking a look.

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
File mojo/public/bindings/js/codec.js (right):

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:7: function store8(memory, ptr, val) {
On 2013/11/13 19:32:36, Aaron Boodman wrote:
> What is intended to be the public API out of all these identifiers?The rest of
> them should be hidden in a closure.

Definitely!  I was hoping we'd have a module system by the time I got this
working so I could just use that to control visibility.

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:63: return size + (kAlignment - (size %
kAlignment)) % kAlignment;
On 2013/11/13 19:32:36, Aaron Boodman wrote:
> I think the "% kAlignment" at the end is unnecessary?

I haven't found any callers for this function, so I'm just going to delete it
until needed.

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:68: function encodePointer(memory, ptr,
offsetPtr) {
On 2013/11/13 19:32:36, Aaron Boodman wrote:
> Weird to use both 'ptr' and 'pointer'. Google style would be to use 'pointer'
> everywhere.

Ok.  I was thinking about switching to addr, but we can use pointer.

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:124: // buffer using v8::String::NewFromUtf8.
On 2013/11/13 19:32:36, Aaron Boodman wrote:
> There are javascript implementations of utf8! I wrote one once!

Link?  :)

I'd like to start with a pure JS implementation and optimize as indicated.

[abarth-chromium, 2013-11-15 02:38:59]

I made a bunch of progress today and I've updated my work-in-progress patch. 
This CL is now dependent on https://codereview.chromium.org/62333018/ for a
module system.

I've gotten to the point of actually compiling and executing this code.  Now I
just need to make my tests pass.  :)

[abarth-chromium, 2013-11-15 19:25:13]

Ok, this CL is ready for review.  Unit testing found lots of bugs in codec.js,
which I've fixed.  We'll likely discover more bugs when we start testing interop
with the C++ implementation.

Please take a look!  Thanks.  :)

[abarth-chromium, 2013-11-16 16:12:08]

@Dave: You might be interested in reviewing this CL as well.

[abarth-chromium, 2013-11-17 20:07:05]

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
File mojo/public/bindings/js/test/run_js_tests.cc (right):

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
mojo/public/bindings/js/test/run_js_tests.cc:49: // teach the module loading
system how to find imported modules on disk.
This code will get much cleaner once https://codereview.chromium.org/74753002/
lands and I rebase over it.  However, I'm leaving this CL in its current state
so the two can land independently.

[Aaron Boodman, 2013-11-18 18:34:48]

lgtm

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
File mojo/public/bindings/js/codec.js (right):

https://codereview.chromium.org/69843003/diff/80001/mojo/public/bindings/js/c...
mojo/public/bindings/js/codec.js:124: // buffer using v8::String::NewFromUtf8.
On 2013/11/13 19:41:51, abarth wrote:
> On 2013/11/13 19:32:36, Aaron Boodman wrote:
> > There are javascript implementations of utf8! I wrote one once!
> 
> Link?  :)
> 
> I'd like to start with a pure JS implementation and optimize as indicated.

I believe that there is one I wrote internally as one of my first projects at
Google. I thought we open sourced it, but I can't find it. Will research
tomorrow when I'm in the office -- no vpn here.

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

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
mojo/public/bindings/js/codec.js:5: define("codec", function() {
According to the AMD web page, it is encouraged to leave the modules anonymous
and to let their URLs define them.

I was curious what your thoughts on this were and if you plan to change it
later, once URL loading is in place.

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
mojo/public/bindings/js/codec.js:27: var hi = (val / 0x10000) | 0;
high :)

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
mojo/public/bindings/js/codec.js:64: this.grow(1.5 * (this.memory.length +
size));
Make the rounding explicit here, just so that readers don't have to trace
through grow() to make sure fractional values work.

[abarth-chromium, 2013-11-18 18:57:30]

I've rebased this now that gin_shell has landed and it's much cleaner.

[abarth-chromium, 2013-11-18 19:11:03]

On 2013/11/18 18:57:30, abarth wrote:
> I've rebased this now that gin_shell has landed and it's much cleaner.

Sorry, I messed up the upload.  Will fix.  Looking at your comments now.

[abarth-chromium, 2013-11-18 19:13:19]

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

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
mojo/public/bindings/js/codec.js:5: define("codec", function() {
On 2013/11/18 18:34:48, Aaron Boodman wrote:
> According to the AMD web page, it is encouraged to leave the modules anonymous
> and to let their URLs define them.

Yep.

> I was curious what your thoughts on this were and if you plan to change it
> later, once URL loading is in place.

I've fixed this now that we can find the modules on disk.

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
mojo/public/bindings/js/codec.js:27: var hi = (val / 0x10000) | 0;
On 2013/11/18 18:34:48, Aaron Boodman wrote:
> high :)

Fixed.  :)

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
mojo/public/bindings/js/codec.js:50: return lo + hi * 0x10000;
Fixed here too.

https://codereview.chromium.org/69843003/diff/240001/mojo/public/bindings/js/...
mojo/public/bindings/js/codec.js:64: this.grow(1.5 * (this.memory.length +
size));
On 2013/11/18 18:34:48, Aaron Boodman wrote:
> Make the rounding explicit here, just so that readers don't have to trace
> through grow() to make sure fractional values work.

Done.

[abarth-chromium, 2013-11-18 19:16:58]

Fixed.  There are some more C++ changes in this CL if you'd like to take a look
at it again.  It's mostly deleting code that's now automated at the Gin layer.

[Aaron Boodman, 2013-11-18 20:15:20]

Oh, I did. Yay.

[Aaron Boodman, 2013-11-18 20:15:40]

lgtm - sorry I meant to lgtm and forget on last round.


On Mon, Nov 18, 2013 at 11:16 AM, <abarth@chromium.org> wrote:

> Fixed.  There are some more C++ changes in this CL if you'd like to take a
> look
> at it again.  It's mostly deleting code that's now automated at the Gin
> layer.
>
> https://codereview.chromium.org/69843003/
>

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

[commit-bot: I haz the power, 2013-11-18 21:12:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/69843003/410001

[commit-bot: I haz the power, 2013-11-18 22:20:27]

Retried try job too often on win_rel for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-18 22:49:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/abarth@chromium.org/69843003/410001

[abarth-chromium, 2013-11-18 23:05:17]

Committed patchset #10 manually as r235838 (presubmit successful).