Add serialization support to the JS DataSender and DataReceiver.

Add serialization support to the JS DataSender and DataReceiver.

To support persistent serial connections on mojo, serial.Connection and
its dependencies must be serializable to mojo structs so it can be
stashed. This adds serialization support to the two serial.Connection
dependencies: DataSender and DataReceiver.

BUG=389016
Committed: https://crrev.com/10fd1075c2bbe78be3c0603ff2d9f3fa69646454
Cr-Commit-Position: refs/heads/master@{#296658}

[Sam McNally, 2014-09-16 08:03:55]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2014-09-17 00:39:29]

sammc@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2014-09-17 02:05:04]

some initial thoughts :)

https://codereview.chromium.org/571333002/diff/20001/device/serial/data_strea...
File device/serial/data_stream.mojom (right):

https://codereview.chromium.org/571333002/diff/20001/device/serial/data_strea...
device/serial/data_stream.mojom:46: struct SerializedDataSender {
As noted I think it would be good to move these to a separate file because they
are more of an implementation detail of the client. 

Please also add some comments about what they are for.

https://codereview.chromium.org/571333002/diff/20001/extensions/renderer/reso...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/20001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:195:
DataReceiver.prototype.serialize = function() {
Please add comments to these :)

https://codereview.chromium.org/571333002/diff/20001/extensions/test/data/dat...
File extensions/test/data/data_receiver_unittest.js (right):

https://codereview.chromium.org/571333002/diff/20001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:109: },
I find the style that these tests are written in very hard to understand and I
have to stare at it for a long time before I see roughly what is going on. It is
very dense. I'm not sure if it is just me - so if you think it would be easy to
understand for other devs, please CC them for their opinion :)

Rather than having everything inline inside of the test, could we pull out all
the functions definitions to the top of the test and name them. Then below that
we can have the execution chained together with "then". I think this could make
it much clearer to read with fewer brackets and less inline nesting. Could we
try something like that?

[Sam McNally, 2014-09-17 08:07:14]

https://codereview.chromium.org/571333002/diff/20001/device/serial/data_strea...
File device/serial/data_stream.mojom (right):

https://codereview.chromium.org/571333002/diff/20001/device/serial/data_strea...
device/serial/data_stream.mojom:46: struct SerializedDataSender {
On 2014/09/17 02:05:04, raymes wrote:
> As noted I think it would be good to move these to a separate file because
they
> are more of an implementation detail of the client. 
> 
> Please also add some comments about what they are for.

Done.

https://codereview.chromium.org/571333002/diff/20001/extensions/renderer/reso...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/20001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:195:
DataReceiver.prototype.serialize = function() {
On 2014/09/17 02:05:04, raymes wrote:
> Please add comments to these :)

Done.

https://codereview.chromium.org/571333002/diff/20001/extensions/test/data/dat...
File extensions/test/data/data_receiver_unittest.js (right):

https://codereview.chromium.org/571333002/diff/20001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:109: },
On 2014/09/17 02:05:04, raymes wrote:
> I find the style that these tests are written in very hard to understand and I
> have to stare at it for a long time before I see roughly what is going on. It
is
> very dense. I'm not sure if it is just me - so if you think it would be easy
to
> understand for other devs, please CC them for their opinion :)
> 
> Rather than having everything inline inside of the test, could we pull out all
> the functions definitions to the top of the test and name them. Then below
that
> we can have the execution chained together with "then". I think this could
make
> it much clearer to read with fewer brackets and less inline nesting. Could we
> try something like that?

Done.

[raymes, 2014-09-18 03:16:08]

https://codereview.chromium.org/571333002/diff/40001/device/serial/data_strea...
File device/serial/data_stream_serialization.mojom (right):

https://codereview.chromium.org/571333002/diff/40001/device/serial/data_strea...
device/serial/data_stream_serialization.mojom:9: // A serialized form of a JS
DataSender.
does it have to be "js"? The C++ serialization would look the same wouldn't it?

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:198: * @return
{Promise.<?SerializedDataReceiver>} The serialization of this
Well really it returns a promise that resolves to that?

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:204: return
Promise.resolve(null);
How come you decided to return null in this case rather than serializing the
shutDown state as well?

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:266: * @return
{Promise.<?SerializedDataSender>} The serialization of this
returns a promise that resolves to

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:267: *     DataSender. If this
DataSender has shut down, this will return null.
It's probably worth noting that any sends in progress will be cancelled?

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:324: this.sendsAwaitingAck_ = [];
One downside of this approach (not calling the constructor) is that we might not
initialize something that we should. Is there a way to mitigate against that or
do you think it's a non-issue?

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:368: return this.cancelPromise_;
so it's safe to have multiple things call .then() on the same promise? That's
cool and a bit scary!

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
File extensions/test/data/data_receiver_unittest.js (right):

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:29: function
receiveAndCheckData(expectedData) {
It would be good to comment these functions and point out that they return
functions which do what they are named.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:56:
requireAsync('data_receiver'),
This seems weird to have here?

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:59: var dataReceiver =
promises[1];
this is really "dataReceiverModule", right?

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:60: return
promises[1].DataReceiver.deserialize(promises[0]);
I think you meant to use the named vars here.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
File extensions/test/data/data_sender_unittest.js (right):

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:40: function
sendAndExpectSuccess(data, expectedBytesSent) {
Please add comments for the helpers here too

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:91: var dataSender = promises[1];
dataSenderModule

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:92: return
promises[1].DataSender.deserialize(promises[0]);
same here, use the variable names :)

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:113: BUFFER_SIZE * 3))
I think the indentation should be 2 spaces less here.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:148:
.then(closeSender).then(test.succeed, test.fail);
I think the indentation is wrong here - it looks different to the above.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:196:
sender.then(closeSender).then(test.succeed, test.fail)
I like having every .then on a separate line :)

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:197: .then(function(sender) {
Won't this run after the test has passed or failed =\

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:211:
]).then(closeSender).then(test.succeed, test.fail);
.then on a new line (and lots below)

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:234: function
testSendErrorBetweenPacketsWithSerialization() {
Do this test and the next one test something substantially different from the
other tests? Is there particular state we expect to be stored in these cases
that isn't tested in the other cases? It seems to me they might be redundant.

If it is testing something different, it would be good to spell out what the
test is doing a bit more.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:267: function
testCancelWithSerialization() {
does this test actually check anything?

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:284: function
testSerializeWithSendsInProgress() {
does this test actually check anything?

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:298:
sender.then(serializeRoundTrip),
does this test actually check anything?

[Sam McNally, 2014-09-19 04:50:12]

Patchset #3 (id:60001) has been deleted

[Sam McNally, 2014-09-19 04:58:48]

https://codereview.chromium.org/571333002/diff/40001/device/serial/data_strea...
File device/serial/data_stream_serialization.mojom (right):

https://codereview.chromium.org/571333002/diff/40001/device/serial/data_strea...
device/serial/data_stream_serialization.mojom:9: // A serialized form of a JS
DataSender.
On 2014/09/18 03:16:06, raymes wrote:
> does it have to be "js"? The C++ serialization would look the same wouldn't
it?

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:198: * @return
{Promise.<?SerializedDataReceiver>} The serialization of this
On 2014/09/18 03:16:06, raymes wrote:
> Well really it returns a promise that resolves to that?

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:204: return
Promise.resolve(null);
On 2014/09/18 03:16:06, raymes wrote:
> How come you decided to return null in this case rather than serializing the
> shutDown state as well?

None of the other state is useful after shutdown.

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:266: * @return
{Promise.<?SerializedDataSender>} The serialization of this
On 2014/09/18 03:16:06, raymes wrote:
> returns a promise that resolves to

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:267: *     DataSender. If this
DataSender has shut down, this will return null.
On 2014/09/18 03:16:06, raymes wrote:
> It's probably worth noting that any sends in progress will be cancelled? 

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:324: this.sendsAwaitingAck_ = [];
On 2014/09/18 03:16:06, raymes wrote:
> One downside of this approach (not calling the constructor) is that we might
not
> initialize something that we should. Is there a way to mitigate against that
or
> do you think it's a non-issue?

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:368: return this.cancelPromise_;
On 2014/09/18 03:16:06, raymes wrote:
> so it's safe to have multiple things call .then() on the same promise? That's
> cool and a bit scary!

Yes!

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
File extensions/test/data/data_receiver_unittest.js (right):

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:29: function
receiveAndCheckData(expectedData) {
On 2014/09/18 03:16:06, raymes wrote:
> It would be good to comment these functions and point out that they return
> functions which do what they are named.

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:56:
requireAsync('data_receiver'),
On 2014/09/18 03:16:06, raymes wrote:
> This seems weird to have here? 

createReceiver does the same.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:59: var dataReceiver =
promises[1];
On 2014/09/18 03:16:07, raymes wrote:
> this is really "dataReceiverModule", right?

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_receiver_unittest.js:60: return
promises[1].DataReceiver.deserialize(promises[0]);
On 2014/09/18 03:16:07, raymes wrote:
> I think you meant to use the named vars here.

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
File extensions/test/data/data_sender_unittest.js (right):

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:40: function
sendAndExpectSuccess(data, expectedBytesSent) {
On 2014/09/18 03:16:07, raymes wrote:
> Please add comments for the helpers here too

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:91: var dataSender = promises[1];
On 2014/09/18 03:16:07, raymes wrote:
> dataSenderModule

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:92: return
promises[1].DataSender.deserialize(promises[0]);
On 2014/09/18 03:16:07, raymes wrote:
> same here, use the variable names :)

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:113: BUFFER_SIZE * 3))
On 2014/09/18 03:16:07, raymes wrote:
> I think the indentation should be 2 spaces less here.

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:148:
.then(closeSender).then(test.succeed, test.fail);
On 2014/09/18 03:16:07, raymes wrote:
> I think the indentation is wrong here - it looks different to the above.

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:196:
sender.then(closeSender).then(test.succeed, test.fail)
On 2014/09/18 03:16:07, raymes wrote:
> I like having every .then on a separate line :)

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:197: .then(function(sender) {
On 2014/09/18 03:16:07, raymes wrote:
> Won't this run after the test has passed or failed =\

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:211:
]).then(closeSender).then(test.succeed, test.fail);
On 2014/09/18 03:16:07, raymes wrote:
> .then on a new line (and lots below)

Done.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:234: function
testSendErrorBetweenPacketsWithSerialization() {
On 2014/09/18 03:16:07, raymes wrote:
> Do this test and the next one test something substantially different from the
> other tests? Is there particular state we expect to be stored in these cases
> that isn't tested in the other cases? It seems to me they might be redundant.
> 
> If it is testing something different, it would be good to spell out what the
> test is doing a bit more. 

Removed these.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:267: function
testCancelWithSerialization() {
On 2014/09/18 03:16:07, raymes wrote:
> does this test actually check anything?

Just that cancel still works the same after serialization.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:284: function
testSerializeWithSendsInProgress() {
On 2014/09/18 03:16:07, raymes wrote:
> does this test actually check anything?

Yes. This checks that serialize() cancels any sends in progress. Renamed to be
clearer.

https://codereview.chromium.org/571333002/diff/40001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:298:
sender.then(serializeRoundTrip),
On 2014/09/18 03:16:07, raymes wrote:
> does this test actually check anything?

Yes. This checks that serialize() doesn't override the cancel already in
progress. Renamed to be clearer.

[raymes, 2014-09-23 03:20:57]

lgtm with nit

https://codereview.chromium.org/571333002/diff/80001/extensions/renderer/reso...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:144: source, dataPipe,
fatalErrorValue, bytesReceived, pendingError, paused) {
Is this valid JS style? In C++ we would put 1 arg per line

[Sam McNally, 2014-09-23 03:47:23]

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

[Sam McNally, 2014-09-23 03:47:24]

+rockot

https://codereview.chromium.org/571333002/diff/80001/extensions/renderer/reso...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:144: source, dataPipe,
fatalErrorValue, bytesReceived, pendingError, paused) {
On 2014/09/23 03:20:57, raymes wrote:
> Is this valid JS style? In C++ we would put 1 arg per line

Not sure - clang format is happy with it, but I think one per line is nicer
here.

[Ken Rockot(use gerrit already), 2014-09-24 16:08:02]

lgtm but please take another pass over the jsdoc and make sure you're expressing
correct nullability constraints and avoiding redundant prefixes.

Number, string, boolean, and function types are implicitly non-nullable. To make
them nullable the '?' prefix is required. ('!' is always redundant on these
types.)

Anything else is implicitly nullable and requires the '!' to make it
non-nullable. ('?' is always redundant on these types.)

https://codereview.chromium.org/571333002/diff/80001/extensions/renderer/reso...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/data_receiver.js:144: source, dataPipe,
fatalErrorValue, bytesReceived, pendingError, paused) {
On 2014/09/23 03:47:24, Sam McNally wrote:
> On 2014/09/23 03:20:57, raymes wrote:
> > Is this valid JS style? In C++ we would put 1 arg per line
> 
> Not sure - clang format is happy with it, but I think one per line is nicer
> here.

Either one is fine for Google JS style. See the first example under "Function
Arguments" in this section:

https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#Code_f...

https://codereview.chromium.org/571333002/diff/100001/extensions/renderer/res...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/100001/extensions/renderer/res...
extensions/renderer/resources/data_receiver.js:216: * @return
{Promise.<?SerializedDataReceiver>} A promise that will resolve to
Assuming we follow the Google style guide here (and sorry if I've already missed
this in other places) the proper notation for this return type should be

{!Promise.<SerializedDataReceiver>}

Both Promise and SerializedDataReceiver are implicitly nullable, so the '?' is
redundant and the '!' signifies that you always return a Promise object.

[Sam McNally, 2014-09-25 05:01:40]

On 2014/09/24 16:08:02, Ken Rockot (slow this week) wrote:
> lgtm but please take another pass over the jsdoc and make sure you're
expressing
> correct nullability constraints and avoiding redundant prefixes.
> 
> Number, string, boolean, and function types are implicitly non-nullable. To
make
> them nullable the '?' prefix is required. ('!' is always redundant on these
> types.)
> 
> Anything else is implicitly nullable and requires the '!' to make it
> non-nullable. ('?' is always redundant on these types.)
> 
Done. Thanks for the explanation.

https://codereview.chromium.org/571333002/diff/100001/extensions/renderer/res...
File extensions/renderer/resources/data_receiver.js (right):

https://codereview.chromium.org/571333002/diff/100001/extensions/renderer/res...
extensions/renderer/resources/data_receiver.js:216: * @return
{Promise.<?SerializedDataReceiver>} A promise that will resolve to
On 2014/09/24 16:08:02, Ken Rockot (slow this week) wrote:
> Assuming we follow the Google style guide here (and sorry if I've already
missed
> this in other places) the proper notation for this return type should be
> 
> {!Promise.<SerializedDataReceiver>}
> 
> Both Promise and SerializedDataReceiver are implicitly nullable, so the '?' is
> redundant and the '!' signifies that you always return a Promise object.

Done.

[Sam McNally, 2014-09-25 05:02:18]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-09-25 05:03:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/571333002/120001

[commit-bot: I haz the power, 2014-09-25 05:48:56]

Committed patchset #5 (id:120001) as e9e907150c387b80115f6d31204cf45171986bf0

[commit-bot: I haz the power, 2014-09-25 05:49:55]

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