Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22914026:
[ABANDONED] Introduce blink-side bridges for the new WebSocket implementation. (Closed)
|
Created:
7 years, 4 months ago by yhirano Modified:
7 years, 2 months ago CC:
blink-reviews, jamesr, dglazkov+blink, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionIntroduce blink-side bridges for the new WebSocket implementation.
We are implementing the new WebSocket implementation.
This CL introduces WebSocketHandle and WebSocketHandleClient,
which are the bridge classes between WebCore and webkit_glue classes.
NewWebSocketChannelHandle is also introduced, which is a WebSocketChannel derived class
for WebSocketChannelHandle.
These classes will be tested when content and chrome implementation is done.
Currently this cl doesn't change any behavior because nobody uses the classes yet.
BUG=275459
R=ricea, tyoshino, tkent
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 6
Patch Set 9 : #
Total comments: 4
Patch Set 10 : #
Total comments: 8
Patch Set 11 : Use a timer in NewWebSocketChannelImpl::resume #Patch Set 12 : rebase #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #
Total comments: 11
Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #
Total comments: 16
Patch Set 19 : #Patch Set 20 : #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/22914026/diff/2001/Source/modules/websockets/... File Source/modules/websockets/NewWebSocketChannelImpl.h (right): https://codereview.chromium.org/22914026/diff/2001/Source/modules/websockets/... Source/modules/websockets/NewWebSocketChannelImpl.h:58: static PassRefPtr<NewWebSocketChannelImpl> create(ScriptExecutionContext* context, WebSocketChannelClient* client, const String& sourceURL = String(), unsigned lineNumber = 0) { return adoptRef(new NewWebSocketChannelImpl(context, client, sourceURL, lineNumber)); } let's break at { https://codereview.chromium.org/22914026/diff/2001/Source/modules/websockets/... Source/modules/websockets/NewWebSocketChannelImpl.h:148: // WebSocketChannel functions https://codereview.chromium.org/22914026/diff/2001/Source/modules/websockets/... Source/modules/websockets/NewWebSocketChannelImpl.h:182: #endif // WebSocketMessagePool_h correct comment
https://codereview.chromium.org/22914026/diff/2001/Source/modules/websockets/... File Source/modules/websockets/NewWebSocketChannelImpl.h (right): https://codereview.chromium.org/22914026/diff/2001/Source/modules/websockets/... Source/modules/websockets/NewWebSocketChannelImpl.h:58: static PassRefPtr<NewWebSocketChannelImpl> create(ScriptExecutionContext* context, WebSocketChannelClient* client, const String& sourceURL = String(), unsigned lineNumber = 0) { return adoptRef(new NewWebSocketChannelImpl(context, client, sourceURL, lineNumber)); } On 2013/08/22 08:25:22, tyoshino wrote: > let's break at { Done. https://codereview.chromium.org/22914026/diff/2001/Source/modules/websockets/... Source/modules/websockets/NewWebSocketChannelImpl.h:148: On 2013/08/22 08:25:22, tyoshino wrote: > // WebSocketChannel functions Done. https://codereview.chromium.org/22914026/diff/2001/Source/modules/websockets/... Source/modules/websockets/NewWebSocketChannelImpl.h:182: #endif // WebSocketMessagePool_h On 2013/08/22 08:25:22, tyoshino wrote: > correct comment Done.
https://codereview.chromium.org/22914026/diff/12001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.h (right): https://codereview.chromium.org/22914026/diff/12001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.h:156: ScriptExecutionContext* m_context; Is this raw pointer safe? Should this object be a ContextDestructionObserver? https://codereview.chromium.org/22914026/diff/12001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/22914026/diff/12001/public/platform/Platform.... public/platform/Platform.h:307: virtual WebSocketChannelHandle* createWebSocketChannelHandle() { return 0; } Is the word "channel" essential here? Can we just call this a WebSocketHandle? https://codereview.chromium.org/22914026/diff/12001/public/platform/WebSocket... File public/platform/WebSocketChannelHandle.h (right): https://codereview.chromium.org/22914026/diff/12001/public/platform/WebSocket... public/platform/WebSocketChannelHandle.h:55: virtual void connect(const WebURL& /* url */, const WebVector<WebString>& protocols, const WebString& origin, WebSocketChannelHandleClient*) = 0; No need for /* */ around argument names. Just put in the argument names.
https://codereview.chromium.org/22914026/diff/12001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.h (right): https://codereview.chromium.org/22914026/diff/12001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.h:156: ScriptExecutionContext* m_context; On 2013/08/22 19:39:07, abarth wrote: > Is this raw pointer safe? Should this object be a ContextDestructionObserver? WebCore::WebSocket is a ContextLifecycleObserver and its contextDestroyed ASSERTs !m_channel when it is called. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... So, in NewWebSocketChannelImpl, accessing m_context has no problem if m_state != Closed. Some statements violated it, so I fixed them. https://codereview.chromium.org/22914026/diff/12001/public/platform/Platform.h File public/platform/Platform.h (right): https://codereview.chromium.org/22914026/diff/12001/public/platform/Platform.... public/platform/Platform.h:307: virtual WebSocketChannelHandle* createWebSocketChannelHandle() { return 0; } On 2013/08/22 19:39:07, abarth wrote: > Is the word "channel" essential here? Can we just call this a WebSocketHandle? Done. https://codereview.chromium.org/22914026/diff/12001/public/platform/WebSocket... File public/platform/WebSocketChannelHandle.h (right): https://codereview.chromium.org/22914026/diff/12001/public/platform/WebSocket... public/platform/WebSocketChannelHandle.h:55: virtual void connect(const WebURL& /* url */, const WebVector<WebString>& protocols, const WebString& origin, WebSocketChannelHandleClient*) = 0; On 2013/08/22 19:39:07, abarth wrote: > No need for /* */ around argument names. Just put in the argument names. check-webkit-style complains as: The parameter name "url" adds no information, so it should be removed.
https://codereview.chromium.org/22914026/diff/23001/public/platform/WebSocket... File public/platform/WebSocketHandle.h (right): https://codereview.chromium.org/22914026/diff/23001/public/platform/WebSocket... public/platform/WebSocketHandle.h:56: virtual void send(const char* data, size_t /* size */, MessageType, bool fin) = 0; (type, data, fin) is better than (data, type, fin).
https://codereview.chromium.org/22914026/diff/23001/public/platform/WebSocket... File public/platform/WebSocketHandle.h (right): https://codereview.chromium.org/22914026/diff/23001/public/platform/WebSocket... public/platform/WebSocketHandle.h:56: virtual void send(const char* data, size_t /* size */, MessageType, bool fin) = 0; On 2013/08/23 06:57:13, yhirano wrote: > (type, data, fin) is better than (data, type, fin). Done.
I've just uploaded webkit_glue side change: https://codereview.chromium.org/22815034/
https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:111: m_handle->connect(url, webProtocols, m_context->securityOrigin()->toString(), this); TODO to remember porting callStack copying code? https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:173: m_handle->close(static_cast<unsigned short>(code), reason); closing handshake timeout will be handled by lower layer? https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.h (right): https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.h:161: unsigned long m_identifier; // m_identifier == 0 means that we could not obtain a valid identifier. could you add a comment and explain the purpose of m_identifier in it?
https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:111: m_handle->connect(url, webProtocols, m_context->securityOrigin()->toString(), this); On 2013/08/26 08:04:11, tyoshino wrote: > TODO to remember porting callStack copying code? Thanks, I added an implementation. https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:173: m_handle->close(static_cast<unsigned short>(code), reason); On 2013/08/26 08:04:11, tyoshino wrote: > closing handshake timeout will be handled by lower layer? Yes. https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.h (right): https://codereview.chromium.org/22914026/diff/31001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.h:161: unsigned long m_identifier; // m_identifier == 0 means that we could not obtain a valid identifier. On 2013/08/26 08:04:11, tyoshino wrote: > could you add a comment and explain the purpose of m_identifier in it? Done.
https://codereview.chromium.org/22914026/diff/38001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/38001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:355: if (type == WebKit::WebSocketHandle::MessageTypeText) { switch? https://codereview.chromium.org/22914026/diff/38001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:497: isClean(event.closingCode) ? WebSocketChannelClient::ClosingHandshakeComplete : WebSocketChannelClient::ClosingHandshakeIncomplete, let's define a variable for this argument.
https://codereview.chromium.org/22914026/diff/38001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/38001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:355: if (type == WebKit::WebSocketHandle::MessageTypeText) { On 2013/08/26 09:35:42, tyoshino wrote: > switch? Done. https://codereview.chromium.org/22914026/diff/38001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:497: isClean(event.closingCode) ? WebSocketChannelClient::ClosingHandshakeComplete : WebSocketChannelClient::ClosingHandshakeIncomplete, On 2013/08/26 09:35:42, tyoshino wrote: > let's define a variable for this argument. Done.
On 2013/08/23 01:35:13, yhirano wrote: > https://codereview.chromium.org/22914026/diff/12001/Source/modules/websockets... > File Source/modules/websockets/NewWebSocketChannelImpl.h (right): > > https://codereview.chromium.org/22914026/diff/12001/Source/modules/websockets... > Source/modules/websockets/NewWebSocketChannelImpl.h:156: ScriptExecutionContext* > m_context; > On 2013/08/22 19:39:07, abarth wrote: > > Is this raw pointer safe? Should this object be a ContextDestructionObserver? > > WebCore::WebSocket is a ContextLifecycleObserver and its contextDestroyed > ASSERTs !m_channel when it is called. But NewWebSocketChannelImpl is RefCounted, which means someone else could still hold a reference and the pointer would be dangling...
On 2013/08/26 19:22:33, abarth wrote: > On 2013/08/23 01:35:13, yhirano wrote: > > > https://codereview.chromium.org/22914026/diff/12001/Source/modules/websockets... > > File Source/modules/websockets/NewWebSocketChannelImpl.h (right): > > > > > https://codereview.chromium.org/22914026/diff/12001/Source/modules/websockets... > > Source/modules/websockets/NewWebSocketChannelImpl.h:156: > ScriptExecutionContext* > > m_context; > > On 2013/08/22 19:39:07, abarth wrote: > > > Is this raw pointer safe? Should this object be a > ContextDestructionObserver? > > > > WebCore::WebSocket is a ContextLifecycleObserver and its contextDestroyed > > ASSERTs !m_channel when it is called. > > But NewWebSocketChannelImpl is RefCounted, which means someone else could still > hold a reference and the pointer would be dangling... m_status == Closed holds when !m_channel in WebCore::WebSocket, because it calls m_channel->disconnect before setting null. So, in NewWebSocketChannelImpl, accessing m_context has no problem if m_state != Closed.
https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:163: } comment that buffer.slice copies contents. https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:308: m_messages.remove(0, i); do you think we can use WTF::Deque for m_messages? https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:351: LOG(Network, "NewWebSocketChannelImpl %p didReceiveData(%p, (%p, %zu), %d, %d)", this, handle, data.data(), data.size(), type, fin); match the order with one of the arguments this, handle, type, data... https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:376: if (fin) { if (!fin) return ...
https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:163: } On 2013/08/27 05:38:48, tyoshino wrote: > comment that buffer.slice copies contents. Done. https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:308: m_messages.remove(0, i); On 2013/08/27 05:38:48, tyoshino wrote: > do you think we can use WTF::Deque for m_messages? Thank you, done. https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:351: LOG(Network, "NewWebSocketChannelImpl %p didReceiveData(%p, (%p, %zu), %d, %d)", this, handle, data.data(), data.size(), type, fin); On 2013/08/27 05:38:48, tyoshino wrote: > match the order with one of the arguments > > this, handle, type, data... Done. https://codereview.chromium.org/22914026/diff/43001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:376: if (fin) { On 2013/08/27 05:38:48, tyoshino wrote: > if (!fin) > return > ... Done.
https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:368: ASSERT(!m_receivingMessageData.isEmpty()); it's possible that the first frame doesn't contain any data. if the new impl of lower layer can pass such a frame as-is to this class, we shouldn't have this ASSERT. otherwise, add some comment in .h saying that non-final empty messages must be coalesced. https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:384: if (m_receivingMessageTypeIsText) { could you try factoring out common code here and in processPendingEvents? https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:394: } else { ditto https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:420: WebSocketChannelClient* client = m_client; ditto
https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:279: size_t size = std::min(static_cast<size_t>(m_sendingQuota), message.text.length() - m_sentSizeOfTopMessage); If m_sendingQuota == message.text.length - m_sentSizeOfTopMessage, the loop breaks and two fin messages will be sent, which is wrong.
https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:279: size_t size = std::min(static_cast<size_t>(m_sendingQuota), message.text.length() - m_sentSizeOfTopMessage); If m_sendingQuota == message.text.length - m_sentSizeOfTopMessage, the loop breaks and two fin messages will be sent, which is wrong.
https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:279: size_t size = std::min(static_cast<size_t>(m_sendingQuota), message.text.length() - m_sentSizeOfTopMessage); On 2013/08/28 02:11:36, yhirano wrote: > If m_sendingQuota == message.text.length - m_sentSizeOfTopMessage, the loop > breaks and two fin messages will be sent, which is wrong. Done. https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:368: ASSERT(!m_receivingMessageData.isEmpty()); On 2013/08/27 09:15:39, tyoshino wrote: > it's possible that the first frame doesn't contain any data. if the new impl of > lower layer can pass such a frame as-is to this class, we shouldn't have this > ASSERT. otherwise, add some comment in .h saying that non-final empty messages > must be coalesced. As stated in websocket_messages.h[1], non-final frames cannot be empty. I added an ASSERT above. [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/common/web... https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:384: if (m_receivingMessageTypeIsText) { On 2013/08/27 09:15:39, tyoshino wrote: > could you try factoring out common code here and in processPendingEvents? Done. https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:394: } else { On 2013/08/27 09:15:39, tyoshino wrote: > ditto Done. https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:420: WebSocketChannelClient* client = m_client; On 2013/08/27 09:15:39, tyoshino wrote: > ditto Sorry, what does this comment mean?
https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/67001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:420: WebSocketChannelClient* client = m_client; On 2013/08/28 02:14:45, yhirano wrote: > On 2013/08/27 09:15:39, tyoshino wrote: > > ditto > Sorry, what does this comment mean? There's similar code at L523-525. So, I'd like you to think if it's better to factor out them. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:104: if (m_identifier) { ASSERT(m_context->isDocument()) If you think it's too much to put ASSERTs to each InspectorInstrumentation method call, please write a comment on m_identifier in .h file saying that if m_identifier is set, m_context is document. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:196: if (m_identifier) { ASSERT(m_context->isDocument()) https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:204: m_client->didReceiveMessageError(); is it possible that this happens between resume() call and resumeTimerFired() invocation? https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:219: if (m_identifier) { ASSERT(m_context->isDocument()) https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:227: m_handle = 0; is this ok? L409 expects m_handle to be non-NULL https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:406: if (m_identifier) { ASSERT(m_context->isDocument()) https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:460: ASSERT(m_pendingEvents.isEmpty()); what's the reason these conditions are met https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.h (right): https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.h:185: bool m_hasAlreadyFailed; say that this is set when - already failed - or fail event is already queued in m_pendingEvents
https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.cpp (right): https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:104: if (m_identifier) { On 2013/08/28 05:26:03, tyoshino wrote: > ASSERT(m_context->isDocument()) > > If you think it's too much to put ASSERTs to each InspectorInstrumentation > method call, > please write a comment on m_identifier in .h file saying that if m_identifier is > set, > m_context is document. Done. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:196: if (m_identifier) { On 2013/08/28 05:26:03, tyoshino wrote: > ASSERT(m_context->isDocument()) Done. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:204: m_client->didReceiveMessageError(); On 2013/08/28 05:26:03, tyoshino wrote: > is it possible that this happens between resume() call and resumeTimerFired() > invocation? You are right. I introduced m_isSuspendState instead of m_isSuspended. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:219: if (m_identifier) { On 2013/08/28 05:26:03, tyoshino wrote: > ASSERT(m_context->isDocument()) Done. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:227: m_handle = 0; On 2013/08/28 05:26:03, tyoshino wrote: > is this ok? > L409 expects m_handle to be non-NULL Please see L403. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:406: if (m_identifier) { On 2013/08/28 05:26:03, tyoshino wrote: > ASSERT(m_context->isDocument()) Done. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.cpp:460: ASSERT(m_pendingEvents.isEmpty()); On 2013/08/28 05:26:03, tyoshino wrote: > what's the reason these conditions are met It was wrong. Done. https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... File Source/modules/websockets/NewWebSocketChannelImpl.h (right): https://codereview.chromium.org/22914026/diff/80001/Source/modules/websockets... Source/modules/websockets/NewWebSocketChannelImpl.h:185: bool m_hasAlreadyFailed; On 2013/08/28 05:26:03, tyoshino wrote: > say that this is set when > - already failed > - or fail event is already queued in m_pendingEvents m_hasAlreadyFailed is set when the channel is already failed, but the notification can delay. I added comments for m_suspendState.
There's a bunch of stuff in this Cl that worries me. You've got a raw pointer to the ScriptExecutionContext and manual calls to ref() and deref(). This CL is too big and scary for me to review. You're welcome to find someone else to review it, but if you'd like me to review it, you're going to need to break it down into smaller pieces that are easier to understand.
On 2013/08/28 06:35:59, abarth wrote: > There's a bunch of stuff in this Cl that worries me. You've got a raw pointer > to the ScriptExecutionContext and manual calls to ref() and deref(). This CL is > too big and scary for me to review. You're welcome to find someone else to > review it, but if you'd like me to review it, you're going to need to break it > down into smaller pieces that are easier to understand. I see. I will divide this CL into smaller CLs. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
