Implement serial API for Windows.

Implement serial API for Windows.

- Got it working using PlatformFile.
- Noticed threading-constraint violations; refactored APIResourceController to allow for its resources to exist on different threads, depending on the resource type.
- Wondered why I wasn't using PlatformFile for the POSIX version, which would have caused me to build the POSIX version on the file thread to being with.
- Fixed that.
- Once things were working, observed that serial was not yet using ArrayBuffer. Fixed that.
- Simplified ExtensionService's relationship with APIResourceController.
- Hardcode all platforms to set bitrate of 57600.

BUG=129391, 129483
TEST=Widened scope of existing tests, manually tested with Arduino

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139463

[asargent_no_longer_on_chrome, 2012-05-24 22:43:03]

As mentioned in person, let's think about possible race conditions at shutdown
around accessing the APIResourceController from a thread other than the UI one.

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
File chrome/browser/extensions/api/api_resource_controller.cc (right):

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_controller.cc:56: // same one that
created it.
Please create a bug for this so we don't forget to do it. I have a feeling this
would be a security black eye!

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_controller.cc:79: int
APIResourceController::AddAPIResource(APIResource* api_resource) {
Since the Get* functions must be called on particular threads (IO or FILE),
should you also be asserting here that Add is called on the matching thread (to
avoid race conditions in the future)?

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
File chrome/browser/extensions/api/api_resource_controller.h (right):

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_controller.h:46: bool
RemoveUsbDeviceResource(int api_resource_id);
There's a sort of asymmetry now between add and remove, in that you have to call
a specific Remove* function depending on the resource type. Would it make sense
to have Add* functions that match this?

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
File chrome/browser/extensions/api/serial/serial_api.cc (right):

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/serial/serial_api.cc:56:
EXTENSION_FUNCTION_VALIDATE(args_->GetString(argument_position++, &port_));
Since SerialConnection::Open has a CHECK that port is ascii, should you
EXTENSION_FUNCTION_VALIDATE that here in advance so that an extension only kills
itself and doesn't accidentally/maliciously shut down the whole browser with
invalid inputs?

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/serial/serial_api.cc:70:
SerialPortEnumerator::GenerateValidSerialPortNames());
shouldn't this remain indented 4 spaces?

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/serial/serial_api.cc:95: }
nit: looks like you call AsyncWorkCompleted in any path through here - should
you merge the 2 calls to just 1 outside the if statement?

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
File chrome/browser/extensions/api/serial/serial_connection.cc (right):

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/serial/serial_connection.cc:33:
CHECK(IsStringASCII(port_));
Does this really warrant a CHECK (eg all of chrome immediately exits) and not
just returning false?

[miket_OOO, 2012-05-24 23:44:43]

Changes made. On trybots now.

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
File chrome/browser/extensions/api/api_resource_controller.cc (right):

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_controller.cc:56: // same one that
created it.
On 2012/05/24 22:43:03, Antony Sargent wrote:
> Please create a bug for this so we don't forget to do it. I have a feeling
this
> would be a security black eye!

https://code.google.com/p/chromium/issues/detail?id=129675

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/api_resource_controller.cc:79: int
APIResourceController::AddAPIResource(APIResource* api_resource) {
On 2012/05/24 22:43:03, Antony Sargent wrote:
> Since the Get* functions must be called on particular threads (IO or FILE),
> should you also be asserting here that Add is called on the matching thread
(to
> avoid race conditions in the future)? 

Done.

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
File chrome/browser/extensions/api/serial/serial_api.cc (right):

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/serial/serial_api.cc:56:
EXTENSION_FUNCTION_VALIDATE(args_->GetString(argument_position++, &port_));
On 2012/05/24 22:43:03, Antony Sargent wrote:
> Since SerialConnection::Open has a CHECK that port is ascii, should you
> EXTENSION_FUNCTION_VALIDATE that here in advance so that an extension only
kills
> itself and doesn't accidentally/maliciously shut down the whole browser with
> invalid inputs?

Good idea, though it's currently not possible because SerialOpenFunction
validates the input against a whitelist of known-good serial ports for the
current machine.

Still, done.

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/serial/serial_api.cc:70:
SerialPortEnumerator::GenerateValidSerialPortNames());
On 2012/05/24 22:43:03, Antony Sargent wrote:
> shouldn't this remain indented 4 spaces?

Yes.

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/serial/serial_api.cc:95: }
On 2012/05/24 22:43:03, Antony Sargent wrote:
> nit: looks like you call AsyncWorkCompleted in any path through here - should
> you merge the 2 calls to just 1 outside the if statement?

And result_.reset(result).

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
File chrome/browser/extensions/api/serial/serial_connection.cc (right):

http://codereview.chromium.org/10392181/diff/33003/chrome/browser/extensions/...
chrome/browser/extensions/api/serial/serial_connection.cc:33:
CHECK(IsStringASCII(port_));
On 2012/05/24 22:43:03, Antony Sargent wrote:
> Does this really warrant a CHECK (eg all of chrome immediately exits) and not
> just returning false?

In this case, I'd say yes. It's because if this were to fail, it's a coding
error (we failed to validate the input). I'm afraid that the FromUTF8Unsafe()
call below might be... uh, unsafe, if I can't guarantee that the input is ASCII
(& thus UTF8). Better to kill the browser for a logic error than to pass
known-bad data to a function marked Unsafe.

[miket_OOO, 2012-05-24 23:47:38]

Matt, added you as OWNER of base.

[Matt Perry, 2012-05-24 23:53:04]

On 2012/05/24 23:47:38, miket wrote:
> Matt, added you as OWNER of base.

I'm not a base OWNER.

[miket_OOO, 2012-05-25 14:48:45]

On 2012/05/24 23:53:04, Matt Perry wrote:
> On 2012/05/24 23:47:38, miket wrote:
> > Matt, added you as OWNER of base.
> 
> I'm not a base OWNER.

Sorry about that. I'll try to figure out why the presubmit script suggested you.

[asargent_no_longer_on_chrome, 2012-05-25 17:40:45]

Changes lgtm.

I came up with a couple of ideas for solving the narrow shutdown race condition
(where an API method might be in its ::Work method on a non-UI thread when the
controller is getting destructed on the UI thread, and the API method's code
calls controller()->{Add,Remove}*). I'll follow up shortly - in the mean time
can you just file a bug for this?

[commit-bot: I haz the power, 2012-05-29 16:29:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/10392181/33025

[commit-bot: I haz the power, 2012-05-29 16:30:01]

Presubmit check for 10392181-33025 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base

Presubmit checks took 2.0s to calculate.

[miket_OOO, 2012-05-29 16:32:33]

Darin, I need a base/ reviewer. Would you please?

[darin (slow to review), 2012-05-29 19:04:33]

https://chromiumcodereview.appspot.com/10392181/diff/33025/base/platform_file.h
File base/platform_file.h (right):

https://chromiumcodereview.appspot.com/10392181/diff/33025/base/platform_file...
base/platform_file.h:131: BASE_EXPORT int
ReadPlatformFileTerminalDevice(PlatformFile file,
nit: since the point of this function is to read without seeking, it seems like
the name should probably reflect that. 
ReadPlatformFile{Stream,FromCurrentPos,FromCurrentPosition,WithoutSeeking}

ReadPlatformFileFromCurrentPos seems OK to me, despite the "Pos" abbreviation.

[miket_OOO, 2012-05-29 20:04:06]

> ReadPlatformFile{Stream,FromCurrentPos,FromCurrentPosition,WithoutSeeking}

Done. Same number of characters, too. On trybots now.

[darin (slow to review), 2012-05-29 20:24:10]

https://chromiumcodereview.appspot.com/10392181/diff/29025/base/platform_file.h
File base/platform_file.h (right):

https://chromiumcodereview.appspot.com/10392181/diff/29025/base/platform_file...
base/platform_file.h:148: BASE_EXPORT int
WritePlatformFileFromCurrentPos(PlatformFile file,
nitty-nit: I forgot to consider how this would look for Write*.  You probably
want "read..from" and "write..to" _or_ you could go with "read..at" and
"write..at"

[miket_OOO, 2012-05-29 20:53:03]

Updated.

https://chromiumcodereview.appspot.com/10392181/diff/29025/base/platform_file.h
File base/platform_file.h (right):

https://chromiumcodereview.appspot.com/10392181/diff/29025/base/platform_file...
base/platform_file.h:148: BASE_EXPORT int
WritePlatformFileFromCurrentPos(PlatformFile file,
On 2012/05/29 20:24:10, darin wrote:
> nitty-nit: I forgot to consider how this would look for Write*.  You probably
> want "read..from" and "write..to" _or_ you could go with "read..at" and
> "write..at"

Very interesting! I thought of it as "reading from" and "writing from" a certain
position (as in "start at this point and keep going FROM there"), but I surveyed
100% of the engineers in my office (N=Antony), and 100% of them thought that
"from" sounded awkward, too. I am outvoted so I won't argue.

[darin (slow to review), 2012-05-29 20:55:26]

src/base/ <- LGTM

[commit-bot: I haz the power, 2012-05-29 21:16:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/10392181/24104

[commit-bot: I haz the power, 2012-05-30 02:31:17]

Change committed as 139463