[webkit-reviews] review denied: [Bug 49791] [GTK] Implement SharedMemory for WebKit2 : [Attachment 78229] Shared Memory Implementation on latest revision

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Amruth Raj
<amruthraj at motorola.com>'s request for review:
Bug 49791: [GTK] Implement SharedMemory for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=49791

Attachment 78229: Shared Memory Implementation on latest revision
https://bugs.webkit.org/attachment.cgi?id=78229&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78229&action=review

Very nice start!

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:81
> +    : m_fileName()

No need to use an empty initializer here.

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:121
> +    char fileName[] = "/tmp/WK2SharedMemoryXXXXXX";
> +    if (mkstemp(fileName) == -1) // create a unique name
> +	   return 0;

Please use GLib functions to do this.

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:126
> +    RefPtr<SharedMemory> sharedMemory(adoptRef(new SharedMemory));
> +    sharedMemory->m_size = size;
> +    sharedMemory->m_data = createShareableMemory(fileName, size);
> +    sharedMemory->m_fileName = String(fileName);

I would much prefer these to be arguments to the constructor, if possible. It's
an error here to use cast a char* to a String. You should use filenameToString
(from FileSystemGtk) or use a CString a m_fileName, if possible.

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:149
> +    int mode = memoryProtection(protection);

You only use this once, so no need to cache it in "mode"

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:150
> +    char* fileName = const_cast<char*>(handle.m_fileName.utf8().data());

You cannot assume UTF-8 here. See above.

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:155
> +    RefPtr<SharedMemory> sharedMemory(adoptRef(new SharedMemory));
> +    sharedMemory->m_size = size;
> +    sharedMemory->m_data = obtainSharedMemory(fileName, size, mode);

Again, I think it's better to pass these as arguments to the constructor.

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:158
> +    handle.m_fileName = String();

Why is this necessary? It should probably have a comment explaining it.