[webkit-reviews] review granted: [Bug 51984] [WK2][Qt] Multiple problems with MemoryMappedPool : [Attachment 79608] And 3rd patch, remove unneeded qthread include from ConnectionQt.cpp and invalid assert in Attachment::dispose

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Kimmo Kinnunen
<kimmo.t.kinnunen at nokia.com>'s request for review:
Bug 51984: [WK2][Qt] Multiple problems with MemoryMappedPool
https://bugs.webkit.org/show_bug.cgi?id=51984

Attachment 79608: And 3rd patch, remove unneeded qthread include from
ConnectionQt.cpp and invalid assert in Attachment::dispose
https://bugs.webkit.org/attachment.cgi?id=79608&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79608&action=review

Pretty nice patch, r=me

> Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:53
> +    for (Deque<Attachment>::iterator i = m_attachments.begin(); i != end;
++i)

We normally call non-int iterators for 'it'

> Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.cpp:54
> +	       i->dispose();

Something went wrong with indent here

> Source/WebKit2/Platform/CoreIPC/Attachment.cpp:33
> +#if PLATFORM(QT)
> +#include <unistd.h>
> +#include <errno.h>
> +#endif

I think this could be made into a Linux (or POSIX) implementation in the
future, so we might want to change the ifdefs then

> Source/WebKit2/Platform/CoreIPC/Attachment.cpp:92
> +	   while (close(m_fileDescriptor) == -1 && (errno == EINTR)) {}

space needed between { and }, coding style rule

> Source/WebKit2/Platform/CoreIPC/Connection.h:103
>  #elif PLATFORM(QT)
> -    typedef const QString Identifier;
> +    typedef int Identifier;
>  #elif PLATFORM(GTK)
>      typedef int Identifier;
>  #endif

Seems that these could be combined

> Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:98
> +	   while (close(m_socketDescriptor) == -1 && errno == EINTR) {}

Space needed between { and }

> Source/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:313
> +
> +

Remove one of those newlines

> Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:55
> +	   while (close(m_fileDescriptor) == -1 && errno == EINTR) {}

space between { and }

> Source/WebKit2/Platform/qt/SharedMemoryQt.cpp:115
> +	       while (close(fileDescriptor) == -1 && errno == EINTR) {}

space between { and }, I think this is used to much that we should make a macro
for it. That would also make the code more clear

> Source/WebKit2/UIProcess/Launcher/qt/ProcessLauncherQt.cpp:114
> +    // Don't expose web socket to possible future web processes

*the* web socket