Add the JS data pipe client to be used to implement serial send.

Add the JS data pipe client to be used to implement serial send.

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

[raymes, 2014-08-20 03:34:48]

Haven't reviewed the test yet. Comments are great! :)

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:24: * The data to be sent.
nit: Could just add a note that this is the *remaining* data to be sent whereas
sendLength is the *total* length of the data to be sent.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:34: this.sendLength_ =
data.byteLength;
nit, optional: could just call it "length_"

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:109: if (result.bytesLeft > 0)
A comment here noting that having more bytes left to send implies that no error
occurred in this PendingSend might be helpful for a reader

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:110: return
this.reportBytesSent(result.bytesLeft);
Again, I think it's confusing to call reportBytesInternal twice in this case.
Can we avoid this?

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:271: throw new Error('System
error');
Maybe something more descriptive? :)

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:314: if (result != core.RESULT_OK)
{
It seems like it could be good to handle this error right at the start of this
function (more for clarity than anything).

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:324: return;
unnecessary return

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:327: return;
unnecessary return

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:359: if (this.pendingSends_.length
+ this.sendsAwaitingAck_.length == 0)
Maybe a note on why we can call the cancel callback here.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:378: while
(this.pendingSends_.length) {
optionally make a note here that only the first pending send has data to flush.
The code is a bit weird because it could make the reader think that that isn't
true. But I still think it's clearer the way you have it.

[Sam McNally, 2014-08-20 04:19:55]

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:24: * The data to be sent.
On 2014/08/20 03:34:48, raymes wrote:
> nit: Could just add a note that this is the *remaining* data to be sent
whereas
> sendLength is the *total* length of the data to be sent.

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:34: this.sendLength_ =
data.byteLength;
On 2014/08/20 03:34:48, raymes wrote:
> nit, optional: could just call it "length_"

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:109: if (result.bytesLeft > 0)
On 2014/08/20 03:34:48, raymes wrote:
> A comment here noting that having more bytes left to send implies that no
error
> occurred in this PendingSend might be helpful for a reader

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:110: return
this.reportBytesSent(result.bytesLeft);
On 2014/08/20 03:34:48, raymes wrote:
> Again, I think it's confusing to call reportBytesInternal twice in this case.
> Can we avoid this?

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:271: throw new Error('System
error');
On 2014/08/20 03:34:48, raymes wrote:
> Maybe something more descriptive? :)

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:314: if (result != core.RESULT_OK)
{
On 2014/08/20 03:34:48, raymes wrote:
> It seems like it could be good to handle this error right at the start of this
> function (more for clarity than anything).

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:324: return;
On 2014/08/20 03:34:48, raymes wrote:
> unnecessary return

One of either return or break is required.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:327: return;
On 2014/08/20 03:34:48, raymes wrote:
> unnecessary return

This one is unnecessary because close() clears pendingSends_, but that feels a
bit too magic.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:359: if (this.pendingSends_.length
+ this.sendsAwaitingAck_.length == 0)
On 2014/08/20 03:34:48, raymes wrote:
> Maybe a note on why we can call the cancel callback here.

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:378: while
(this.pendingSends_.length) {
On 2014/08/20 03:34:48, raymes wrote:
> optionally make a note here that only the first pending send has data to
flush.
> The code is a bit weird because it could make the reader think that that isn't
> true. But I still think it's clearer the way you have it.

Done.

[raymes, 2014-08-20 04:23:14]

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/api/seri...
File extensions/renderer/api/serial/data_sender_unittest.cc (right):

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/api/seri...
extensions/renderer/api/serial/data_sender_unittest.cc:64:
buffer->Done(buffer->GetSize());
I guess we haven't tested the case where an error occurs part way through a
large send. But maybe that's not an interesting case, I'll let you decide.

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
File extensions/test/data/data_sender_unittest.js (right):

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
extensions/test/data/data_sender_unittest.js:34: function
testSend(serviceProvider, dataSender, mojom) {
Couldn't we define all of the real test bodies in a list (e.g.

var tests = [
  function testSend(....) {
  },

  function testLargeSend(...) {
  },
  ...
];


And then just have some snippet at the end which wraps them up using
wrapAsyncTest? I'm guessing this is something we probably want to pull out into
a utility.

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
extensions/test/data/data_sender_unittest.js:36:
serviceProvider.connectToService(mojom.DataSinkProxy.NAME_), 10, 2);
use constants for construction. If it's easier, pull this line out into a small
function: createDataSender()

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
extensions/test/data/data_sender_unittest.js:59: test.assertEq(100, bytesSent);
As before consider using a multiple of the buffer size. Also below.

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
extensions/test/data/data_sender_unittest.js:108: sender.send(generateData(2,
'b')).catch(test.callbackPass(function(e) {
It's easy to forget why this is expected to fail. Maybe make a note here that
after an error all sends in progress will be cancelled.

[raymes, 2014-08-20 04:29:41]

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/data_sender.js (right):

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/data_sender.js:327: return;
Ahh I missed the fact it is in a loop.

[Sam McNally, 2014-08-20 04:50:16]

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/api/seri...
File extensions/renderer/api/serial/data_sender_unittest.cc (right):

https://codereview.chromium.org/488003002/diff/1/extensions/renderer/api/seri...
extensions/renderer/api/serial/data_sender_unittest.cc:64:
buffer->Done(buffer->GetSize());
On 2014/08/20 04:23:13, raymes wrote:
> I guess we haven't tested the case where an error occurs part way through a
> large send. But maybe that's not an interesting case, I'll let you decide.

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
File extensions/test/data/data_sender_unittest.js (right):

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
extensions/test/data/data_sender_unittest.js:34: function
testSend(serviceProvider, dataSender, mojom) {
On 2014/08/20 04:23:13, raymes wrote:
> Couldn't we define all of the real test bodies in a list (e.g.
> 
> var tests = [
>   function testSend(....) {
>   },
> 
>   function testLargeSend(...) {
>   },
>   ...
> ];
> 
> 
> And then just have some snippet at the end which wraps them up using
> wrapAsyncTest? I'm guessing this is something we probably want to pull out
into
> a utility.

exportTests uses the function names to decide the names to export the test
functions as and I couldn't find a way to generate a wrapper with the same name
as the wrapped function.

Changed to match the style of the receiver unit tests.

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
extensions/test/data/data_sender_unittest.js:36:
serviceProvider.connectToService(mojom.DataSinkProxy.NAME_), 10, 2);
On 2014/08/20 04:23:13, raymes wrote:
> use constants for construction. If it's easier, pull this line out into a
small
> function: createDataSender()

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
extensions/test/data/data_sender_unittest.js:59: test.assertEq(100, bytesSent);
On 2014/08/20 04:23:13, raymes wrote:
> As before consider using a multiple of the buffer size. Also below.

Done.

https://codereview.chromium.org/488003002/diff/1/extensions/test/data/data_se...
extensions/test/data/data_sender_unittest.js:108: sender.send(generateData(2,
'b')).catch(test.callbackPass(function(e) {
On 2014/08/20 04:23:13, raymes wrote:
> It's easy to forget why this is expected to fail. Maybe make a note here that
> after an error all sends in progress will be cancelled.

Done.

[raymes, 2014-08-20 05:09:52]

> 
> exportTests uses the function names to decide the names to export the test
> functions as and I couldn't find a way to generate a wrapper with the same
name
> as the wrapped function.
> 
> Changed to match the style of the receiver unit tests.

Ah I see - that's frustrating. I think it's ok for now, but I think that going
forward we should think about making this nicer. For example exportTests could
take a list of modules to require asynchronously before each test.

lgtm

[benwells, 2014-08-20 06:13:47]

Overall lg, but I want to see if my understanding of how it works is correct
based on the answers to my questions.

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

https://codereview.chromium.org/488003002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:40: this.bytesSent_ = 0;
This name is confusing. It isn't sent bytes, it is bytes that have been received
at the other end. To me this is bytes sent - bytes in transit.

Can you rename it to something communicating that?

https://codereview.chromium.org/488003002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:134: var result = {bytesLeft: 0,
bytesToFlush: 0};
Nit: bytesToFlush isn't used in the normal (non-error) flow at all. Consider
removing it from this object and adding it in reportBytesSentAndError.

https://codereview.chromium.org/488003002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:352: numBytes = result.bytesLeft;
*Now* I understand bytesLeft. In the context of PendingSend it is confusing - I
thought it was bytes left to be sent by that PendingSend. Maybe bytesUnreported
would be a better name.

Anyway - up to you.

https://codereview.chromium.org/488003002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:387: // Only the first PendingSend
in |pendingSends_| will have data to flush
Nit: this comment is informative but doesn't seem to relate to the code around
it - the first pendingSend isn't treated specially as far as I can see. Maybe
preface it with 'Note:' so people trying to understand the code won't get
confused but it?

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

https://codereview.chromium.org/488003002/diff/60001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:58: function testLargeSend() {
Could you also add a test which sends heterogenous data > buffer size? E.g.
"abcdefghijklmnopqrstuvwxyz".

[Sam McNally, 2014-08-20 06:30:57]

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

https://codereview.chromium.org/488003002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:40: this.bytesSent_ = 0;
On 2014/08/20 06:13:47, benwells wrote:
> This name is confusing. It isn't sent bytes, it is bytes that have been
received
> at the other end. To me this is bytes sent - bytes in transit.
> 
> Can you rename it to something communicating that?

Done.

https://codereview.chromium.org/488003002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:134: var result = {bytesLeft: 0,
bytesToFlush: 0};
On 2014/08/20 06:13:47, benwells wrote:
> Nit: bytesToFlush isn't used in the normal (non-error) flow at all. Consider
> removing it from this object and adding it in reportBytesSentAndError.

Done.

https://codereview.chromium.org/488003002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:352: numBytes = result.bytesLeft;
On 2014/08/20 06:13:47, benwells wrote:
> *Now* I understand bytesLeft. In the context of PendingSend it is confusing -
I
> thought it was bytes left to be sent by that PendingSend. Maybe
bytesUnreported
> would be a better name.
> 
> Anyway - up to you.

Done.

https://codereview.chromium.org/488003002/diff/60001/extensions/renderer/reso...
extensions/renderer/resources/data_sender.js:387: // Only the first PendingSend
in |pendingSends_| will have data to flush
On 2014/08/20 06:13:47, benwells wrote:
> Nit: this comment is informative but doesn't seem to relate to the code around
> it - the first pendingSend isn't treated specially as far as I can see. Maybe
> preface it with 'Note:' so people trying to understand the code won't get
> confused but it?

Done.

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

https://codereview.chromium.org/488003002/diff/60001/extensions/test/data/dat...
extensions/test/data/data_sender_unittest.js:58: function testLargeSend() {
On 2014/08/20 06:13:47, benwells wrote:
> Could you also add a test which sends heterogenous data > buffer size? E.g.
> "abcdefghijklmnopqrstuvwxyz".

Done.

[benwells, 2014-08-20 07:04:54]

lgtm

[Sam McNally, 2014-08-20 07:05:28]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-08-20 07:06:18]

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

[commit-bot: I haz the power, 2014-08-20 08:05:17]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 09:41:56]

Committed patchset #4 (80001) as 290797