Add ProtoDecoder for sending messages between Host and Controller when pairing

Add ProtoDecoder for sending messages between Host and Controller when pairing

BUG=380961, 380962
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289097

[Zachary Kuznia, 2014-08-07 21:43:32]

Please take a look

[achuithb, 2014-08-11 22:04:31]

First pass.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
File components/pairing/proto_decoder.cc (right):

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:29: next_message_size_(0) {}
DCHECK(observer);

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:40: if (message_buffer_->AvailableBytes() <
(int)sizeof(uint8_t)) {
Don't need curly braces.

You should use reinterpret_cast here as well:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...

I'm wondering if the type of AvailableBytes should be size_t?

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:44: uint8_t message_type = MESSAGE_NONE;
Why not char instead of uint8_t?

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:57: if (message_buffer_->AvailableBytes() <
(int)sizeof(uint16_t)) {
reinterpret cast, no curly braces, consider changing the return type of
AvailableBytes()

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:69: next_message_size_ = (high_byte << 8) +
low_byte;
It feels like hte proto libraries should help us with this stuff? It doesn't
feel right to be fiddling with bits here.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:73: if (message_buffer_->AvailableBytes() <
next_message_size_) {
No curly braces.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:127: if
(!message.SerializeToString(&serialized_proto)) {
Shouldn't this be a DCHECK?

Here and everywhere below.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:177: int* size) {
Should this be size_t? You're doing an implicit conversion below.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:190: uint8_t data = (message_size >> 8) &
0xFF;
Is there no way of getting protobufs to do this work as well?

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
File components/pairing/proto_decoder.h (right):

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.h:38: virtual void OnHostStatusMessage(
Please add a public virtual dtor with an empty body, protected ctor with an
empty body, and private DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.h:53: // Decode the data from an io_buffer, and
send out events for any complete
Decodes ..., and sends

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.h:55: bool DecodeIOBuffer(int size,
scoped_refptr<net::IOBuffer> io_buffer);
consider using a typedef for scoped_refptr<net::IOBuffer> - this would make this
interface easier to read.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.h:75: scoped_ptr<MessageBuffer>
message_buffer_;
Why not just have a MessageBuffer member:
MessageBuffer message_buffer_;

[Zachary Kuznia, 2014-08-11 23:40:30]

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
File components/pairing/proto_decoder.cc (right):

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:29: next_message_size_(0) {}
On 2014/08/11 22:04:31, achuithb wrote:
> DCHECK(observer);

Done.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:40: if (message_buffer_->AvailableBytes() <
(int)sizeof(uint8_t)) {
On 2014/08/11 22:04:31, achuithb wrote:
> Don't need curly braces.
> 
> You should use reinterpret_cast here as well:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin...
> 
> I'm wondering if the type of AvailableBytes should be size_t?

I went back and forth on that as well, but this minimizes type conversion.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:44: uint8_t message_type = MESSAGE_NONE;
On 2014/08/11 22:04:31, achuithb wrote:
> Why not char instead of uint8_t?

Unsigned types have less undefined behavior with type conversions.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:57: if (message_buffer_->AvailableBytes() <
(int)sizeof(uint16_t)) {
On 2014/08/11 22:04:30, achuithb wrote:
> reinterpret cast, no curly braces, consider changing the return type of
> AvailableBytes()

Changed to static_cast, removed braces.

I considered changing the return value, but it added more casts elsewhere.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:69: next_message_size_ = (high_byte << 8) +
low_byte;
On 2014/08/11 22:04:31, achuithb wrote:
> It feels like hte proto libraries should help us with this stuff? It doesn't
> feel right to be fiddling with bits here.

See comment at line 190

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:73: if (message_buffer_->AvailableBytes() <
next_message_size_) {
On 2014/08/11 22:04:30, achuithb wrote:
> No curly braces.

Done.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:127: if
(!message.SerializeToString(&serialized_proto)) {
On 2014/08/11 22:04:31, achuithb wrote:
> Shouldn't this be a DCHECK?
> 
> Here and everywhere below.

I avoid putting a function call in any kind of assert, because the calling
semantics can change without notice.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:177: int* size) {
On 2014/08/11 22:04:31, achuithb wrote:
> Should this be size_t? You're doing an implicit conversion below.

The BluetoothSocket function deals in IOBuffer and int size, so I wanted to keep
type conversions isolated to this class.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.cc:190: uint8_t data = (message_size >> 8) &
0xFF;
On 2014/08/11 22:04:31, achuithb wrote:
> Is there no way of getting protobufs to do this work as well?

The other call sites I've seen in Chrome serialize the message size themselves
as well.  Trying to get the protobuffers to handle the type and size looked
possible, but would have been a more complex solution and require more overhead,
so I opted for sticking to reading/writing three bytes.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
File components/pairing/proto_decoder.h (right):

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.h:38: virtual void OnHostStatusMessage(
On 2014/08/11 22:04:31, achuithb wrote:
> Please add a public virtual dtor with an empty body, protected ctor with an
> empty body, and private DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.h:53: // Decode the data from an io_buffer, and
send out events for any complete
On 2014/08/11 22:04:31, achuithb wrote:
> Decodes ..., and sends

Done.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.h:55: bool DecodeIOBuffer(int size,
scoped_refptr<net::IOBuffer> io_buffer);
On 2014/08/11 22:04:31, achuithb wrote:
> consider using a typedef for scoped_refptr<net::IOBuffer> - this would make
this
> interface easier to read.

Done.

https://codereview.chromium.org/448273002/diff/40001/components/pairing/proto...
components/pairing/proto_decoder.h:75: scoped_ptr<MessageBuffer>
message_buffer_;
On 2014/08/11 22:04:31, achuithb wrote:
> Why not just have a MessageBuffer member:
> MessageBuffer message_buffer_;

Done.

[achuithb, 2014-08-12 18:37:38]

lgtm

[Zachary Kuznia, 2014-08-12 18:40:02]

The CQ bit was checked by zork@chromium.org

[commit-bot: I haz the power, 2014-08-12 18:41:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zork@chromium.org/448273002/60001

[commit-bot: I haz the power, 2014-08-12 22:50:08]

Change committed as 289097