Use vectors instead of IOBuffer for U2fPackets

Use vectors instead of IOBuffer for U2fPackets

Changed to using vector class variables in U2fPacket. Now only
converting to IOBuffer on the fly in U2fMessage when needed.

BUG=704309
Review-Url: https://codereview.chromium.org/2771673002
Cr-Commit-Position: refs/heads/master@{#459211}
Committed: https://chromium.googlesource.com/chromium/src/+/b4dde882f6c2b2be84035b53f60ec286d347bc50

[Casey Piper, 2017-03-22 22:28:07]

Patchset #1 (id:1) has been deleted

[Casey Piper, 2017-03-22 22:33:38]

Patchset #1 (id:20001) has been deleted

[Casey Piper, 2017-03-22 22:35:07]

piperc@chromium.org changed reviewers:
+ kpaulhamus@chromium.org, reillyg@chromium.org

[Casey Piper, 2017-03-22 22:35:07]

piperc@chromium.org changed required reviewers:
+ reillyg@chromium.org

[Reilly Grant (use Gerrit), 2017-03-22 23:08:17]

Note, everywhere where I've suggested using const std::vector<uint8_t>& you
could use std::vector<uint8_t> and always std::move into that parameter but I
think the code style doesn't recommend that because it's too easy to forget and
introduce extra copies.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_message.cc
File device/u2f/u2f_message.cc (right):

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_message....
device/u2f/u2f_message.cc:33: std::vector<uint8_t> init_data(buf->data(),
buf->data() + buf->size());
This method should take a const std::vector<uint8_t>& instead of
net::IOBufferWithSize because U2fHidDevice::OnRead is already making a copy to
convert from net::IOBuffer to net::IOBufferWithSize.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_message....
device/u2f/u2f_message.cc:99: std::vector<uint8_t> data =
packets_.front()->GetSerializedData();
If GetSerializedData() will be building this vector from scratch it may be
better have it return the net::IOBufferWithSize directly.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.cc
File device/u2f/u2f_packet.cc (right):

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.c...
device/u2f/u2f_packet.cc:12: U2fPacket::U2fPacket(const std::vector<uint8_t>
data, const uint32_t channel_id)
const std::vector<uint8_t>&

The other parameters aren't const in the header and so shouldn't be here.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.c...
device/u2f/u2f_packet.cc:32: const std::vector<uint8_t> data,
const std::vector<uint8_t>&

The other parameters aren't const in the header and so shouldn't be here.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.h
File device/u2f/u2f_packet.h (right):

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.h...
device/u2f/u2f_packet.h:55: U2fInitPacket(std::vector<uint8_t> serialized,
size_t* remaining_size);
const std::vector<uint8_t>&

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.h...
device/u2f/u2f_packet.h:63: std::vector<uint8_t> serialized,
const std::vector<uint8_t>&

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.h...
device/u2f/u2f_packet.h:84: U2fContinuationPacket(std::vector<uint8_t>
serialized,
const std::vector<uint8_t>&

[Casey Piper, 2017-03-23 03:32:55]

Patchset #2 (id:60001) has been deleted

[Casey Piper, 2017-03-23 03:41:20]

Patchset #2 (id:80001) has been deleted

[Casey Piper, 2017-03-23 03:46:52]

Patchset #2 (id:100001) has been deleted

[Casey Piper, 2017-03-23 03:47:25]

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_message.cc
File device/u2f/u2f_message.cc (right):

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_message....
device/u2f/u2f_message.cc:33: std::vector<uint8_t> init_data(buf->data(),
buf->data() + buf->size());
On 2017/03/22 23:08:16, Reilly Grant wrote:
> This method should take a const std::vector<uint8_t>& instead of
> net::IOBufferWithSize because U2fHidDevice::OnRead is already making a copy to
> convert from net::IOBuffer to net::IOBufferWithSize.

Done.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_message....
device/u2f/u2f_message.cc:99: std::vector<uint8_t> data =
packets_.front()->GetSerializedData();
On 2017/03/22 23:08:17, Reilly Grant wrote:
> If GetSerializedData() will be building this vector from scratch it may be
> better have it return the net::IOBufferWithSize directly.

Done.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.cc
File device/u2f/u2f_packet.cc (right):

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.c...
device/u2f/u2f_packet.cc:12: U2fPacket::U2fPacket(const std::vector<uint8_t>
data, const uint32_t channel_id)
On 2017/03/22 23:08:17, Reilly Grant wrote:
> const std::vector<uint8_t>&
> 
> The other parameters aren't const in the header and so shouldn't be here.

Done.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.c...
device/u2f/u2f_packet.cc:32: const std::vector<uint8_t> data,
On 2017/03/22 23:08:17, Reilly Grant wrote:
> const std::vector<uint8_t>&
> 
> The other parameters aren't const in the header and so shouldn't be here.

Done.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.h
File device/u2f/u2f_packet.h (right):

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.h...
device/u2f/u2f_packet.h:55: U2fInitPacket(std::vector<uint8_t> serialized,
size_t* remaining_size);
On 2017/03/22 23:08:17, Reilly Grant wrote:
> const std::vector<uint8_t>&

Done.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.h...
device/u2f/u2f_packet.h:63: std::vector<uint8_t> serialized,
On 2017/03/22 23:08:17, Reilly Grant wrote:
> const std::vector<uint8_t>&

Done.

https://codereview.chromium.org/2771673002/diff/40001/device/u2f/u2f_packet.h...
device/u2f/u2f_packet.h:84: U2fContinuationPacket(std::vector<uint8_t>
serialized,
On 2017/03/22 23:08:17, Reilly Grant wrote:
> const std::vector<uint8_t>&

Done.

[Casey Piper, 2017-03-23 18:06:32]

The CQ bit was checked by piperc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 18:07:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Reilly Grant (use Gerrit), 2017-03-23 18:15:39]

https://codereview.chromium.org/2771673002/diff/120001/device/u2f/u2f_packet.cc
File device/u2f/u2f_packet.cc (right):

https://codereview.chromium.org/2771673002/diff/120001/device/u2f/u2f_packet....
device/u2f/u2f_packet.cc:54: serialized->data()[index++] = data_[data_idx];
std::memcpy(&serialized->data()[index], data_.data(), data_.size());
index += data_.size();

https://codereview.chromium.org/2771673002/diff/120001/device/u2f/u2f_packet....
device/u2f/u2f_packet.cc:82: payload_size |=
static_cast<uint8_t>(serialized[index++]);
This static_cast is now unnecessary.

https://codereview.chromium.org/2771673002/diff/120001/device/u2f/u2f_packet....
device/u2f/u2f_packet.cc:120: serialized->data()[index++] = data_[idx];
std::memcpy(&serialized->data()[index], data_.data(), data_.size());
index += data_.size();

[Casey Piper, 2017-03-23 18:16:28]

The CQ bit was unchecked by piperc@chromium.org

[Casey Piper, 2017-03-23 18:24:15]

Patchset #3 (id:140001) has been deleted

[Casey Piper, 2017-03-23 18:35:51]

https://codereview.chromium.org/2771673002/diff/120001/device/u2f/u2f_packet.cc
File device/u2f/u2f_packet.cc (right):

https://codereview.chromium.org/2771673002/diff/120001/device/u2f/u2f_packet....
device/u2f/u2f_packet.cc:54: serialized->data()[index++] = data_[data_idx];
On 2017/03/23 18:15:39, Reilly Grant wrote:
> std::memcpy(&serialized->data()[index], data_.data(), data_.size());
> index += data_.size();

Done.

https://codereview.chromium.org/2771673002/diff/120001/device/u2f/u2f_packet....
device/u2f/u2f_packet.cc:82: payload_size |=
static_cast<uint8_t>(serialized[index++]);
On 2017/03/23 18:15:39, Reilly Grant wrote:
> This static_cast is now unnecessary.

Acknowledged.

https://codereview.chromium.org/2771673002/diff/120001/device/u2f/u2f_packet....
device/u2f/u2f_packet.cc:120: serialized->data()[index++] = data_[idx];
On 2017/03/23 18:15:39, Reilly Grant wrote:
> std::memcpy(&serialized->data()[index], data_.data(), data_.size());
> index += data_.size();

Done.

[Reilly Grant (use Gerrit), 2017-03-23 18:39:54]

lgtm

https://codereview.chromium.org/2771673002/diff/160001/device/u2f/u2f_message...
File device/u2f/u2f_message_fuzzer.cc (right):

https://codereview.chromium.org/2771673002/diff/160001/device/u2f/u2f_message...
device/u2f/u2f_message_fuzzer.cc:15: uint8_t* start =
const_cast<uint8_t*>(data);
Looking at this now I can't figure out why this needed a const_cast.

[Casey Piper, 2017-03-23 18:48:05]

https://codereview.chromium.org/2771673002/diff/160001/device/u2f/u2f_message...
File device/u2f/u2f_message_fuzzer.cc (right):

https://codereview.chromium.org/2771673002/diff/160001/device/u2f/u2f_message...
device/u2f/u2f_message_fuzzer.cc:15: uint8_t* start =
const_cast<uint8_t*>(data);
On 2017/03/23 18:39:54, Reilly Grant wrote:
> Looking at this now I can't figure out why this needed a const_cast.

The compiler is requiring it in this case. Granted, I could've kept and
incremented an index instead and just used the original data pointer.

[Casey Piper, 2017-03-23 20:15:29]

https://codereview.chromium.org/2771673002/diff/160001/device/u2f/u2f_message...
File device/u2f/u2f_message_fuzzer.cc (right):

https://codereview.chromium.org/2771673002/diff/160001/device/u2f/u2f_message...
device/u2f/u2f_message_fuzzer.cc:15: uint8_t* start =
const_cast<uint8_t*>(data);
On 2017/03/23 18:48:05, Casey Piper wrote:
> On 2017/03/23 18:39:54, Reilly Grant wrote:
> > Looking at this now I can't figure out why this needed a const_cast.
> 
> The compiler is requiring it in this case. Granted, I could've kept and
> incremented an index instead and just used the original data pointer.

Done.

[Casey Piper, 2017-03-23 20:17:47]

The CQ bit was checked by piperc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 20:18:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 20:45:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 20:45:01]

Dry run: This issue passed the CQ dry run.

[Reilly Grant (use Gerrit), 2017-03-23 20:45:27]

still lgtm

[Casey Piper, 2017-03-23 20:45:54]

The CQ bit was checked by piperc@chromium.org

[commit-bot: I haz the power, 2017-03-23 20:46:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 20:51:15]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490301954192850,
"parent_rev": "c9416cfaa6e0303da35d21214502be39239032a8", "commit_rev":
"b4dde882f6c2b2be84035b53f60ec286d347bc50"}

[commit-bot: I haz the power, 2017-03-23 20:52:04]

Description was changed from

==========
Use vectors instead of IOBuffer for U2fPackets

Changed to using vector class variables in U2fPacket. Now only
converting to IOBuffer on the fly in U2fMessage when needed.

BUG=704309
==========

to

==========
Use vectors instead of IOBuffer for U2fPackets

Changed to using vector class variables in U2fPacket. Now only
converting to IOBuffer on the fly in U2fMessage when needed.

BUG=704309

Review-Url: https://codereview.chromium.org/2771673002
Cr-Commit-Position: refs/heads/master@{#459211}
Committed:
https://chromium.googlesource.com/chromium/src/+/b4dde882f6c2b2be84035b53f60e...
==========

[commit-bot: I haz the power, 2017-03-23 20:52:05]

Committed patchset #4 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/b4dde882f6c2b2be84035b53f60e...