[webkit-reviews] review denied: [Bug 48507] [GTK] Implement RunLoop, WorkQueue, Connection classes for WebKit2 : [Attachment 72680] RunLoop WorkQueue and Connection class implementation for GTK port

[bugzilla-daemon at webkit.org]

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 72680: RunLoop WorkQueue and Connection class implementation for GTK
port
https://bugs.webkit.org/attachment.cgi?id=72680&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.