Summary: | Abstract out WebSocketChannel message queue | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, cgarcia, commit-queue, webkit-bug-importer, youennf | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 201377 | ||||||||||
Attachments: |
|
Description
youenn fablet
2019-08-30 15:27:16 PDT
Created attachment 377757 [details]
Patch
Comment on attachment 377757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377757&action=review > Source/WebCore/ChangeLog:25 > + (WebCore::MessageQueueWithBlobs::MessageQueueWithBlobs): I think this needs a better name, like NetworkSendQueue or something. This currently looks related to WTF::MessageQueue, but it's not. > Source/WebCore/fileapi/MessageQueueWithBlobs.h:46 > + MessageQueueWithBlobs(Document&, Function<void(const String&)>&&, Function<void(const char*, size_t)>&&, Function<bool(int)>&&); I think these functions need names in the header. Why does notifyError return a bool? > Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:56 > + }, [&channel](const char* data, size_t byteLength) { > + channel.sendMessage(Messages::NetworkSocketChannel::SendData { IPC::DataReference { reinterpret_cast<const uint8_t*>(data), byteLength } }, byteLength); This captures a raw pointer and uses it later. I think we should capture a strong or weak reference instead. (In reply to Alex Christensen from comment #2) > Comment on attachment 377757 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377757&action=review > > > Source/WebCore/ChangeLog:25 > > + (WebCore::MessageQueueWithBlobs::MessageQueueWithBlobs): > > I think this needs a better name, like NetworkSendQueue or something. This > currently looks related to WTF::MessageQueue, but it's not. OK. > > Source/WebCore/fileapi/MessageQueueWithBlobs.h:46 > > + MessageQueueWithBlobs(Document&, Function<void(const String&)>&&, Function<void(const char*, size_t)>&&, Function<bool(int)>&&); > > I think these functions need names in the header. Why does notifyError > return a bool? Will add names, notifyError returns a boolean to determine whether processing messages should stop after an error or not. I will change it to processError. > > Source/WebKit/WebProcess/Network/WebSocketChannel.cpp:56 > > + }, [&channel](const char* data, size_t byteLength) { > > + channel.sendMessage(Messages::NetworkSocketChannel::SendData { IPC::DataReference { reinterpret_cast<const uint8_t*>(data), byteLength } }, byteLength); > > This captures a raw pointer and uses it later. I think we should capture a > strong or weak reference instead. channel is owning the MessageQueueWithBlobs so the lambdas will be destroyed at channel destruction time. The only tricky part is the case of the lambdas being called during the destruction of channel. I believe this is handled by the ABORT_ERROR check. Created attachment 377788 [details]
Patch
Created attachment 377796 [details]
Patch
Comment on attachment 377796 [details] Patch Clearing flags on attachment: 377796 Committed r249481: <https://trac.webkit.org/changeset/249481> All reviewed patches have been landed. Closing bug. |