Implement the client side of Serial I/O on data pipe.

Implement the client side of Serial I/O on data pipe.

This change implements chrome.serial.send and the
chrome.serial.onReceive(Error) events using DataSender and DataReceiver,
respectively. It also adds a TimeoutManager to allow control over when
callbacks queued by setTimeout are invoked to facilitate testing of send
and receive timeouts.

BUG=389016
Committed: https://crrev.com/93c5de27b7e35e8e9973645e75075743344d9b26
Cr-Commit-Position: refs/heads/master@{#293092}

[Sam McNally, 2014-08-27 07:12:30]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2014-08-27 07:34:31]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2014-08-27 07:34:41]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2014-08-27 07:37:20]

Patchset #1 (id:60001) has been deleted

[Sam McNally, 2014-08-27 07:43:43]

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

[raymes, 2014-08-29 06:07:16]

Could you add some documentation for each of the tests explaining in a bit more
detail what they test and also mentioning any interactions with the IO handlers.

Thanks!

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/api/...
File extensions/renderer/api/serial/serial_api_unittest.cc (right):

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/api/...
extensions/renderer/api/serial/serial_api_unittest.cc:341:
DCHECK_GE(pending_write_buffer_len(), 2u);
Why can you make this assumption? A comment would be helpful here.

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

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/serial_service.js:297: }
Using if/else here probably makes this slightly clearer (to say: if paused do
this otherwise do this)

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/serial_service.js:317: error:
SEND_ERROR_FROM_MOJO[e.error] || 'system_error',
In what case will it return 'system_error'?

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/serial_service.js:331: if (this.pendingReceive_) {
Some comments describing how pendingReceive_/pendingError_ is used only if we
receive some data while we are paused would be helpful for a reader.

More comments in general in this file would be helpful, sorry that's been the
theme of my feedback hasn't it! But I'll leave it to you at this point.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
File extensions/test/data/serial_unittest.js (right):

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:6: * Unit tests for the JS serial
service client.
A comment mentioning which CC file drives this would be good (and vice-versa)

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:38: function addReceiveHook(callback) {
Please add a comment for these functions. Saves people from having to dig into
what they actually do.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:331: function testEcho() {
This looks like it does more than a simple echo. The nesting of sends makes this
particularly difficult to read. Could you add comments about what the different
cases it tests are?

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:346: }));
It would be useful to comment about what the second send is for.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:366: }
maybe move this up above the send.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:371: function testPausedEcho() {
Change this to be more like testPausedReceiveError with a custom IO handler

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:378: serial.setPaused(connectionId,
false, test.callbackPass());
Why do we need to call this twice?

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:415: serial.setPaused(connectionId,
false, test.callbackPass());
If we've already tested this case, no need to test again.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:449: },
Consider removing this test as dicussed since it's not testing anything in
serial.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:466: test.assertEq('system_error',
sendInfo.error);
Move this above the send just for clarity.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:532: test.assertEq(12345,
timeoutManager.currentTime);
Maybe move these above the call to the timeout

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:590: runSendErrorTest('timeout');
make a note that this test should pause receiving but wouldn't in production?

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/uni...
File extensions/test/data/unit_test_environment_specific_bindings.js (right):

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/uni...
extensions/test/data/unit_test_environment_specific_bindings.js:49: function
TimeoutManager() {
Comment about what this does would be nice and what it allows.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/uni...
extensions/test/data/unit_test_environment_specific_bindings.js:99:
TimeoutManager.prototype.timeoutCreated = function() {
Maybe we can just have a function which sets the amount to advance after a
setTimeout is called

[Ken Rockot(use gerrit already), 2014-08-29 16:46:38]

Nice. LGTM pending action on raymes' comments

[Sam McNally, 2014-09-01 06:35:19]

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/api/...
File extensions/renderer/api/serial/serial_api_unittest.cc (right):

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/api/...
extensions/renderer/api/serial/serial_api_unittest.cc:341:
DCHECK_GE(pending_write_buffer_len(), 2u);
On 2014/08/29 06:07:15, raymes wrote:
> Why can you make this assumption? A comment would be helpful here.

Done.

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

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/serial_service.js:297: }
On 2014/08/29 06:07:16, raymes wrote:
> Using if/else here probably makes this slightly clearer (to say: if paused do
> this otherwise do this)

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/serial_service.js:317: error:
SEND_ERROR_FROM_MOJO[e.error] || 'system_error',
On 2014/08/29 06:07:15, raymes wrote:
> In what case will it return 'system_error'?

It shouldn't hit that case.

https://codereview.chromium.org/509813002/diff/80001/extensions/renderer/reso...
extensions/renderer/resources/serial_service.js:331: if (this.pendingReceive_) {
On 2014/08/29 06:07:15, raymes wrote:
> Some comments describing how pendingReceive_/pendingError_ is used only if we
> receive some data while we are paused would be helpful for a reader.
> 
> More comments in general in this file would be helpful, sorry that's been the
> theme of my feedback hasn't it! But I'll leave it to you at this point.

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
File extensions/test/data/serial_unittest.js (right):

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:6: * Unit tests for the JS serial
service client.
On 2014/08/29 06:07:16, raymes wrote:
> A comment mentioning which CC file drives this would be good (and vice-versa)

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:38: function addReceiveHook(callback) {
On 2014/08/29 06:07:16, raymes wrote:
> Please add a comment for these functions. Saves people from having to dig into
> what they actually do.

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:331: function testEcho() {
On 2014/08/29 06:07:16, raymes wrote:
> This looks like it does more than a simple echo. The nesting of sends makes
this
> particularly difficult to read. Could you add comments about what the
different
> cases it tests are?

Split this test into one test for each thing being tested here.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:346: }));
On 2014/08/29 06:07:16, raymes wrote:
> It would be useful to comment about what the second send is for.

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:366: }
On 2014/08/29 06:07:16, raymes wrote:
> maybe move this up above the send.

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:371: function testPausedEcho() {
On 2014/08/29 06:07:16, raymes wrote:
> Change this to be more like testPausedReceiveError with a custom IO handler

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:378: serial.setPaused(connectionId,
false, test.callbackPass());
On 2014/08/29 06:07:16, raymes wrote:
> Why do we need to call this twice?

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:415: serial.setPaused(connectionId,
false, test.callbackPass());
On 2014/08/29 06:07:16, raymes wrote:
> If we've already tested this case, no need to test again.

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:449: },
On 2014/08/29 06:07:16, raymes wrote:
> Consider removing this test as dicussed since it's not testing anything in
> serial.

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:466: test.assertEq('system_error',
sendInfo.error);
On 2014/08/29 06:07:16, raymes wrote:
> Move this above the send just for clarity.

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:532: test.assertEq(12345,
timeoutManager.currentTime);
On 2014/08/29 06:07:16, raymes wrote:
> Maybe move these above the call to the timeout

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/ser...
extensions/test/data/serial_unittest.js:590: runSendErrorTest('timeout');
On 2014/08/29 06:07:16, raymes wrote:
> make a note that this test should pause receiving but wouldn't in production?

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/uni...
File extensions/test/data/unit_test_environment_specific_bindings.js (right):

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/uni...
extensions/test/data/unit_test_environment_specific_bindings.js:49: function
TimeoutManager() {
On 2014/08/29 06:07:16, raymes wrote:
> Comment about what this does would be nice and what it allows.

Done.

https://codereview.chromium.org/509813002/diff/80001/extensions/test/data/uni...
extensions/test/data/unit_test_environment_specific_bindings.js:99:
TimeoutManager.prototype.timeoutCreated = function() {
On 2014/08/29 06:07:16, raymes wrote:
> Maybe we can just have a function which sets the amount to advance after a
> setTimeout is called

Reworked as discussed.

[raymes, 2014-09-02 04:59:32]

Looks like you didn't want to add comments for each of the tests?

https://codereview.chromium.org/509813002/diff/120001/extensions/renderer/api...
File extensions/renderer/api/serial/serial_api_unittest.cc (right):

https://codereview.chromium.org/509813002/diff/120001/extensions/renderer/api...
extensions/renderer/api/serial/serial_api_unittest.cc:12: 
Make a note somewhere in this file of what these tests test and how these tests
work (their interaction with the JS).

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
File extensions/test/data/serial_unittest.js (right):

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
extensions/test/data/serial_unittest.js:437:
timeoutManager.enableAutorun.bind(timeoutManager)));
What about calling these timeoutManager.pause() and timeoutManager.start()? Also
I'm not 100% sure about why we need to disable running?

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
extensions/test/data/serial_unittest.js:551: // case doesn't trigger a pause.
Should this comment also be on testSendErrorTimeout?

[Sam McNally, 2014-09-02 07:41:13]

On 2014/09/02 04:59:32, raymes wrote:
> Looks like you didn't want to add comments for each of the tests?
> 
Whoops. Done.

https://codereview.chromium.org/509813002/diff/120001/extensions/renderer/api...
File extensions/renderer/api/serial/serial_api_unittest.cc (right):

https://codereview.chromium.org/509813002/diff/120001/extensions/renderer/api...
extensions/renderer/api/serial/serial_api_unittest.cc:12: 
On 2014/09/02 04:59:32, raymes wrote:
> Make a note somewhere in this file of what these tests test and how these
tests
> work (their interaction with the JS).

Done.

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
File extensions/test/data/serial_unittest.js (right):

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
extensions/test/data/serial_unittest.js:437:
timeoutManager.enableAutorun.bind(timeoutManager)));
On 2014/09/02 04:59:32, raymes wrote:
> What about calling these timeoutManager.pause() and timeoutManager.start()?
Also
> I'm not 100% sure about why we need to disable running?

Done. For testReceiveTimeout, leaving it running causes the getInfo call to
never complete so it seemed safer to pause it after receiving the timeout.

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
extensions/test/data/serial_unittest.js:551: // case doesn't trigger a pause.
On 2014/09/02 04:59:32, raymes wrote:
> Should this comment also be on testSendErrorTimeout?

Removed these tests.

[raymes, 2014-09-03 01:28:32]

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
File extensions/test/data/serial_unittest.js (right):

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
extensions/test/data/serial_unittest.js:437:
timeoutManager.enableAutorun.bind(timeoutManager)));
Hmm do you know why that is the case? This seems subtle. Maybe we can avoid
needing to pause somehow or failing that we should make it very clear why we
need to pause.

[Sam McNally, 2014-09-03 01:56:13]

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
File extensions/test/data/serial_unittest.js (right):

https://codereview.chromium.org/509813002/diff/120001/extensions/test/data/se...
extensions/test/data/serial_unittest.js:437:
timeoutManager.enableAutorun.bind(timeoutManager)));
On 2014/09/03 01:28:32, raymes wrote:
> Hmm do you know why that is the case? This seems subtle. Maybe we can avoid
> needing to pause somehow or failing that we should make it very clear why we
> need to pause.

Removed the need to pause as discussed.

[raymes, 2014-09-03 06:48:01]

lgtm

[Sam McNally, 2014-09-03 06:52:07]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-09-03 06:53:21]

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

[commit-bot: I haz the power, 2014-09-03 07:32:07]

Committed patchset #4 (id:160001) as eacc99110cba81a973c9ca8d47a1860149afbb7e

[commit-bot: I haz the power, 2014-09-10 03:24:14]

Patchset 4 (id:??) landed as
https://crrev.com/93c5de27b7e35e8e9973645e75075743344d9b26
Cr-Commit-Position: refs/heads/master@{#293092}