Implement suspend / resume in NewWebSocketChannelImpl.

Implement suspend / resume in NewWebSocketChannelImpl.

Now NewWebSocketChannel can be suspended and be resumed.

BUG=275459
R=tyoshino, abarth

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157665

[tyoshino (SeeGerritForStatus), 2013-09-10 09:39:29]

https://codereview.chromium.org/23464050/diff/3001/Source/modules/websockets/...
File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right):

https://codereview.chromium.org/23464050/diff/3001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:72: Active,
could you consider moving state to the channel?

it's good that you created this inner class, but it looks it might be cleaner to
let the channel have the state.

[yhirano, 2013-09-10 12:25:17]

https://codereview.chromium.org/23464050/diff/3001/Source/modules/websockets/...
File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right):

https://codereview.chromium.org/23464050/diff/3001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:72: Active,
On 2013/09/10 09:39:29, tyoshino wrote:
> could you consider moving state to the channel?
> 
> it's good that you created this inner class, but it looks it might be cleaner
to
> let the channel have the state.

I divided the state to two booleans: NewWebSocketChannelImpl::m_isSuspended and
Resumer::m_isAborted.

[abarth-chromium, 2013-09-10 16:24:14]

LGTM

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right):

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:71: struct PendingEvent {
I would move this class out from inside Resumer into the anonymous namespace at
the top of the file.  That's simpler than having three levels of nested classes.

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:89:
Resumer(NewWebSocketChannelImpl*);
Please mark one-argument constructors explicit.

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:98: class
PendingEventProcessor : public RefCounted<PendingEventProcessor> {
I'd also move this class into the anonymous namespace at the top of the file for
the same reason.

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:119:
NewWebSocketChannelImpl::Resumer::PendingEvent::PendingEvent(Type type,
Vector<char>* data) : type(type), closingCode(0)
These initializers go on separate lines.

[yhirano, 2013-09-12 01:35:49]

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right):

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:71: struct PendingEvent {
On 2013/09/10 16:24:15, abarth wrote:
> I would move this class out from inside Resumer into the anonymous namespace
at
> the top of the file.  That's simpler than having three levels of nested
classes.

Done.

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:89:
Resumer(NewWebSocketChannelImpl*);
On 2013/09/10 16:24:15, abarth wrote:
> Please mark one-argument constructors explicit.

Done.

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:98: class
PendingEventProcessor : public RefCounted<PendingEventProcessor> {
On 2013/09/10 16:24:15, abarth wrote:
> I'd also move this class into the anonymous namespace at the top of the file
for
> the same reason.

Done.

https://codereview.chromium.org/23464050/diff/9001/Source/modules/websockets/...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:119:
NewWebSocketChannelImpl::Resumer::PendingEvent::PendingEvent(Type type,
Vector<char>* data) : type(type), closingCode(0)
On 2013/09/10 16:24:15, abarth wrote:
> These initializers go on separate lines.

Done.

[tyoshino (SeeGerritForStatus), 2013-09-12 05:27:09]

lgtm

https://codereview.chromium.org/23464050/diff/24001/Source/modules/websockets...
File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right):

https://codereview.chromium.org/23464050/diff/24001/Source/modules/websockets...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:210:
m_channel->flowControlIfNecessary();
these two operations look unnecessary.

https://codereview.chromium.org/23464050/diff/24001/Source/modules/websockets...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:339: if
(m_receivingMessageData.size() > 0) {
wow. not an issue of this CL, but this should be messageData->size() ?

let's fix it in a separate CL and add a unittest containing non-ASCII input.

[yhirano, 2013-09-12 06:02:08]

https://codereview.chromium.org/23464050/diff/24001/Source/modules/websockets...
File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right):

https://codereview.chromium.org/23464050/diff/24001/Source/modules/websockets...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:210:
m_channel->flowControlIfNecessary();
On 2013/09/12 05:27:10, tyoshino wrote:
> these two operations look unnecessary.

Done.

https://codereview.chromium.org/23464050/diff/24001/Source/modules/websockets...
Source/modules/websockets/NewWebSocketChannelImpl.cpp:339: if
(m_receivingMessageData.size() > 0) {
On 2013/09/12 05:27:10, tyoshino wrote:
> wow. not an issue of this CL, but this should be messageData->size() ?
> 
> let's fix it in a separate CL and add a unittest containing non-ASCII input.

You are right.
By the way, I confirmed that String::fromUTF8 returns an empty string if
messageData->size() is zero.
So I deleted the branch.

[commit-bot: I haz the power, 2013-09-12 06:28:59]

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

[commit-bot: I haz the power, 2013-09-12 07:33:20]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-09-12 07:34:58]

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

[commit-bot: I haz the power, 2013-09-12 08:17:06]

Change committed as 157665