Don't pass buffers to HidConnection::Read because it knows the size.

Don't pass buffers to HidConnection::Read because it knows the size.

The HidConnection knows the appropriate read buffer size and the
platform-specific implementation may already have a buffer that it is
waiting to return to the next caller. Passing a buffer in is therefore
unnecessary, one can be provided in the callback.

By standardizing on a buffer format which always includes the report ID
as the first byte (even when it is zero) most of the buffer copying can
be removed except in the case of OS X's persistent read buffer and
getting feature reports on Windows.

BUG=

Committed: https://crrev.com/0e9bf9bc05be91d727134887b3c453353be1024d
Cr-Commit-Position: refs/heads/master@{#291954}

[Ken Rockot(use gerrit already), 2014-08-26 16:32:04]

lgtm pending nits and verification of all the fun buffer size adjustment
behavior.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_li...
File device/hid/hid_connection_linux.cc (right):

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_li...
device/hid/hid_connection_linux.cc:112: new
net::IOBufferWithSize(device_info().max_feature_report_size + 1));
Just for sanity checking: This seems to imply that max_feature_report_size will
always be the size of a report sans report ID, but that the ioctl will always
include a report ID (or 0) as the first byte. Seems suspicious but it wouldn't
surprise me. Please confirm/deny.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_li...
device/hid/hid_connection_linux.cc:146: size_t expected_report_size =
device_info().max_input_report_size + 1;
Same deal with the +1 here.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_li...
device/hid/hid_connection_linux.cc:210: if (CompleteRead(report.buffer,
report.size, read.callback)) {
Much nicer.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_un...
File device/hid/hid_connection_unittest.cc (right):

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_un...
device/hid/hid_connection_unittest.cc:48: HidConnection::ReadCallback
read_callback() {
nit: It's not strictly an accessor so this naming convention should no longer
apply. I would either change the name, or just bind new read_callback_ and
write_callback_ members in the constructor so these can remain as accessors.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_win.h
File device/hid/hid_connection_win.h (right):

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_wi...
device/hid/hid_connection_win.h:23: // HidConnection implementation.
nit: Might as well make the Platform* methods private all around, yeah? They
should only be called by HidConnection.

[Reilly Grant (use Gerrit), 2014-08-26 17:16:47]

Patchset #2 (id:20001) has been deleted

[Reilly Grant (use Gerrit), 2014-08-26 17:17:22]

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_li...
File device/hid/hid_connection_linux.cc (right):

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_li...
device/hid/hid_connection_linux.cc:112: new
net::IOBufferWithSize(device_info().max_feature_report_size + 1));
On 2014/08/26 16:32:04, Ken Rockot wrote:
> Just for sanity checking: This seems to imply that max_feature_report_size
will
> always be the size of a report sans report ID, but that the ioctl will always
> include a report ID (or 0) as the first byte. Seems suspicious but it wouldn't
> surprise me. Please confirm/deny.

The ioctl will overwrite the first byte with the first byte of the report so...
I don't need to +1 here though there's no harm in doing so.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_li...
device/hid/hid_connection_linux.cc:146: size_t expected_report_size =
device_info().max_input_report_size + 1;
On 2014/08/26 16:32:04, Ken Rockot wrote:
> Same deal with the +1 here.

Here I do because the read may or may not include the report ID but I want the
buffer passed to always include the report ID, hence the buffer manipulation
below.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_li...
device/hid/hid_connection_linux.cc:210: if (CompleteRead(report.buffer,
report.size, read.callback)) {
On 2014/08/26 16:32:04, Ken Rockot wrote:
> Much nicer.

Acknowledged.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_un...
File device/hid/hid_connection_unittest.cc (right):

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_un...
device/hid/hid_connection_unittest.cc:48: HidConnection::ReadCallback
read_callback() {
On 2014/08/26 16:32:04, Ken Rockot wrote:
> nit: It's not strictly an accessor so this naming convention should no longer
> apply. I would either change the name, or just bind new read_callback_ and
> write_callback_ members in the constructor so these can remain as accessors.

Done.

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_win.h
File device/hid/hid_connection_win.h (right):

https://codereview.chromium.org/499713002/diff/1/device/hid/hid_connection_wi...
device/hid/hid_connection_win.h:23: // HidConnection implementation.
On 2014/08/26 16:32:04, Ken Rockot wrote:
> nit: Might as well make the Platform* methods private all around, yeah? They
> should only be called by HidConnection.

Done.

[Reilly Grant (use Gerrit), 2014-08-26 18:18:06]

The CQ bit was checked by reillyg@chromium.org

[commit-bot: I haz the power, 2014-08-26 18:18:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/reillyg@chromium.org/499713002/40001

[commit-bot: I haz the power, 2014-08-26 19:10:24]

Committed patchset #2 (40001) as 33cd493b35187731768ea8eaf4bc9b320422a252

[commit-bot: I haz the power, 2014-09-10 02:44:32]

Patchset 2 (id:??) landed as
https://crrev.com/0e9bf9bc05be91d727134887b3c453353be1024d
Cr-Commit-Position: refs/heads/master@{#291954}