Bug 158539 – Modernize WebSocket code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Modernize WebSocket code

[Alex Christensen]

Created attachment 280835 [details]
Patch

[Alex Christensen]

Comment on attachment 280835 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280835&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

I'll put "no change in behavior" here

[WebKit Commit Bot]

Attachment 280835 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 280840 [details]
Patch

[Brady Eidson]

Comment on attachment 280840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280840&action=review

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:175
> -    RefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper = m_workerClientWrapper;
> +    RefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper = m_workerClientWrapper.copyRef();
>      m_loaderProxy.postTaskForModeToWorkerGlobalScope([workerClientWrapper = WTFMove(workerClientWrapper), sendRequestResult] (ScriptExecutionContext&) {

This can be written as 1 line.

m_loaderProxy.postTaskForModeToWorkerGlobalScope([workerClientWrapper = Ref<ThreadableWebSocketChannelClientWrapper>(*m_workerClientWrapper), sendRequestResult] (ScriptExecutionContext&) mutable {

All the other ones like it, too!

[Brady Eidson]

(In reply to comment #5)
> Comment on attachment 280840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280840&action=review
> 
> > Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:175
> > -    RefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper = m_workerClientWrapper;
> > +    RefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper = m_workerClientWrapper.copyRef();
> >      m_loaderProxy.postTaskForModeToWorkerGlobalScope([workerClientWrapper = WTFMove(workerClientWrapper), sendRequestResult] (ScriptExecutionContext&) {
> 
> This can be written as 1 line.
> 
> m_loaderProxy.postTaskForModeToWorkerGlobalScope([workerClientWrapper =
> Ref<ThreadableWebSocketChannelClientWrapper>(*m_workerClientWrapper),
> sendRequestResult] (ScriptExecutionContext&) mutable {
> 
> All the other ones like it, too!

Also, there should be no space between the capture list and the open paren:

m_loaderProxy.postTaskForModeToWorkerGlobalScope([workerClientWrapper =
> Ref<ThreadableWebSocketChannelClientWrapper>(*m_workerClientWrapper),
> sendRequestResult](ScriptExecutionContext&) mutable {

[Alex Christensen]

Created attachment 280845 [details]
Patch

[Brady Eidson]

Comment on attachment 280845 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280845&action=review

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:355
> +    auto peer = std::make_unique<Peer>(clientWrapper.copyRef(), loaderProxy, context, taskMode);
> +    bool sent = loaderProxy.postTaskForModeToWorkerGlobalScope({
>          ScriptExecutionContext::Task::CleanupTask,
> -        [clientWrapper, loaderProxy, peer = WTFMove(peer)] (ScriptExecutionContext& context) mutable {
> +        [clientWrapper = clientWrapper.copyRef(), &loaderProxy, peer = WTFMove(peer)] (ScriptExecutionContext& context) mutable {

You could move the peer creation into the capture list.

[Alex Christensen]

http://trac.webkit.org/changeset/201837