Blimp: add packet-level DEFLATE compression using zlib.

Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Integrate CompressedPacketReader/CompressedPacketWriter into application.
* Make PacketReader resize its input GrowableIOBuffer as needed.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145
Committed: https://crrev.com/38ad3ca2674316ec799b678a69fe3a7bfd5f0edf
Cr-Commit-Position: refs/heads/master@{#384029}

[agl, 2016-03-23 05:21:10]

LGTM for deps addition.

(I'm on vacation so haven't looked in more detail, but I didn't want to block
this on something so minor.)

Be aware that compression under encryption causes information to leak in the
length of the resulting compressed data. I've no idea whether that's actually
applicable here though.

[Kevin M, 2016-03-24 17:56:11]

Description was changed from

==========
Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Modify PacketReader interface to pass back read sizes as result values
  instead of buffer offsets. This is necessary for efficient re-use of
  GrowableIOBuffer across reads, otherwise it's just reallocs everywhere.
* Modify PacketReader implementations, dependents, and test code to use
  new interface.
* Modify PacketWriter to take scoped_refptr by reference, to reduce
  unneeded refcount churn.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145
==========

to

==========
Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145
==========

[Kevin M, 2016-03-24 17:59:23]

Moved PacketReader return value change to 1831833003.

[Kevin M, 2016-03-25 21:17:46]

Description was changed from

==========
Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145
==========

to

==========
Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Integrate CompressedPacketReader/CompressedPacketWriter into application.
* Make PacketReader resize its input GrowableIOBuffer as needed.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145
==========

[maniscalco, 2016-03-25 22:23:16]

maniscalco@chromium.org changed reviewers:
+ maniscalco@chromium.org

[maniscalco, 2016-03-25 22:23:17]

(drive-by question)

https://codereview.chromium.org/1825263003/diff/80001/blimp/net/compressed_pa...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1825263003/diff/80001/blimp/net/compressed_pa...
blimp/net/compressed_packet_writer.cc:35: zlib_stream_.zalloc = 0;
Is there a reason you're zero-initializing only these two fields of the
zlib_stream_ struct instead of zero-initializing the whole thing?  Zeroing out
the whole struct seems like a safer practice.

[Kevin M, 2016-03-25 22:51:41]

https://codereview.chromium.org/1825263003/diff/80001/blimp/net/compressed_pa...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1825263003/diff/80001/blimp/net/compressed_pa...
blimp/net/compressed_packet_writer.cc:35: zlib_stream_.zalloc = 0;
On 2016/03/25 22:23:17, maniscalco wrote:
> Is there a reason you're zero-initializing only these two fields of the
> zlib_stream_ struct instead of zero-initializing the whole thing?  Zeroing out
> the whole struct seems like a safer practice.

From what I've read, deflateInit2 can be depended on to initialize the struct
for us. The only values it doesn't clobber are the malloc/free function
pointers, and "opaque", which is N/A as we are using the default malloc
routines.

But anyways, your point is valid. Might as well init the whole thing myself just
to be on the safe side.

[maniscalco, 2016-03-25 23:00:08]

https://codereview.chromium.org/1825263003/diff/80001/blimp/net/compressed_pa...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1825263003/diff/80001/blimp/net/compressed_pa...
blimp/net/compressed_packet_writer.cc:35: zlib_stream_.zalloc = 0;
On 2016/03/25 22:51:41, Kevin M wrote:
> On 2016/03/25 22:23:17, maniscalco wrote:
> > Is there a reason you're zero-initializing only these two fields of the
> > zlib_stream_ struct instead of zero-initializing the whole thing?  Zeroing
out
> > the whole struct seems like a safer practice.
> 
> From what I've read, deflateInit2 can be depended on to initialize the struct
> for us. The only values it doesn't clobber are the malloc/free function
> pointers, and "opaque", which is N/A as we are using the default malloc
> routines.
> 
> But anyways, your point is valid. Might as well init the whole thing myself
just
> to be on the safe side.

Cool, I think that's wise.  It looks like most other places in chromium also
zero-initialize the struct.  There's another one in CompressedPacketReader too,
right?

[Kevin M, 2016-03-25 23:37:20]

https://codereview.chromium.org/1825263003/diff/80001/blimp/net/compressed_pa...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1825263003/diff/80001/blimp/net/compressed_pa...
blimp/net/compressed_packet_writer.cc:35: zlib_stream_.zalloc = 0;
On 2016/03/25 23:00:08, maniscalco wrote:
> On 2016/03/25 22:51:41, Kevin M wrote:
> > On 2016/03/25 22:23:17, maniscalco wrote:
> > > Is there a reason you're zero-initializing only these two fields of the
> > > zlib_stream_ struct instead of zero-initializing the whole thing?  Zeroing
> out
> > > the whole struct seems like a safer practice.
> > 
> > From what I've read, deflateInit2 can be depended on to initialize the
struct
> > for us. The only values it doesn't clobber are the malloc/free function
> > pointers, and "opaque", which is N/A as we are using the default malloc
> > routines.
> > 
> > But anyways, your point is valid. Might as well init the whole thing myself
> just
> > to be on the safe side.
> 
> Cool, I think that's wise.  It looks like most other places in chromium also
> zero-initialize the struct.  There's another one in CompressedPacketReader
too,
> right?

Done.

[Wez, 2016-03-29 00:45:40]

Some initial comments on the reader side.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:31: int err = inflateInit2(&zlib_stream_,
-MAX_WBITS);
nit: |init_result|?

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:32: DCHECK_EQ(err, Z_OK);
nit: Suggest DCHECK_EQ(Z_OK, err)

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:41: const net::CompletionCallback& cb) {
nit: |callback|, here and below.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:68:
base::NetToHost32(*reinterpret_cast<uint32_t*>(compressed));
Don't you need to check that |compressed_size| is >= sizeof(uint32_t) here?

I'd also prefer that, given a compressed buffer + size, we have simple static
helper methods that pull out e.g. the decompressed size, the compressed-data
pointer, and the actual compressed-data size, to make the rest of this more
readable.  Or, at least define a kHeaderSize = sizeof(uint32_t) that can be used
below.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:70: // Empty payload - nothing to do.
Why would we allow an empty but compressed-looking payload, rather than a
zero-length message at the underlying layer?

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:77: if
(static_cast<uint32_t>(decompressed->capacity()) < decompressed_size) {
nit: Don't abutt this buffer-resize logic into the early-exit sanity checks; I'd
suggest:

FetchSize

IsItZero?
IsItInsane?

SetCapacityToSize

as the logical blocks here, each w/ a one-line comment introducing them, e.g.
"Check that the payload size is within bounds."

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:84: static_cast<unsigned>(compressed_size
- sizeof(uint32_t));
unsigned what?

What about using a safe type-conversion to get compressed_size from |int| to
|size_t| and then doing the -= on that?

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:87: int err = inflate(&zlib_stream_,
Z_SYNC_FLUSH);
nit: |inflate_result|

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:95: if (zlib_stream_.avail_in > 0) {
This provides us a mechanism for telling when our buffer was too small; we could
e.g. double the decompressed-data buffer in such situations, allowing us to try
decompressing into the buffer and resize it if there wasn't enough space,
removing the need for the explicit decompressed-size header. WDYT?

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.h (right):

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:37: void ReadCompressedPacketDone(
nit: OnCompressedPacketReceived or OnCompressedReadPacketComplete?

There's a general pattern of OnFoo() for callback/handler methods in classes, in
Chromium.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:50: int compressed_size,
nit: Could you just pass the IOBuffer into which the compressed data was read,
or if that doesn't have the right length then use StringPiece to fold the data
pointer+size into a single parameter?

nit: Since this method already touches |zlib_stream_|, why not let it access
|compressed_buf_| too, rather than pass the buffer in as ptr+size?

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:55: z_stream zlib_stream_;
nit: I'm not familiar with all the C++11 rules, but could this in-line
initialize to zeros w/out memset() by using ... = {0}?

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:56:
base::WeakPtrFactory<CompressedPacketReader> weak_factory_;
nit: Suggest a comment preceding |weak_factory_|, e.g. "Used to abandon pending
ReadPacket() calls on teardown.)

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
File blimp/net/compressed_packet_unittest.cc (right):

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:39: DCHECK_EQ(0, dest_buf->offset());
Don't we use ASSERT in tests, rather than DCHECK?

[Kevin M, 2016-03-29 01:25:55]

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:31: int err = inflateInit2(&zlib_stream_,
-MAX_WBITS);
On 2016/03/29 00:45:39, Wez wrote:
> nit: |init_result|?

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:32: DCHECK_EQ(err, Z_OK);
On 2016/03/29 00:45:39, Wez wrote:
> nit: Suggest DCHECK_EQ(Z_OK, err)

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:41: const net::CompletionCallback& cb) {
On 2016/03/29 00:45:39, Wez wrote:
> nit: |callback|, here and below.

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:68:
base::NetToHost32(*reinterpret_cast<uint32_t*>(compressed));
On 2016/03/29 00:45:39, Wez wrote:
> Don't you need to check that |compressed_size| is >= sizeof(uint32_t) here?
> 
> I'd also prefer that, given a compressed buffer + size, we have simple static
> helper methods that pull out e.g. the decompressed size, the compressed-data
> pointer, and the actual compressed-data size, to make the rest of this more
> readable.  Or, at least define a kHeaderSize = sizeof(uint32_t) that can be
used
> below.

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:70: // Empty payload - nothing to do.
On 2016/03/29 00:45:39, Wez wrote:
> Why would we allow an empty but compressed-looking payload, rather than a
> zero-length message at the underlying layer?

Good question. Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:77: if
(static_cast<uint32_t>(decompressed->capacity()) < decompressed_size) {
On 2016/03/29 00:45:39, Wez wrote:
> nit: Don't abutt this buffer-resize logic into the early-exit sanity checks;
I'd
> suggest:
> 
> FetchSize
> 
> IsItZero?
> IsItInsane?
> 
> SetCapacityToSize
> 
> as the logical blocks here, each w/ a one-line comment introducing them, e.g.
> "Check that the payload size is within bounds."

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:84: static_cast<unsigned>(compressed_size
- sizeof(uint32_t));
On 2016/03/29 00:45:39, Wez wrote:
> unsigned what?
> 
> What about using a safe type-conversion to get compressed_size from |int| to
> |size_t| and then doing the -= on that?

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:87: int err = inflate(&zlib_stream_,
Z_SYNC_FLUSH);
On 2016/03/29 00:45:39, Wez wrote:
> nit: |inflate_result|

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:95: if (zlib_stream_.avail_in > 0) {
On 2016/03/29 00:45:40, Wez wrote:
> This provides us a mechanism for telling when our buffer was too small; we
could
> e.g. double the decompressed-data buffer in such situations, allowing us to
try
> decompressing into the buffer and resize it if there wasn't enough space,
> removing the need for the explicit decompressed-size header. WDYT?

I was on the fence about this. I favored the simplicity of this approach
initially, but now that I have a stable solution and better understand how zlib
works, I can go with the resizing route.

Added a TODO that I'll address in an upcoming patch.

[Kevin M, 2016-03-29 20:36:24]

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:95: if (zlib_stream_.avail_in > 0) {
Done. Removed the payload header size. Instead of dynamically resizing the
buffer, I am simply sizing the buffer up to the payload size limit.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.h (right):

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:37: void ReadCompressedPacketDone(
On 2016/03/29 00:45:40, Wez wrote:
> nit: OnCompressedPacketReceived or OnCompressedReadPacketComplete?
> 
> There's a general pattern of OnFoo() for callback/handler methods in classes,
in
> Chromium.

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:50: int compressed_size,
On 2016/03/29 00:45:40, Wez wrote:
> nit: Could you just pass the IOBuffer into which the compressed data was read,
> or if that doesn't have the right length then use StringPiece to fold the data
> pointer+size into a single parameter?
> 
> nit: Since this method already touches |zlib_stream_|, why not let it access
> |compressed_buf_| too, rather than pass the buffer in as ptr+size?

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:55: z_stream zlib_stream_;
On 2016/03/29 00:45:40, Wez wrote:
> nit: I'm not familiar with all the C++11 rules, but could this in-line
> initialize to zeros w/out memset() by using ... = {0}?

{} seems to do the trick.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:56:
base::WeakPtrFactory<CompressedPacketReader> weak_factory_;
On 2016/03/29 00:45:40, Wez wrote:
> nit: Suggest a comment preceding |weak_factory_|, e.g. "Used to abandon
pending
> ReadPacket() calls on teardown.)

Done.

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
File blimp/net/compressed_packet_unittest.cc (right):

https://codereview.chromium.org/1825263003/diff/140001/blimp/net/compressed_p...
blimp/net/compressed_packet_unittest.cc:39: DCHECK_EQ(0, dest_buf->offset());
On 2016/03/29 00:45:40, Wez wrote:
> Don't we use ASSERT in tests, rather than DCHECK?

Done.

[Wez, 2016-03-29 21:38:38]

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:81: decompressed->RemainingCapacity() -
zlib_stream_.avail_out;
Maybe DCHECK that RemainingCapacity() > zlib_stream_.avail_out first?

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:84: // buffer.
Update this comment.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:54: z_stream zlib_stream_{};
Is this valid syntax? Never seen this form before!

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:30: int err = deflateInit2(&zlib_stream_,
Z_BEST_COMPRESSION, Z_DEFLATED,
nit: zlib_error or init_error?

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:37: deflateEnd(&zlib_stream_);
You're calling deflateEnd() here but only DCHECK()ing the result of init, so a
release build may end up in a weird state? Should we just CHECK the OK result
from init?

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:41: const
scoped_refptr<net::DrainableIOBuffer>& buf,
nit: buf & cb should have style-guide-compliant names, please!

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:45: size_t uncompressed_size =
static_cast<size_t>(buf->BytesRemaining());
Use a checked cast here (see the header in base/).

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:48: // Zero-length input => zero-length
output.
nit: Move this outside the if block, and add a comment on the max-payload if
block below?

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:63: compressed_size_total_ +=
compress_result;
Do we log these in non-DVLOG enabled builds? If not then suggest wrapping code
touching them in #if !defined(NDEBUG)

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:77: DCHECK_EQ(dest_buf->offset(), 0);
nit: Prefer DCHECK_EQ(expected, actual)

nit: Follow pre-condition DCHECK w/ a blank line to make preconditions vs
actually does-work code separation obvious.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:88: int err = deflate(&zlib_stream_,
Z_SYNC_FLUSH);
See request above re naming.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_writer.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:23: // decompressed payload, used for
sizing output buffers.
Looks like you got rid of that in the latest patch?

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:38: // Compresses |src_buf| into
|dest_buf|, and sets |compressed_size| with the
There is no |compressed_size| - you mean it returns the length?

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:39: // length of the compressed packet
(including decompressed length).
Not sure what "including decompressed length means"?

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:50: size_t uncompressed_size_total_ = 0;
nit: Do these need to be unsigned literals?

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/test_common.h
File blimp/net/test_common.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/test_common....
blimp/net/test_common.h:74: bool BufferStartsWith(net::GrowableIOBuffer* buf,
nit: Make this a const&?

[Kevin M, 2016-03-29 23:48:24]

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:81: decompressed->RemainingCapacity() -
zlib_stream_.avail_out;
On 2016/03/29 21:38:37, Wez wrote:
> Maybe DCHECK that RemainingCapacity() > zlib_stream_.avail_out first?

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:84: // buffer.
On 2016/03/29 21:38:37, Wez wrote:
> Update this comment.

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:54: z_stream zlib_stream_{};
On 2016/03/29 21:38:38, Wez wrote:
> Is this valid syntax? Never seen this form before!

"brace-init", new in C++11. Empty braces imply default initialization, which
zero out the values in a POD struct.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:30: int err = deflateInit2(&zlib_stream_,
Z_BEST_COMPRESSION, Z_DEFLATED,
On 2016/03/29 21:38:38, Wez wrote:
> nit: zlib_error or init_error?

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:37: deflateEnd(&zlib_stream_);
On 2016/03/29 21:38:38, Wez wrote:
> You're calling deflateEnd() here but only DCHECK()ing the result of init, so a
> release build may end up in a weird state? Should we just CHECK the OK result
> from init?

Good point, done

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:41: const
scoped_refptr<net::DrainableIOBuffer>& buf,
On 2016/03/29 21:38:38, Wez wrote:
> nit: buf & cb should have style-guide-compliant names, please!

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:41: const
scoped_refptr<net::DrainableIOBuffer>& buf,
On 2016/03/29 21:38:38, Wez wrote:
> nit: buf & cb should have style-guide-compliant names, please!

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:45: size_t uncompressed_size =
static_cast<size_t>(buf->BytesRemaining());
On 2016/03/29 21:38:38, Wez wrote:
> Use a checked cast here (see the header in base/).

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:48: // Zero-length input => zero-length
output.
On 2016/03/29 21:38:38, Wez wrote:
> nit: Move this outside the if block, and add a comment on the max-payload if
> block below?

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:48: // Zero-length input => zero-length
output.
On 2016/03/29 21:38:38, Wez wrote:
> nit: Move this outside the if block, and add a comment on the max-payload if
> block below?

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:63: compressed_size_total_ +=
compress_result;
On 2016/03/29 21:38:38, Wez wrote:
> Do we log these in non-DVLOG enabled builds? If not then suggest wrapping code
> touching them in #if !defined(NDEBUG)

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:63: compressed_size_total_ +=
compress_result;
On 2016/03/29 21:38:38, Wez wrote:
> Do we log these in non-DVLOG enabled builds? If not then suggest wrapping code
> touching them in #if !defined(NDEBUG)

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:77: DCHECK_EQ(dest_buf->offset(), 0);
On 2016/03/29 21:38:38, Wez wrote:
> nit: Prefer DCHECK_EQ(expected, actual)
> 
> nit: Follow pre-condition DCHECK w/ a blank line to make preconditions vs
> actually does-work code separation obvious.

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:88: int err = deflate(&zlib_stream_,
Z_SYNC_FLUSH);
On 2016/03/29 21:38:38, Wez wrote:
> See request above re naming.

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_writer.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:23: // decompressed payload, used for
sizing output buffers.
On 2016/03/29 21:38:38, Wez wrote:
> Looks like you got rid of that in the latest patch?

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:38: // Compresses |src_buf| into
|dest_buf|, and sets |compressed_size| with the
On 2016/03/29 21:38:38, Wez wrote:
> There is no |compressed_size| - you mean it returns the length?

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:39: // length of the compressed packet
(including decompressed length).
On 2016/03/29 21:38:38, Wez wrote:
> Not sure what "including decompressed length means"?

I meant "decompressed length header." But that's N/A now.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:50: size_t uncompressed_size_total_ = 0;
On 2016/03/29 21:38:38, Wez wrote:
> nit: Do these need to be unsigned literals?

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.h:50: size_t uncompressed_size_total_ = 0;
On 2016/03/29 21:38:38, Wez wrote:
> nit: Do these need to be unsigned literals?

Done.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/test_common.h
File blimp/net/test_common.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/test_common....
blimp/net/test_common.h:74: bool BufferStartsWith(net::GrowableIOBuffer* buf,
On 2016/03/29 21:38:38, Wez wrote:
> nit: Make this a const&?

Can't; IOBuffer::data() is non-const, so I can't pass it as a const reference.

As non-const references aren't allowed (according to lint), pointer it is,
then...?

[Wez, 2016-03-29 23:59:15]

lgtm

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:54: z_stream zlib_stream_{};
On 2016/03/29 23:48:23, Kevin M wrote:
> On 2016/03/29 21:38:38, Wez wrote:
> > Is this valid syntax? Never seen this form before!
> 
> "brace-init", new in C++11. Empty braces imply default initialization, which
> zero out the values in a POD struct.

Poo :( Looks like that language feature is currently on the banned list for
Chromium code.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/test_common.h
File blimp/net/test_common.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/test_common....
blimp/net/test_common.h:74: bool BufferStartsWith(net::GrowableIOBuffer* buf,
On 2016/03/29 23:48:24, Kevin M wrote:
> On 2016/03/29 21:38:38, Wez wrote:
> > nit: Make this a const&?
> 
> Can't; IOBuffer::data() is non-const, so I can't pass it as a const reference.
> 
> As non-const references aren't allowed (according to lint), pointer it is,
> then...?

That's a shame; I wonder why there is no const override?

https://codereview.chromium.org/1825263003/diff/200001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1825263003/diff/200001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:81:
static_cast<int>(zlib_stream_.avail_out));
Use a checked cast here, otherwise some crazy scenario could trigger avail_out
to be >2GB and this would trivially pass.

https://codereview.chromium.org/1825263003/diff/200001/blimp/net/compressed_p...
File blimp/net/compressed_packet_writer.cc (right):

https://codereview.chromium.org/1825263003/diff/200001/blimp/net/compressed_p...
blimp/net/compressed_packet_writer.cc:35: CHECK_EQ(init_error, Z_OK);
nit: CHECK_EQ(expected, result)

[Kevin M, 2016-03-30 00:26:33]

Will land CL when I can verify these latest changes e2e on a client device.

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.h (right):

https://codereview.chromium.org/1825263003/diff/180001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.h:54: z_stream zlib_stream_{};
On 2016/03/29 23:59:15, Wez wrote:
> On 2016/03/29 23:48:23, Kevin M wrote:
> > On 2016/03/29 21:38:38, Wez wrote:
> > > Is this valid syntax? Never seen this form before!
> > 
> > "brace-init", new in C++11. Empty braces imply default initialization, which
> > zero out the values in a POD struct.
> 
> Poo :( Looks like that language feature is currently on the banned list for
> Chromium code.

Back to memset. :(

https://codereview.chromium.org/1825263003/diff/200001/blimp/net/compressed_p...
File blimp/net/compressed_packet_reader.cc (right):

https://codereview.chromium.org/1825263003/diff/200001/blimp/net/compressed_p...
blimp/net/compressed_packet_reader.cc:81:
static_cast<int>(zlib_stream_.avail_out));
On 2016/03/29 23:59:15, Wez wrote:
> Use a checked cast here, otherwise some crazy scenario could trigger avail_out
> to be >2GB and this would trivially pass.

Done.

[Kevin M, 2016-03-30 17:39:03]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-03-30 17:39:04]

The patchset sent to the CQ was uploaded after l-g-t-m from agl@chromium.org,
wez@chromium.org
Link to the patchset: https://codereview.chromium.org/1825263003/#ps220001
(title: "wez feedback")

[commit-bot: I haz the power, 2016-03-30 17:39:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1825263003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1825263003/220001

[commit-bot: I haz the power, 2016-03-30 18:30:20]

Description was changed from

==========
Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Integrate CompressedPacketReader/CompressedPacketWriter into application.
* Make PacketReader resize its input GrowableIOBuffer as needed.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145
==========

to

==========
Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Integrate CompressedPacketReader/CompressedPacketWriter into application.
* Make PacketReader resize its input GrowableIOBuffer as needed.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145
==========

[commit-bot: I haz the power, 2016-03-30 18:30:21]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-03-30 18:31:50]

Description was changed from

==========
Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Integrate CompressedPacketReader/CompressedPacketWriter into application.
* Make PacketReader resize its input GrowableIOBuffer as needed.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145
==========

to

==========
Blimp: add packet-level DEFLATE compression using zlib.

Compresses the packets in the stream as length-prefixed DEFLATE blocks.
Blocks are transmitted in a single, long-lived stream with a
compression window that is retained across consecutive compression
calls.
* Create new classes: CompressedPacketReader and CompressedPacketWriter.
* Integrate CompressedPacketReader/CompressedPacketWriter into application.
* Make PacketReader resize its input GrowableIOBuffer as needed.
* Add third_party/zlib DEP.

R=wez@chromium.org,agl@chromium.org
BUG=597145

Committed: https://crrev.com/38ad3ca2674316ec799b678a69fe3a7bfd5f0edf
Cr-Commit-Position: refs/heads/master@{#384029}
==========

[commit-bot: I haz the power, 2016-03-30 18:31:51]

Patchset 12 (id:??) landed as
https://crrev.com/38ad3ca2674316ec799b678a69fe3a7bfd5f0edf
Cr-Commit-Position: refs/heads/master@{#384029}