[webkit-reviews] review denied: [Bug 48507] [GTK] Implement RunLoop, WorkQueue, Connection classes for WebKit2 : [Attachment 74930] RunLoop WorkQueue Connection class implementation without using ioctl
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Nov 28 04:58:12 PST 2010
Martin Robinson <mrobinson at webkit.org> has denied Ravi Phaneendra Kasibhatla
<ravi.kasibhatla at motorola.com>'s request for review:
Bug 48507: [GTK] Implement RunLoop, WorkQueue, Connection classes for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48507
Attachment 74930: RunLoop WorkQueue Connection class implementation without
using ioctl
https://bugs.webkit.org/attachment.cgi?id=74930&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74930&action=review
Great progress. I'm very happy with the elimination of the ioctl call. I still
have a couple questions though.
> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:39
> +static const size_t messageInitialSize = 4096;
initialMessageBufferSize?
> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:49
> + uint8_t* buffer = reinterpret_cast<uint8_t*>(ptr);
Why not just pass in a uint8_t pointer here?
> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:77
> + uint8_t* buffer = reinterpret_cast<uint8_t*>(ptr);
Same.
> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:136
> + if ((m_pendingBytes -= readBytesFromSocket(m_socket,
(void*)(m_readBuffer.data() + (m_currentMessageSize - m_pendingBytes)),
m_pendingBytes)) > 0)
Why not just pass a uint8_t*? Can't readBytesFromSocket just take a uint8_t.
You shouldn't need to cast a pointer to void* anyway.
> WebKit2/Platform/gtk/RunLoopGtk.cpp:45
> + m_runLoopMain = 0;
I don't think you need to set this to zero in the destructor.
> WebKit2/Platform/gtk/RunLoopGtk.cpp:50
> + m_runLoopContext = 0;
Same.
> WebKit2/Platform/gtk/WorkQueueGtk.cpp:84
> + return TRUE;
If the queue is invalid do we want to keep getting this callback? Can a queue
ever change from the invalid state?
> WebKit2/Platform/gtk/WorkQueueGtk.cpp:161
> + if (condition == G_IO_IN) {
> + g_source_set_callback(dispatchSource,
> +
reinterpret_cast<GSourceFunc>(&WorkQueue::EventSource::performWork),
> + eventSource,
> +
reinterpret_cast<GDestroyNotify>(&WorkQueue::EventSource::deleteEventSource));
> + } else if (condition == (G_IO_HUP | G_IO_ERR)) {
> + g_source_set_callback(dispatchSource,
> +
reinterpret_cast<GSourceFunc>(&WorkQueue::EventSource::closeConnection),
> + eventSource,
> +
reinterpret_cast<GDestroyNotify>(&WorkQueue::EventSource::deleteEventSource));
> + }
This design is a little bit unbalanced, I think. Both the caller and the callee
need to know about the semantics of the g_source_set_callback call. I'm not
sure if it's possible to make the method more general or not. Maybe you could
have a single source callback and pass some data about what the return value
should be (true or false). That would remove the need for a specialized source
callback (EventSource::closeConnection).
More information about the webkit-reviews
mailing list