Now, WebSocketChannel is exported as WebKit::WebSocket in Chromium port. For now, bufferedAmount property needs some latency to get from plugins. This callback allow client side cache its value and reduce IPC turnaround time latency.
Created attachment 116915 [details] Patch
Comment on attachment 116915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116915&action=review > Source/WebCore/platform/network/SocketStreamHandleBase.cpp:42 > + : m_handle(handle) You don't have to save m_handle. As SocketStreamHandle is derived from SocketStreamHandleBase, it is safe to cast |this| to SocketStreamHandle* (i.e. static_cast<SocketStreamHandle*>(this) is always equal to |handle|). Compilers take care of offset adjustment. > Source/WebCore/platform/network/SocketStreamHandleClient.h:49 > + virtual void didUpdateBufferedAmount(SocketStreamHandle*, int bufferedAmount) { } Is |int| large enough? > Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:2 > + * Copyright (C) 2011 Apple Inc. All rights reserved. Don't update copyright year of others. > Source/WebCore/platform/network/curl/SocketStreamHandleCurl.cpp:2 > + * Copyright (C) 2011 Brent Fulgham. All rights reserved. Ditto. > Source/WebCore/platform/network/qt/SocketStreamHandleQt.cpp:2 > + * Copyright (C) 2011 Nokia Inc. All rights reserved. Ditto. > Source/WebCore/platform/network/win/SocketStreamHandleWin.cpp:2 > + * Copyright (C) 2011 Brent Fulgham. All rights reserved. Ditto.
Comment on attachment 116915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116915&action=review > Source/WebCore/websockets/WebSocket.h:109 > + virtual void didUpdateBufferedAmount(int bufferedAmount); You had better append OVERRIDE. > Source/WebCore/websockets/WebSocketChannel.h:84 > + virtual void didUpdateBufferedAmount(SocketStreamHandle*, int bufferedAmount); ditto.
Created attachment 116941 [details] Patch
Comment on attachment 116915 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116915&action=review >> Source/WebCore/platform/network/SocketStreamHandleBase.cpp:42 >> + : m_handle(handle) > > You don't have to save m_handle. As SocketStreamHandle is derived from SocketStreamHandleBase, it is safe to cast |this| to SocketStreamHandle* (i.e. static_cast<SocketStreamHandle*>(this) is always equal to |handle|). Compilers take care of offset adjustment. Thanks Yuta! Now, my change is quite simpler. >> Source/WebCore/platform/network/SocketStreamHandleClient.h:49 >> + virtual void didUpdateBufferedAmount(SocketStreamHandle*, int bufferedAmount) { } > > Is |int| large enough? Originally, SocketStreamHandleBase::bufferedAmount() returns an int value. But, size_t seems better than int because returned value comes from Vector::size() and it returns size_t. On the other hand, I guess I must use unsigned long in WebSocketChannelClient because WebSocketChannel::bufferedAmount returns unsigned long value. >> Source/WebCore/platform/network/cf/SocketStreamHandleCFNet.cpp:2 >> + * Copyright (C) 2011 Apple Inc. All rights reserved. > > Don't update copyright year of others. Thanks. But, these update will be removed in next change because I don't have to pass this pointer from them now. >> Source/WebCore/websockets/WebSocket.h:109 >> + virtual void didUpdateBufferedAmount(int bufferedAmount); > > You had better append OVERRIDE. Thanks. I added OVERRIDE to all inherit methods in WebSocket and WebSocketChannel.
Comment on attachment 116941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116941&action=review > Source/WebCore/websockets/WebSocket.h:93 > - virtual const AtomicString& interfaceName() const; > - virtual ScriptExecutionContext* scriptExecutionContext() const; > + virtual const AtomicString& interfaceName() const OVERRIDE; > + virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE; We had better NOT add OVERRIDE to functions other than didUpdateBufferAmount() in this patch. > Source/WebCore/websockets/WebSocketChannelClient.h:49 > + virtual void didUpdateBufferedAmount(unsigned long bufferedAmount) { } Why is the argument type is unsigned long instead of size_t?
Comment on attachment 116941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116941&action=review >> Source/WebCore/websockets/WebSocket.h:93 >> + virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE; > > We had better NOT add OVERRIDE to functions other than didUpdateBufferAmount() in this patch. You mean adding OVERRIDE to other methods must be splited into another change? If so, I'll make another change to add OVERRIDEs. >> Source/WebCore/websockets/WebSocketChannelClient.h:49 >> + virtual void didUpdateBufferedAmount(unsigned long bufferedAmount) { } > > Why is the argument type is unsigned long instead of size_t? WebSocketChannel::bufferedAmount() return unsigned long value. That mean all interfaces exposed to WebCore::WebSocket follows the WebSocket API specification defining types. So I decide to use unsigned long for WebSocketChannelClient to keep consistency with them.
Comment on attachment 116941 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116941&action=review >>> Source/WebCore/websockets/WebSocket.h:93 >>> + virtual ScriptExecutionContext* scriptExecutionContext() const OVERRIDE; >> >> We had better NOT add OVERRIDE to functions other than didUpdateBufferAmount() in this patch. > > You mean adding OVERRIDE to other methods must be splited into another change? > If so, I'll make another change to add OVERRIDEs. Yes. >>> Source/WebCore/websockets/WebSocketChannelClient.h:49 >>> + virtual void didUpdateBufferedAmount(unsigned long bufferedAmount) { } >> >> Why is the argument type is unsigned long instead of size_t? > > WebSocketChannel::bufferedAmount() return unsigned long value. > That mean all interfaces exposed to WebCore::WebSocket follows the WebSocket API specification defining types. > So I decide to use unsigned long for WebSocketChannelClient to keep consistency with them. Ok, I understand.
Created attachment 116946 [details] Patch
Thank you Kent, I uploaded revised one which remove all OVERRIDEs.
Adding OVERRIDEs in another change, I notice that we must care for the worker version of WebSocket. Please wait for the next patch which support the worker implementation.
Created attachment 116956 [details] Patch
Comment on attachment 116956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116956&action=review A few drive by comments. > Source/WebCore/platform/network/SocketStreamHandleBase.cpp:2 > + * Copyright (C) 2011 Google Inc. All rights reserved. In WebKit typically just add the year (2009, 2011) as opposed to deleting the old year like what is done in Chromium. > Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148 > + m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount)); You shouldn't use "AllowCrossThreadAccess" here and in fact should get rid of it throughout this file. Here's why: 1. It was added to highlight the places where we were not doing anything to help with lifetime of the object being passed. 2. At the time it was added, ThreadSafeRefCounted* objects passed through createCallbackTask did not have the ref count increased, so AllowCrossThreadAccess needed to be added here. 3. ThreadableWebSocketChannelClientWrapper is designed to be ref counted across threads -- largely to make sure that its lifetime is preserved by this call. So in short, putting AllowCrossThreadAccess means that ThreadableWebSocketChannelClientWrapper may get deleted before the callback is done leading to unpredictable crashes. Feel free to create a bug that blocks: https://bugs.webkit.org/show_bug.cgi?id=67942 to get rid of AllowCrossThreadAccess in this file. (I've been wanting to do this but I also wanted to look at the files and make sure it was the right thing to do. In this case, the patch motivated me to do so.)
Comment on attachment 116956 [details] Patch cq- due to my comments (specifically the ref count related one).
Comment on attachment 116956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116956&action=review >> Source/WebCore/platform/network/SocketStreamHandleBase.cpp:2 >> + * Copyright (C) 2011 Google Inc. All rights reserved. > > In WebKit typically just add the year (2009, 2011) as opposed to deleting the old year like what is done in Chromium. Thanks, I fix it in the next change. >> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148 >> + m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount)); > > You shouldn't use "AllowCrossThreadAccess" here and in fact should get rid of it throughout this file. > > Here's why: > 1. It was added to highlight the places where we were not doing anything to help with lifetime of the object being passed. > 2. At the time it was added, ThreadSafeRefCounted* objects passed through createCallbackTask did not have the ref count increased, so AllowCrossThreadAccess needed to be added here. > 3. ThreadableWebSocketChannelClientWrapper is designed to be ref counted across threads -- largely to make sure that its lifetime is preserved by this call. > > So in short, putting AllowCrossThreadAccess means that ThreadableWebSocketChannelClientWrapper may get deleted before the callback is done leading to unpredictable crashes. > > Feel free to create a bug that blocks: https://bugs.webkit.org/show_bug.cgi?id=67942 to get rid of AllowCrossThreadAccess in this file. (I've been wanting to do this but I also wanted to look at the files and make sure it was the right thing to do. In this case, the patch motivated me to do so.) OK, I see. I created a bug https://bugs.webkit.org/show_bug.cgi?id=73336 and set to block 67942.
Created attachment 116999 [details] Patch
(In reply to comment #15) > (From update of attachment 116956 [details]) > OK, I see. > I created a bug https://bugs.webkit.org/show_bug.cgi?id=73336 and set to block 67942. PS Feel free to do it too :). (It should be trivial and hopefully fix some crashes for you.)
Comment on attachment 116999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116999&action=review > Source/WebCore/websockets/WebSocket.cpp:510 > + LOG(Network, "WebSocket %p didUpdateBufferedAmount %lld", this, bufferedAmount); Why %lld for unsigned long value?
Comment on attachment 116999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116999&action=review > Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148 > + m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount)); Please remove AllowCrossThreadAccess :).
Created attachment 117104 [details] Patch
Comment on attachment 116999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116999&action=review >> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148 >> + m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount)); > > Please remove AllowCrossThreadAccess :). The change you reviewed in 73336 follows this change and will overwrite this. I decide to split removing AllowCrossThreadAccess change to 73336. >> Source/WebCore/websockets/WebSocket.cpp:510 >> + LOG(Network, "WebSocket %p didUpdateBufferedAmount %lld", this, bufferedAmount); > > Why %lld for unsigned long value? Oops, sorry for careless mistake.
(In reply to comment #21) > (From update of attachment 116999 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116999&action=review > > >> Source/WebCore/websockets/ThreadableWebSocketChannelClientWrapper.cpp:148 > >> + m_pendingTasks.append(createCallbackTask(&ThreadableWebSocketChannelClientWrapper::didUpdateBufferedAmountCallback, AllowCrossThreadAccess(this), bufferedAmount)); > > > > Please remove AllowCrossThreadAccess :). > > The change you reviewed in 73336 follows this change and will overwrite this. > I decide to split removing AllowCrossThreadAccess change to 73336. Oh, thanks. I thought it was just for existing items.... ok, I'm done here. I was just looking at some threaded related items.
Comment on attachment 117104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117104&action=review > Source/WebCore/websockets/WebSocket.cpp:510 > + LOG(Network, "WebSocket %p didUpdateBufferedAmount %d", this, bufferedAmount); Why %d? I think it should be %lu.
Comment on attachment 117104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117104&action=review > Source/WebCore/platform/network/SocketStreamHandleClient.h:3 > + * Copyright (C) 2011 Google Inc. All rights reserved. Fixed. >> Source/WebCore/websockets/WebSocket.cpp:510 >> + LOG(Network, "WebSocket %p didUpdateBufferedAmount %d", this, bufferedAmount); > > Why %d? I think it should be %lu. Sorry for vague understanding on conversion specifiers. I fixed it.
Created attachment 117121 [details] Patch
Comment on attachment 117121 [details] Patch Looks ok
Comment on attachment 117121 [details] Patch Clearing flags on attachment: 117121 Committed r101507: <http://trac.webkit.org/changeset/101507>
All reviewed patches have been landed. Closing bug.