Bug 48507 – [GTK] Implement RunLoop, WorkQueue, Connection classes for WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Amruth Raj]

Implement RunLoop, WorkQueue and Connection classes and their corresponding Makefile changes

[Amruth Raj]

Created attachment 72321 [details]
Implements RunLoop, WorkQueue, Connection classes

[Ravi Phaneendra Kasibhatla]

Adding myself to the CC list for this bug.

[Ravi Phaneendra Kasibhatla]

Created attachment 72680 [details]
RunLoop WorkQueue and Connection class implementation for GTK port

Patch contains the implementation of RunLoop, WorkQueue and Connection classes for GTK port.

[Martin Robinson]

Comment on attachment 72680 [details]
RunLoop WorkQueue and Connection class implementation for GTK port

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

Here's an initial review. These are mostly style issues. Someone with more experience with the WebKit2 ports might have more insightful comments about the functionality.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:34
> +#include "ArgumentEncoder.h"
> +#include "WorkItem.h"
> +
> +#include <errno.h>
> +#include <stdio.h>
> +#include <sys/fcntl.h>

These includes should be together in one clump with a newline before the "using" declaration.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:51
> +    // TODO: unregister from connection queue.
> +    m_connectionQueue.unregisterEventSourceHandler(m_socket);

Why isn't it possible to simply implement this now? It should probably be implemented or the comment should explain why.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:81
> +        Vector<uint8_t> buffer;
> +        buffer.resize(messageSize);
> +        numberOfBytesRead = read(m_socket, buffer.data(), messageSize);
> +
> +        size_t realBufferSize = messageSize - sizeof(MessageID);
> +        unsigned messageID = *reinterpret_cast<unsigned*>(buffer.data() + realBufferSize);
> +
> +        processIncomingMessage(MessageID::fromInt(messageID), adoptPtr(new ArgumentDecoder(buffer.data(), realBufferSize)));

This seems wrong. It assumes that read(...) is returning all of the message data, which read(...) doesn't guarantee. If, for some reason, it is always the case for Unix domain sockets then there should be a comment about it, I think. Additionally numberOfBytesRead is used to store the the result of read(...), but it isn't used. It's only used for the earlier size_t read(...).

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:112
> +    Vector<uint8_t> buffer;
> +    buffer.append(reinterpret_cast<uint8_t*>(&bufferSize), sizeof(size_t));
> +    buffer.append(arguments->buffer(), arguments->bufferSize());

Sending these separately will avoid a memcpy, right? Perhaps we should do that.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:116
> +    int bytesWritten = write(m_socket, buffer.data(), buffer.size());
> +    ASSERT_UNUSED(bytesWritten, bytesWritten);
> +    

Shouldn't we be checking the return value here? write returns -1 on error and perhaps we should handle that somehow. Qt seems to handle all situations where bytesWritten != buffer.size() as a fatal error always. We should probably handle it somehow.

> WebKit2/Platform/RunLoop.h:37
> +#include <glib.h>

Please do no use GTK+ / GLib includes in headers. If you have to use GLib types, use forward-declarations.

> WebKit2/Platform/gtk/RunLoopGtk.cpp:78
> +    g_source_set_callback(source, (GSourceFunc)&RunLoop::performWork, this, NULL);

Use 0 here instead of NULL (and everywhere, except when it generates a warning).

> WebKit2/Platform/gtk/RunLoopGtk.cpp:98
> +gboolean RunLoop::TimerBase::oneShotTimerFired(gpointer context)
> +{
> +    RunLoop::TimerBase* timerclass = static_cast<RunLoop::TimerBase*>(context);
> +    timerclass->fired();

Traditionally we just cast the callback when it's added and use something like gboolean RunLoop::TimerBase::oneShotTimerFired(RunLoop::TimerBase* timerClass). Note the camelCase. Why is this called timerclass, when it's an instance?

> WebKit2/Platform/gtk/RunLoopGtk.cpp:107
> +gboolean RunLoop::TimerBase::repeatingTimerFired(gpointer context)
> +{
> +    RunLoop::TimerBase* timerclass = static_cast<RunLoop::TimerBase*>(context);
> +    timerclass->fired();
> +    return TRUE;
> +}

Same comments here.

> WebKit2/Platform/gtk/RunLoopGtk.cpp:116
> +        g_source_set_callback(m_timerSource, (GSourceFunc)&RunLoop::TimerBase::repeatingTimerFired, this, NULL);

0 instead of NULL.

> WebKit2/Platform/gtk/RunLoopGtk.cpp:119
> +        g_source_set_callback(m_timerSource, (GSourceFunc)&RunLoop::TimerBase::oneShotTimerFired, this, NULL);

Ditto.

> WebKit2/Platform/gtk/RunLoopGtk.cpp:135
> +    return m_timerSource? true : false;

Is it possible to just do something like "return m_timerSource;" without generating a warning?

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:62
> +void* WorkQueue::workQueueThreadBody(void *context)
> +{
> +    static_cast<WorkQueue*>(context)->workQueueThreadBody();
> +    return 0;
> +}

I think this might be clearer if it was void* WorkQueue::workQueueThreadBody(WorkQueue *workQueue) and you just use a reinterpret_cast cast when you add it. See comment above.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:74
> +    : m_dispatchSource(dispatchSource)
> +    , m_workItem(workItem)
> +    , m_workQueue(workQueue)

These should be indented.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:82
> +    static gboolean performWorkOnce(gpointer source)
> +    {
> +        EventSource* eventSource = static_cast<EventSource*>(source);

Ditto with the reinterpret_cast approach.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:94
> +    static gboolean closeConnection(GIOChannel* channel, GIOCondition condition, gpointer source)

Ditto.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:96
> +        if ((condition & G_IO_HUP) || (condition & G_IO_ERR)) {

Please use an early return here instead.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:112
> +    static gboolean performWork(GIOChannel* channel, GIOCondition condition, gpointer source) 
> +    {
> +        if (condition & G_IO_IN) {
> +            EventSource* eventSource = static_cast<EventSource*>(source);

Same two comments as above.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:127
> +    static void deleteEventSource(gpointer source) 

Ditto.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:150
> +    if (condition == G_IO_IN) {
> +        g_source_set_callback(dispatchSource, (GSourceFunc)& WorkQueue::EventSource::performWork, eventSource, &WorkQueue::EventSource::deleteEventSource);
> +    } else if (condition == (G_IO_HUP | G_IO_ERR)) {
> +        g_source_set_callback(dispatchSource, (GSourceFunc)&WorkQueue::EventSource::closeConnection, eventSource, &WorkQueue::EventSource::deleteEventSource);
> +    }

A few issues here:
1. One line bodies should not have the surrounding curly braces per WebKit style. On the other hand these lines are pretty long so maybe you should split them across multiple lines which would require the curly braces. See the WebKit style guidelines for more information: http://webkit.org/coding/coding-style.html
2. Use a reinterpret_cast instead of a C-style cast. This comment applies globally to this patch. :)
3. The spacing is weird around for the ampersand operator in the first block "& WorkQueue::" --> "&WorkQueue".

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:182
> +        Vector<EventSource*> sources;
> +        sources = it->second;

Couldn't this be one line?

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:186
> +            EventSource* eventSource = sources[i];
> +            GSource* gSource = eventSource->dispatchSource();
> +            g_source_destroy(gSource);

Couldn't this be one line too?

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:198
> +    g_source_set_callback(dispatchSource, (GSourceFunc)&WorkQueue::EventSource::performWorkOnce, eventSource, &WorkQueue::EventSource::deleteEventSource);

reinterpret_cast here.

> WebKit2/Platform/PlatformProcessIdentifier.h:3
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.

Typically there is a threshold of about 10 lines before adding copyright lines to a file.

> WebKit2/Platform/WorkQueue.h:35
> +#include <glib.h>

Please do not include GTK+ / GLib includes in header files. Instead use forward declarations. GTK+/GLib headers increase build times.

> WebKit2/Platform/WorkQueue.h:164
> +    class EventSource;

This forward declaration should be at the top of the file, I think.

[Amruth Raj]

(In reply to comment #4)
> (From update of attachment 72680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72680&action=review
> 
> Here's an initial review. These are mostly style issues. Someone with more experience with the WebKit2 ports might have more insightful comments about the functionality.
> 
Adding more WebKit2 contributors.
Could you please give us your comments on this patch. Robinson has given his initial comments and we are working on the same.

[Balazs Kelemen]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 72680 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72680&action=review
> > 
> > Here's an initial review. These are mostly style issues. Someone with more experience with the WebKit2 ports might have more insightful comments about the functionality.
> > 
> Adding more WebKit2 contributors.
> Could you please give us your comments on this patch. Robinson has given his initial comments and we are working on the same.

Generally looks pretty straightforward to me. I do not see any blocker
from landing it after you have fixed the problems mentioned by Robinson.

[Ravi Phaneendra Kasibhatla]

Created attachment 74227 [details]
RunLoop WorkQueue Connection class implementation

RunLoop WorkQueue Connection class implementation after addressing the comments by mrobinson.

[Ravi Phaneendra Kasibhatla]

Created attachment 74230 [details]
RunLoop WorkQueue Connection class implementation

Added a missing change with the previous patch

[Ravi Phaneendra Kasibhatla]

(In reply to comment #4)
> (From update of attachment 72680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72680&action=review
> 
> Here's an initial review. These are mostly style issues. Someone with more experience with the WebKit2 ports might have more insightful comments about the functionality.
> 
> > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:34
> > +#include "ArgumentEncoder.h"
> > +#include "WorkItem.h"
> > +
> > +#include <errno.h>
> > +#include <stdio.h>
> > +#include <sys/fcntl.h>
> 
> These includes should be together in one clump with a newline before the "using" declaration.
Done.
> 
> > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:51
> > +    // TODO: unregister from connection queue.
> > +    m_connectionQueue.unregisterEventSourceHandler(m_socket);
> 
> Why isn't it possible to simply implement this now? It should probably be implemented or the comment should explain why.
> 
This is an incorrect TODO. This is implemented. We have removed this now.
> > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:81
> > +        Vector<uint8_t> buffer;
> > +        buffer.resize(messageSize);
> > +        numberOfBytesRead = read(m_socket, buffer.data(), messageSize);
> > +
> > +        size_t realBufferSize = messageSize - sizeof(MessageID);
> > +        unsigned messageID = *reinterpret_cast<unsigned*>(buffer.data() + realBufferSize);
> > +
> > +        processIncomingMessage(MessageID::fromInt(messageID), adoptPtr(new ArgumentDecoder(buffer.data(), realBufferSize)));
> 
> This seems wrong. It assumes that read(...) is returning all of the message data, which read(...) doesn't guarantee. If, for some reason, it is always the case for Unix domain sockets then there should be a comment about it, I think. Additionally numberOfBytesRead is used to store the the result of read(...), but it isn't used. It's only used for the earlier size_t read(...).
> 
Thanks for pointing. Unix domain sockets do not guarantee it. We implemented functions writeBytesToSocket, readBytesFromSocket to handle these cases. We also handled the error cases.
> > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:112
> > +    Vector<uint8_t> buffer;
> > +    buffer.append(reinterpret_cast<uint8_t*>(&bufferSize), sizeof(size_t));
> > +    buffer.append(arguments->buffer(), arguments->bufferSize());
> 
> Sending these separately will avoid a memcpy, right? Perhaps we should do that.
Done.
> 
> > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:116
> > +    int bytesWritten = write(m_socket, buffer.data(), buffer.size());
> > +    ASSERT_UNUSED(bytesWritten, bytesWritten);
> > +    
> 
> Shouldn't we be checking the return value here? write returns -1 on error and perhaps we should handle that somehow. Qt seems to handle all situations where bytesWritten != buffer.size() as a fatal error always. We should probably handle it somehow.
Done.
> 
> > WebKit2/Platform/RunLoop.h:37
> > +#include <glib.h>
> 
> Please do no use GTK+ / GLib includes in headers. If you have to use GLib types, use forward-declarations.
Done.
> 
> > WebKit2/Platform/gtk/RunLoopGtk.cpp:78
> > +    g_source_set_callback(source, (GSourceFunc)&RunLoop::performWork, this, NULL);
> 
> Use 0 here instead of NULL (and everywhere, except when it generates a warning).
Done.
> 
> > WebKit2/Platform/gtk/RunLoopGtk.cpp:98
> > +gboolean RunLoop::TimerBase::oneShotTimerFired(gpointer context)
> > +{
> > +    RunLoop::TimerBase* timerclass = static_cast<RunLoop::TimerBase*>(context);
> > +    timerclass->fired();
> 
> Traditionally we just cast the callback when it's added and use something like gboolean RunLoop::TimerBase::oneShotTimerFired(RunLoop::TimerBase* timerClass). Note the camelCase. Why is this called timerclass, when it's an instance?
Done.
> 
> > WebKit2/Platform/gtk/RunLoopGtk.cpp:107
> > +gboolean RunLoop::TimerBase::repeatingTimerFired(gpointer context)
> > +{
> > +    RunLoop::TimerBase* timerclass = static_cast<RunLoop::TimerBase*>(context);
> > +    timerclass->fired();
> > +    return TRUE;
> > +}
> 
> Same comments here.
Done.
> 
> > WebKit2/Platform/gtk/RunLoopGtk.cpp:116
> > +        g_source_set_callback(m_timerSource, (GSourceFunc)&RunLoop::TimerBase::repeatingTimerFired, this, NULL);
> 
> 0 instead of NULL.
Done.
> 
> > WebKit2/Platform/gtk/RunLoopGtk.cpp:119
> > +        g_source_set_callback(m_timerSource, (GSourceFunc)&RunLoop::TimerBase::oneShotTimerFired, this, NULL);
> 
> Ditto.
Done.
> 
> > WebKit2/Platform/gtk/RunLoopGtk.cpp:135
> > +    return m_timerSource? true : false;
> 
> Is it possible to just do something like "return m_timerSource;" without generating a warning?
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:62
> > +void* WorkQueue::workQueueThreadBody(void *context)
> > +{
> > +    static_cast<WorkQueue*>(context)->workQueueThreadBody();
> > +    return 0;
> > +}
> 
> I think this might be clearer if it was void* WorkQueue::workQueueThreadBody(WorkQueue *workQueue) and you just use a reinterpret_cast cast when you add it. See comment above.
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:74
> > +    : m_dispatchSource(dispatchSource)
> > +    , m_workItem(workItem)
> > +    , m_workQueue(workQueue)
> 
> These should be indented.
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:82
> > +    static gboolean performWorkOnce(gpointer source)
> > +    {
> > +        EventSource* eventSource = static_cast<EventSource*>(source);
> 
> Ditto with the reinterpret_cast approach.
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:94
> > +    static gboolean closeConnection(GIOChannel* channel, GIOCondition condition, gpointer source)
> 
> Ditto.
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:96
> > +        if ((condition & G_IO_HUP) || (condition & G_IO_ERR)) {
> 
> Please use an early return here instead.
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:112
> > +    static gboolean performWork(GIOChannel* channel, GIOCondition condition, gpointer source) 
> > +    {
> > +        if (condition & G_IO_IN) {
> > +            EventSource* eventSource = static_cast<EventSource*>(source);
> 
> Same two comments as above.
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:127
> > +    static void deleteEventSource(gpointer source) 
> 
> Ditto.
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:150
> > +    if (condition == G_IO_IN) {
> > +        g_source_set_callback(dispatchSource, (GSourceFunc)& WorkQueue::EventSource::performWork, eventSource, &WorkQueue::EventSource::deleteEventSource);
> > +    } else if (condition == (G_IO_HUP | G_IO_ERR)) {
> > +        g_source_set_callback(dispatchSource, (GSourceFunc)&WorkQueue::EventSource::closeConnection, eventSource, &WorkQueue::EventSource::deleteEventSource);
> > +    }
> 
> A few issues here:
> 1. One line bodies should not have the surrounding curly braces per WebKit style. On the other hand these lines are pretty long so maybe you should split them across multiple lines which would require the curly braces. See the WebKit style guidelines for more information: http://webkit.org/coding/coding-style.html
> 2. Use a reinterpret_cast instead of a C-style cast. This comment applies globally to this patch. :)
> 3. The spacing is weird around for the ampersand operator in the first block "& WorkQueue::" --> "&WorkQueue".
> 
Done. Addressed all 3 points.
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:182
> > +        Vector<EventSource*> sources;
> > +        sources = it->second;
> 
> Couldn't this be one line?
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:186
> > +            EventSource* eventSource = sources[i];
> > +            GSource* gSource = eventSource->dispatchSource();
> > +            g_source_destroy(gSource);
> 
> Couldn't this be one line too?
Done.
> 
> > WebKit2/Platform/gtk/WorkQueueGtk.cpp:198
> > +    g_source_set_callback(dispatchSource, (GSourceFunc)&WorkQueue::EventSource::performWorkOnce, eventSource, &WorkQueue::EventSource::deleteEventSource);
> 
> reinterpret_cast here.
Done.
> 
> > WebKit2/Platform/PlatformProcessIdentifier.h:3
> > + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.
> 
> Typically there is a threshold of about 10 lines before adding copyright lines to a file.
Done.
> 
> > WebKit2/Platform/WorkQueue.h:35
> > +#include <glib.h>
> 
> Please do not include GTK+ / GLib includes in header files. Instead use forward declarations. GTK+/GLib headers increase build times.
> 
> > WebKit2/Platform/WorkQueue.h:164
> > +    class EventSource;
> 
> This forward declaration should be at the top of the file, I think.
We want EventSource forward declaration only within the WorkQueue class as we are declaring it inner class of WorkQueue. So, we followed the existing style in the file i.e. as in Mac, Win and Qt ports.

[Ravi Phaneendra Kasibhatla]

Created attachment 74930 [details]
RunLoop WorkQueue Connection class implementation without using ioctl

RunLoop WorkQueue Connection class implementation without using ioctl as per last review comments.

[Martin Robinson]

Comment on attachment 74930 [details]
RunLoop WorkQueue Connection class implementation without using ioctl

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).

[Martin Robinson]

(In reply to comment #11)

> 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.

Sorry. This should read:  "Why not just pass a uint8_t*? You shouldn't need to cast a pointer to void* anyway."

[Amruth Raj]

Created attachment 75006 [details]
RunLoop WorkQueue Connection class implementation

[Amruth Raj]

(In reply to comment #11)
> (From update of attachment 74930 [details])
> 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?
Done
> 
> > 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?
Done.
> 
> > WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:77
> > +    uint8_t* buffer = reinterpret_cast<uint8_t*>(ptr);
> 
> Same.
Done.
> 
> > 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.
Done.
> 
> > WebKit2/Platform/gtk/RunLoopGtk.cpp:45
> > +        m_runLoopMain = 0;
> 
> I don't think you need to set this to zero in the destructor.
> 
Done.
> > WebKit2/Platform/gtk/RunLoopGtk.cpp:50
> > +        m_runLoopContext = 0;
> 
> Same.
Done.
> 
> > 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?
Changed the code to return false. Thanks for pointing.
> 
> > 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).
Removed closeConnection callback. The return value can be determined by the condition parameter of the callback.

[Martin Robinson]

Comment on attachment 75006 [details]
RunLoop WorkQueue Connection class implementation

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

Looks good to me, though there are a few style issues. I'll fix them and land it myself.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:41
> +static int readBytesFromSocket(int fd, uint8_t* ptr, size_t length)

This should be "fileDescriptor" instead of "fd".

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:69
> +static bool writeBytesToSocket(int fd, uint8_t* ptr, size_t length)

Same.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:79
> +    // Keep writing to the socket till the complete message has been written

This is missing the period.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:91
> +    // write operation failed if complete message is not written

This should have a capital letter and a period.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:135
> +    // handle any previously unprocessed message

Ditto.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:144
> +    // prepare to read the next message

Ditto.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:131
> +void WorkQueue::registerEventSourceHandler(int fd, int condition, PassOwnPtr<WorkItem> item)

Sorry that I missed this. It should be "fileDescriptor" not "fd".

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:144
> +            reinterpret_cast<GSourceFunc>(&WorkQueue::EventSource::performWork), 
> +            eventSource, 
> +            reinterpret_cast<GDestroyNotify>(&WorkQueue::EventSource::deleteEventSource));
> +

The indentation looks off here.

> WebKit2/Platform/gtk/WorkQueueGtk.cpp:165
> +void WorkQueue::unregisterEventSourceHandler(int fd)

This should be fileDesciptor as well.

[Amruth Raj]

Created attachment 75106 [details]
Addressed style issues

Addressed the style issues mentioned in the previous comment

[Martin Robinson]

Comment on attachment 75106 [details]
Addressed style issues

Thanks. That makes things easier.

[WebKit Commit Bot]

Comment on attachment 75106 [details]
Addressed style issues

Rejecting patch 75106 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 75106]" exit_code: 2
Last 500 characters of output:
ILED at 83.
Hunk #4 succeeded at 141 (offset 2 lines).
1 out of 4 hunks FAILED -- saving rejects to file WebKit2/Platform/RunLoop.h.rej
patching file WebKit2/Platform/WorkQueue.h
Hunk #3 succeeded at 83 (offset 5 lines).
Hunk #4 succeeded at 158 (offset 5 lines).
patching file WebKit2/Platform/gtk/RunLoopGtk.cpp
patching file WebKit2/Platform/gtk/WorkQueueGtk.cpp

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Martin Robinson', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/6484003

[Amruth Raj]

Created attachment 75256 [details]
Patch on top of latest revision

[WebKit Commit Bot]

Comment on attachment 75256 [details]
Patch on top of latest revision

Clearing flags on attachment: 75256

Committed r73142: <http://trac.webkit.org/changeset/73142>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.