Bug 48507

Summary: [GTK] Implement RunLoop, WorkQueue, Connection classes for WebKit2
Product: WebKit Reporter: Amruth Raj <amruthraj>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, aroben, commit-queue, gns, joone, kbalazs, kenneth, koivisto, mrobinson, ravi.kasibhatla, sam, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 52805    
Attachments:
Description Flags
Implements RunLoop, WorkQueue, Connection classes
none
RunLoop WorkQueue and Connection class implementation for GTK port
mrobinson: review-
RunLoop WorkQueue Connection class implementation
none
RunLoop WorkQueue Connection class implementation
none
RunLoop WorkQueue Connection class implementation without using ioctl
mrobinson: review-
RunLoop WorkQueue Connection class implementation
mrobinson: review+
Addressed style issues
mrobinson: review+, commit-queue: commit-queue-
Patch on top of latest revision none

Description Amruth Raj 2010-10-28 04:38:30 PDT
Implement RunLoop, WorkQueue and Connection classes and their corresponding Makefile changes
Comment 1 Amruth Raj 2010-10-29 06:15:37 PDT
Created attachment 72321 [details]
Implements RunLoop, WorkQueue, Connection classes
Comment 2 Ravi Phaneendra Kasibhatla 2010-10-29 07:14:31 PDT
Adding myself to the CC list for this bug.
Comment 3 Ravi Phaneendra Kasibhatla 2010-11-02 09:39:58 PDT
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.
Comment 4 Martin Robinson 2010-11-15 11:51:31 PST
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.
Comment 5 Amruth Raj 2010-11-16 07:12:59 PST
(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.
Comment 6 Balazs Kelemen 2010-11-16 15:17:04 PST
(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.
Comment 7 Ravi Phaneendra Kasibhatla 2010-11-18 05:17:17 PST
Created attachment 74227 [details]
RunLoop WorkQueue Connection class implementation

RunLoop WorkQueue Connection class implementation after addressing the comments by mrobinson.
Comment 8 Ravi Phaneendra Kasibhatla 2010-11-18 06:08:32 PST
Created attachment 74230 [details]
RunLoop WorkQueue Connection class implementation

Added a missing change with the previous patch
Comment 9 Ravi Phaneendra Kasibhatla 2010-11-18 06:17:15 PST
(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.
Comment 10 Ravi Phaneendra Kasibhatla 2010-11-26 06:27:24 PST
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.
Comment 11 Martin Robinson 2010-11-28 04:58:12 PST
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).
Comment 12 Martin Robinson 2010-11-28 04:59:45 PST
(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."
Comment 13 Amruth Raj 2010-11-29 03:08:54 PST
Created attachment 75006 [details]
RunLoop WorkQueue Connection class implementation
Comment 14 Amruth Raj 2010-11-29 03:20:03 PST
(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.
Comment 15 Martin Robinson 2010-11-29 05:45:28 PST
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.
Comment 16 Amruth Raj 2010-11-29 22:26:50 PST
Created attachment 75106 [details]
Addressed style issues

Addressed the style issues mentioned in the previous comment
Comment 17 Martin Robinson 2010-11-30 00:17:16 PST
Comment on attachment 75106 [details]
Addressed style issues

Thanks. That makes things easier.
Comment 18 WebKit Commit Bot 2010-11-30 19:00:00 PST
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
Comment 19 Amruth Raj 2010-11-30 23:05:42 PST
Created attachment 75256 [details]
Patch on top of latest revision
Comment 20 WebKit Commit Bot 2010-12-02 09:34:22 PST
Comment on attachment 75256 [details]
Patch on top of latest revision

Clearing flags on attachment: 75256

Committed r73142: <http://trac.webkit.org/changeset/73142>
Comment 21 WebKit Commit Bot 2010-12-02 09:34:29 PST
All reviewed patches have been landed.  Closing bug.