[Webkit-unassigned] [Bug 48507] [GTK] Implement RunLoop, WorkQueue, Connection classes for WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48507





--- Comment #14 from Amruth Raj <amruthraj at motorola.com>  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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.