[Webkit-unassigned] [Bug 49791] [GTK] Implement SharedMemory for WebKit2

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78229|review?                     |review-
               Flag|                            |




--- Comment #3 from Martin Robinson <mrobinson at webkit.org>  2011-01-07 12:16:10 PST ---
(From update of attachment 78229)
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.

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