Introduce WebSocketDeflatePredictor.

Introduce WebSocketDeflatePredictor.

Introduce WebSocketDeflatePredictor, an interface class which determines
whether a WebSocketDeflateStream should compress the given message or
not.

BUG=163882
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=231827

[Adam Rice, 2013-10-24 07:59:23]

https://codereview.chromium.org/39193005/diff/30001/net/websockets/websocket_...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/30001/net/websockets/websocket_...
net/websockets/websocket_deflate_predictor.h:38: // frames.
Maybe |target_frame| should also be passed as an index? I am not sure because it
depends on the implementation.

https://codereview.chromium.org/39193005/diff/30001/net/websockets/websocket_...
net/websockets/websocket_deflate_predictor.h:79: return DoNotDeflate;
DO_NOT_DEFLATE ?

[yhirano, 2013-10-25 07:51:37]

PTAL

[Adam Rice, 2013-10-25 13:05:24]

Early comments (I haven't got to the end yet).

https://codereview.chromium.org/39193005/diff/190001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/190001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:39: // but future frames may
contain control message frames.
You might want to mention that if frame_index is the last frame, then
frames[frame_index + 1] won't exist. But that is pretty obvious, so maybe not
worth saying.

https://codereview.chromium.org/39193005/diff/190001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor_impl.h (right):

https://codereview.chromium.org/39193005/diff/190001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl.h:19: // a
WebSocketDeflateStream should compress a message or not.
This comment is wrong.

[yhirano, 2013-10-28 01:42:45]

https://codereview.chromium.org/39193005/diff/190001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/190001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:39: // but future frames may
contain control message frames.
On 2013/10/25 13:05:25, Adam Rice wrote:
> You might want to mention that if frame_index is the last frame, then
> frames[frame_index + 1] won't exist. But that is pretty obvious, so maybe not
> worth saying.
I intended to use Python notation which will work for |a[len(a):]|.
Done.

https://codereview.chromium.org/39193005/diff/190001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor_impl.h (right):

https://codereview.chromium.org/39193005/diff/190001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl.h:19: // a
WebSocketDeflateStream should compress a message or not.
On 2013/10/25 13:05:25, Adam Rice wrote:
> This comment is wrong.

Thanks, done.

[Adam Rice, 2013-10-29 04:11:41]

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:46: virtual void
RecordProcessedDataFrame(const WebSocketFrame* frame) = 0;
I assume this should be called after Predict(), so that for each data frame the
caller will run

Predict()
RecordProcessedDataFrame()
RecordSentDataFrame()

in that order.

I'm not sure what "Processed" means in the name of this method. I would expect
it to be "Raw", ie. before applying any compression or other transformations.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:52: virtual void
RecordSentDataFrame(const WebSocketFrame* frame) = 0;
It's ambiguous whether this is supposed to be called for the final frame that is
sent over the TCP/IP connection (including any other extensions applied like
multiplexing), or only for the output of the deflate extension.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream.cc (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:164: }
It appears to me that if writing_state_ is WRITING_POSSIBLE_COMPRESSED_MESSAGE
and a final frame is not seen, then all the frames will be lost.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:207: compressed->header.opcode =
opcode;
"opcode" has been set by the constructor already!

Also, this is almost identical to the similar chunk of code in
AppendPossiblyCompressedMessage and could probably be factored out into a common
function.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:237: // Many asserts checking that
frames represent one data message.
"one whole data message"

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:240:
DCHECK_NE(WebSocketFrameHeader::kOpCodeContinuation,
It's a bit untidy to have an if() statement that does nothing at all in an
official build. It might even cause compile errors. How about doing something
like

DCHECK(i != 0, WebSocketFrameHeader::kOpCodeContinuation ==
frame->header.opcode);

?

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream_test.cc (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:259: for (size_t i = 0; i <
frames->size(); ++i) {
It looks like you could just do frames_->insert(frames_->end(), frames->begin(),
frames->end())

This would also make it clearer that you are preserving the existing content of
frames_.

[yhirano, 2013-10-29 06:11:05]

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:46: virtual void
RecordProcessedDataFrame(const WebSocketFrame* frame) = 0;
On 2013/10/29 04:11:42, Adam Rice wrote:
> I assume this should be called after Predict(), so that for each data frame
the
> caller will run
> 
> Predict()
> RecordProcessedDataFrame()
> RecordSentDataFrame()
> 
> in that order.
> 
Predict will be called only for the first frame of each data message.

> I'm not sure what "Processed" means in the name of this method. I would expect
> it to be "Raw", ie. before applying any compression or other transformations.
s/Processed/Input/g

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:52: virtual void
RecordSentDataFrame(const WebSocketFrame* frame) = 0;
On 2013/10/29 04:11:42, Adam Rice wrote:
> It's ambiguous whether this is supposed to be called for the final frame that
is
> sent over the TCP/IP connection (including any other extensions applied like
> multiplexing), or only for the output of the deflate extension.
s/Sent/Written/g
and
add "by the stream"

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream.cc (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:164: }
On 2013/10/29 04:11:42, Adam Rice wrote:
> It appears to me that if writing_state_ is WRITING_POSSIBLE_COMPRESSED_MESSAGE
> and a final frame is not seen, then all the frames will be lost.

Yes, definitely.
So the comment of WebSocketDeflatePredictor::TRY_DEFALTE says
 // Returning this result implies that the deflater is running on
 // DoNotTakeOverContext mode and the entire message is visible.
and it is checked at L166.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:207: compressed->header.opcode =
opcode;
On 2013/10/29 04:11:42, Adam Rice wrote:
> "opcode" has been set by the constructor already!
and overwritten by CopyFrom function.

> Also, this is almost identical to the similar chunk of code in
> AppendPossiblyCompressedMessage and could probably be factored out into a
common
> function.
Hmm... It is similar, but not same.
It is difficult to share such ad-hoc flag settings code I think.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:237: // Many asserts checking that
frames represent one data message.
On 2013/10/29 04:11:42, Adam Rice wrote:
> "one whole data message"

Done.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:240:
DCHECK_NE(WebSocketFrameHeader::kOpCodeContinuation,
On 2013/10/29 04:11:42, Adam Rice wrote:
> It's a bit untidy to have an if() statement that does nothing at all in an
> official build. It might even cause compile errors. How about doing something
> like
> 
> DCHECK(i != 0, WebSocketFrameHeader::kOpCodeContinuation ==
> frame->header.opcode);
> 
> ?

Done.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream_test.cc (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:259: for (size_t i = 0; i <
frames->size(); ++i) {
On 2013/10/29 04:11:42, Adam Rice wrote:
> It looks like you could just do frames_->insert(frames_->end(),
frames->begin(),
> frames->end())
> 
> This would also make it clearer that you are preserving the existing content
of
> frames_.

Done.

[Adam Rice, 2013-10-30 00:28:07]

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:46: virtual void
RecordProcessedDataFrame(const WebSocketFrame* frame) = 0;
On 2013/10/29 06:11:06, yhirano wrote:
> On 2013/10/29 04:11:42, Adam Rice wrote:
> > I assume this should be called after Predict(), so that for each data frame
> the
> > caller will run
> > 
> > Predict()
> > RecordProcessedDataFrame()
> > RecordSentDataFrame()
> > 
> > in that order.
> > 
> Predict will be called only for the first frame of each data message.
> 
> > I'm not sure what "Processed" means in the name of this method. I would
expect
> > it to be "Raw", ie. before applying any compression or other
transformations.
> s/Processed/Input/g
> 

Is it okay to call this method before Predict() for the same frame? If not, I
think the comment should say so.

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream.cc (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.cc:207: compressed->header.opcode =
opcode;
On 2013/10/29 06:11:06, yhirano wrote:
> On 2013/10/29 04:11:42, Adam Rice wrote:
> > "opcode" has been set by the constructor already!
> and overwritten by CopyFrom function.

Sorry!

> > Also, this is almost identical to the similar chunk of code in
> > AppendPossiblyCompressedMessage and could probably be factored out into a
> common
> > function.
> Hmm... It is similar, but not same.
> It is difficult to share such ad-hoc flag settings code I think.

Okay.

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:50: // All written data frames by
the stream must be recorded in order
"data frames written by the stream"

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream_test.cc (right):

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:117: 
Please add a comment that these implement the WebSocketDeflatePredictor
interface

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:133: 
Please insert a comment here that the following methods are for setting or
checking expectations.

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:164: std::deque<const
WebSocketFrame*> frames_to_be_input_;
If I understand correctly, these member variables represent expectations of what
will be input/written to the DeflatePredirector. Could you add comments to the
member variables to make that clear?

[yhirano, 2013-10-30 02:20:33]

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/300001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:46: virtual void
RecordProcessedDataFrame(const WebSocketFrame* frame) = 0;
On 2013/10/30 00:28:07, Adam Rice wrote:
> On 2013/10/29 06:11:06, yhirano wrote:
> > On 2013/10/29 04:11:42, Adam Rice wrote:
> > > I assume this should be called after Predict(), so that for each data
frame
> > the
> > > caller will run
> > > 
> > > Predict()
> > > RecordProcessedDataFrame()
> > > RecordSentDataFrame()
> > > 
> > > in that order.
> > > 
> > Predict will be called only for the first frame of each data message.
> > 
> > > I'm not sure what "Processed" means in the name of this method. I would
> expect
> > > it to be "Raw", ie. before applying any compression or other
> transformations.
> > s/Processed/Input/g
> > 
> 
> Is it okay to call this method before Predict() for the same frame? If not, I
> think the comment should say so.
I added |Predict| comment.

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:50: // All written data frames by
the stream must be recorded in order
On 2013/10/30 00:28:07, Adam Rice wrote:
> "data frames written by the stream"

Done.

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream_test.cc (right):

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:117: 
On 2013/10/30 00:28:07, Adam Rice wrote:
> Please add a comment that these implement the WebSocketDeflatePredictor
> interface

Done.

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:133: 
On 2013/10/30 00:28:07, Adam Rice wrote:
> Please insert a comment here that the following methods are for setting or
> checking expectations.

Done.

https://codereview.chromium.org/39193005/diff/670001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:164: std::deque<const
WebSocketFrame*> frames_to_be_input_;
On 2013/10/30 00:28:07, Adam Rice wrote:
> If I understand correctly, these member variables represent expectations of
what
> will be input/written to the DeflatePredirector. Could you add comments to the
> member variables to make that clear?

Done.

[Adam Rice, 2013-10-30 02:56:52]

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:9: #include
"base/memory/scoped_ptr.h"
Not actually used.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor_impl.cc (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl.cc:6: #include
"base/memory/scoped_ptr.h"
Not used. base/basictypes.h and base/memory/scoped_vector.h are not yet used in
a way that exceeds how they are used in the header, so including them again
seems unnecessary, at least until this file has more implementation.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl.cc:8: #include
"net/websockets/websocket_deflate_predictor_impl.h"
This header file should go at the top.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor_impl.h (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl.h:9: #include
"base/memory/scoped_ptr.h"
Not used.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor_impl_test.cc (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl_test.cc:6: #include
"base/memory/scoped_ptr.h"
Not used.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl_test.cc:8: #include
"net/websockets/websocket_deflate_predictor_impl.h"
This should go at the top in my opinion, since it is the specific thing that
this file tests.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream.h (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.h:15: #include
"net/websockets/websocket_deflate_predictor.h"
WebSocketDeflatePredictor can be pre-declared rather than including the header
file.

[yhirano, 2013-10-30 03:19:47]

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor.h (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor.h:9: #include
"base/memory/scoped_ptr.h"
On 2013/10/30 02:56:53, Adam Rice wrote:
> Not actually used.

Done.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor_impl.cc (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl.cc:6: #include
"base/memory/scoped_ptr.h"
On 2013/10/30 02:56:53, Adam Rice wrote:
> Not used. base/basictypes.h and base/memory/scoped_vector.h are not yet used
in
> a way that exceeds how they are used in the header, so including them again
> seems unnecessary, at least until this file has more implementation.

Done.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl.cc:8: #include
"net/websockets/websocket_deflate_predictor_impl.h"
On 2013/10/30 02:56:53, Adam Rice wrote:
> This header file should go at the top.

Done.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor_impl.h (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl.h:9: #include
"base/memory/scoped_ptr.h"
On 2013/10/30 02:56:53, Adam Rice wrote:
> Not used.

Done.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_predictor_impl_test.cc (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl_test.cc:6: #include
"base/memory/scoped_ptr.h"
On 2013/10/30 02:56:53, Adam Rice wrote:
> Not used.

Done.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_predictor_impl_test.cc:8: #include
"net/websockets/websocket_deflate_predictor_impl.h"
On 2013/10/30 02:56:53, Adam Rice wrote:
> This should go at the top in my opinion, since it is the specific thing that
> this file tests.

Done.

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream.h (right):

https://codereview.chromium.org/39193005/diff/790001/net/websockets/websocket...
net/websockets/websocket_deflate_stream.h:15: #include
"net/websockets/websocket_deflate_predictor.h"
On 2013/10/30 02:56:53, Adam Rice wrote:
> WebSocketDeflatePredictor can be pre-declared rather than including the header
> file.

Done.

[Adam Rice, 2013-10-30 03:51:34]

lgtm

[yhirano, 2013-10-30 03:54:57]

+tyoshino for net/websockets OWNER review.
+szym for net/net.gyp OWNER review.

[szym, 2013-10-30 04:09:35]

LGTM

Is there a runtime (non-unittest) need for the interface-impl separation?

[yhirano, 2013-10-30 04:16:26]

> Is there a runtime (non-unittest) need for the interface-impl separation?

No, WebSocketDeflatePredictorImpl is the only subclass of
WebSocketDeflatePredictor except classes in unittests.

[szym, 2013-10-30 04:28:08]

ok, lgtm, don't forget to update CL description

https://codereview.chromium.org/39193005/diff/960001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream_test.cc (right):

https://codereview.chromium.org/39193005/diff/960001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:178: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/39193005/diff/960001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:195: // |mock_stream_| will be
deleted when |deflate_stream_| is destroyed.
Minor suggestion: "// Owned by |deflate_stream_|." would suffice

[yhirano, 2013-10-30 04:36:35]

https://codereview.chromium.org/39193005/diff/960001/net/websockets/websocket...
File net/websockets/websocket_deflate_stream_test.cc (right):

https://codereview.chromium.org/39193005/diff/960001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:178: };
On 2013/10/30 04:28:08, szym wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/39193005/diff/960001/net/websockets/websocket...
net/websockets/websocket_deflate_stream_test.cc:195: // |mock_stream_| will be
deleted when |deflate_stream_| is destroyed.
On 2013/10/30 04:28:08, szym wrote:
> Minor suggestion: "// Owned by |deflate_stream_|." would suffice

Done.

[tyoshino (SeeGerritForStatus), 2013-10-30 06:15:03]

lgtm

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
File net/websockets/websocket_deflate_predictor_impl_test.cc (right):

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_predictor_impl_test.cc:24:
ASSERT_EQ(WebSocketDeflatePredictor::DEFLATE, result);
EXPECT?

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
File net/websockets/websocket_deflate_stream.cc (right):

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream.cc:108: ScopedVector<WebSocketFrame>
frames_of_message;
this postpones sending the initial frame of the message. add note about that.

Frame1/2ofMessage ControlFrame Frame2/2ofMessage
->
ControlFrame Message

The order of timing of message receive completion is the same.
The order of timing of start of message receive is different.

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream.cc:198: scoped_refptr<IOBufferWithSize>
data =
s/data/compressed_payload/

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream.cc:226: scoped_refptr<IOBufferWithSize>
data =
s/data/compressed_payload/

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
File net/websockets/websocket_deflate_stream_test.cc (right):

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream_test.cc:130: }
can you try not to use check but ADD_FAILURE() etc.?

check fail aborts all test cases, so sometimes annoying.

it's ok to do it later.

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream_test.cc:151:
frames_written_.pop_front();
ditto

[yhirano, 2013-10-30 09:32:16]

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
File net/websockets/websocket_deflate_predictor_impl_test.cc (right):

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_predictor_impl_test.cc:24:
ASSERT_EQ(WebSocketDeflatePredictor::DEFLATE, result);
On 2013/10/30 06:15:04, tyoshino wrote:
> EXPECT?

Done.

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
File net/websockets/websocket_deflate_stream.cc (right):

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream.cc:108: ScopedVector<WebSocketFrame>
frames_of_message;
On 2013/10/30 06:15:04, tyoshino wrote:
> this postpones sending the initial frame of the message. add note about that.
> 
> Frame1/2ofMessage ControlFrame Frame2/2ofMessage
> ->
> ControlFrame Message
> 
> The order of timing of message receive completion is the same.
> The order of timing of start of message receive is different.

This is not specific to WRITING_POSSIBLY_COMPRESSED, but a generic
characteristic of WebSocketDeflateStream.
I added the class comment.

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream.cc:198: scoped_refptr<IOBufferWithSize>
data =
On 2013/10/30 06:15:04, tyoshino wrote:
> s/data/compressed_payload/

Done.

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream.cc:226: scoped_refptr<IOBufferWithSize>
data =
On 2013/10/30 06:15:04, tyoshino wrote:
> s/data/compressed_payload/

Done.

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
File net/websockets/websocket_deflate_stream_test.cc (right):

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream_test.cc:130: }
On 2013/10/30 06:15:04, tyoshino wrote:
> can you try not to use check but ADD_FAILURE() etc.?
> 
> check fail aborts all test cases, so sometimes annoying.
> 
> it's ok to do it later.

Done.

https://codereview.chromium.org/39193005/diff/1060001/net/websockets/websocke...
net/websockets/websocket_deflate_stream_test.cc:151:
frames_written_.pop_front();
On 2013/10/30 06:15:04, tyoshino wrote:
> ditto

Done.

[commit-bot: I haz the power, 2013-10-30 11:31:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/39193005/1120001

[commit-bot: I haz the power, 2013-10-30 14:28:25]

Change committed as 231827