Bug 49791 - [GTK] Implement SharedMemory for WebKit2
: [GTK] Implement SharedMemory for WebKit2
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
:
:
: 52805 57234 58030
  Show dependency treegraph
 
Reported: 2010-11-19 02:50 PST by
Modified: 2011-04-07 15:11 PST (History)


Attachments
Implements shared memory for GTK port (6.21 KB, patch)
2010-11-19 03:25 PST, Amruth Raj
no flags Review Patch | Details | Formatted Diff | Diff
Shared Memory Implementation on latest revision (6.15 KB, patch)
2011-01-07 07:13 PST, Amruth Raj
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
Addressed the review comments (6.14 KB, patch)
2011-01-11 07:18 PST, Amruth Raj
mrobinson: review-
Review Patch | Details | Formatted Diff | Diff
Used GLib API for mmap (6.56 KB, patch)
2011-01-12 08:22 PST, Amruth Raj
no flags Review Patch | Details | Formatted Diff | Diff
Reverted back to mmap implementation (6.33 KB, patch)
2011-01-21 10:03 PST, Amruth Raj
no flags Review Patch | Details | Formatted Diff | Diff
Reference patch (7.51 KB, patch)
2011-02-11 11:35 PST, Alejandro G. Castro
no flags Review Patch | Details | Formatted Diff | Diff
Shared Memory changes (10.96 KB, patch)
2011-03-07 11:52 PST, Amruth Raj
no flags Review Patch | Details | Formatted Diff | Diff
Dusted up version of Amruth's patch with a ChangeLog (10.06 KB, patch)
2011-03-11 16:35 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Previous patch with file movement intact (55.84 KB, patch)
2011-03-11 16:49 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch with Carlos' suggestions (17.07 KB, patch)
2011-03-29 12:34 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch with all files (63.04 KB, patch)
2011-03-31 08:59 PST, Martin Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Patch unregistering event source handler before setting the socket descriptor to -1 (63.04 KB, patch)
2011-03-31 09:15 PST, Martin Robinson
kenneth: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-11-19 02:50:20 PST
SharedMemory implementation for WebKit2 GTK
------- Comment #1 From 2010-11-19 03:25:34 PST -------
Created an attachment (id=74365) [details]
Implements shared memory for GTK port
------- Comment #2 From 2011-01-07 07:13:26 PST -------
Created an attachment (id=78229) [details]
Shared Memory Implementation on latest revision
------- Comment #3 From 2011-01-07 12:16:10 PST -------
(From update of attachment 78229 [details])
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.
------- Comment #4 From 2011-01-11 07:18:24 PST -------
Created an attachment (id=78527) [details]
Addressed the review comments
------- Comment #5 From 2011-01-11 07:21:54 PST -------
(In reply to comment #3)
> (From update of attachment 78229 [details] [details])
> 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.
Done.
> 
> > 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.
Done.
> 
> > 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.
The port independent SharedMemory class doesn't define any constructor. Hence, I took this approach. Other ports implement the same way. 
> 
> > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:149
> > +    int mode = memoryProtection(protection);
> 
> You only use this once, so no need to cache it in "mode"
Done.
> 
> > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:150
> > +    char* fileName = const_cast<char*>(handle.m_fileName.utf8().data());
> 
> You cannot assume UTF-8 here. See above.
Done. I used fileSystemRepresentation over here.
> 
> > 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.
Same as above.
> 
> > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:158
> > +    handle.m_fileName = String();
> 
> Why is this necessary? It should probably have a comment explaining it.
Removed this.
------- Comment #6 From 2011-01-11 09:45:23 PST -------
(From update of attachment 78527 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=78527&action=review

Looks good, but consider using GLib functions. If it's not possible, I'll re-review.

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:55
> +    if ((lseek(fd, size - 1, SEEK_SET) == -1) || (write(fd, "", 1) != 1)) {
> +        close(fd);
> +        return 0;
> +    }

Is this just verifying that the file is writable?

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:61
> +    void* mappedMemory = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> +    if (mappedMemory == MAP_FAILED) {
> +        close(fd);
> +        return 0;
> +    }

Is it possible to use GMappedFile here? I think it makes sense to use GLib function wherever possible. This will make this function platform independent and also prevent the need for the above code.

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:77
> +    void* mappedMemory = mmap(0, size, mode, MAP_SHARED, fd, 0);
> +    if (mappedMemory == MAP_FAILED) {
> +        close(fd);
> +        return 0;
> +    }

Ditto.
------- Comment #7 From 2011-01-12 08:22:15 PST -------
Created an attachment (id=78694) [details]
Used GLib API for mmap
------- Comment #8 From 2011-01-12 08:47:09 PST -------
(In reply to comment #6)
> (From update of attachment 78527 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78527&action=review
> 
> Looks good, but consider using GLib functions. If it's not possible, I'll re-review.
> 
> > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:55
> > +    if ((lseek(fd, size - 1, SEEK_SET) == -1) || (write(fd, "", 1) != 1)) {
> > +        close(fd);
> > +        return 0;
> > +    }
> 
> Is this just verifying that the file is writable?
It sets the size of the file by moving size bytes and writing a character at the end.
> 
> > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:61
> > +    void* mappedMemory = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> > +    if (mappedMemory == MAP_FAILED) {
> > +        close(fd);
> > +        return 0;
> > +    }
> 
> Is it possible to use GMappedFile here? I think it makes sense to use GLib function wherever possible. This will make this function platform independent and also prevent the need for the above code.
Done. Used GMappedFile with the new patch.
> 
> > WebKit2/Platform/gtk/SharedMemoryGtk.cpp:77
> > +    void* mappedMemory = mmap(0, size, mode, MAP_SHARED, fd, 0);
> > +    if (mappedMemory == MAP_FAILED) {
> > +        close(fd);
> > +        return 0;
> > +    }
> 
> Ditto.
Done.
------- Comment #9 From 2011-01-13 03:10:32 PST -------
(From update of attachment 78694 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=78694&action=review

Unofficial review

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:48
> +    GIOChannel* ioChannel = g_io_channel_new_file(fileName, "w+", NULL);
> +    if (!ioChannel) 
> +        return 0;

Instead of using GIOChannel, it might be easier to use gio api and use g_seekable_truncate(). It might be something like:

GRefPtr<GFile> tmpFile = g_file_new_for_path(fileName);
GRefPtr<GFileIOStream> fileStream = g_file_open_readwrite(tmpFile.get(), 0, 0); // You might want to use a GError to handle errors here
g_seekable_truncate (G_SEEKABLE (fileStream.get()), size, 0, 0); // You might want to use a GError to handle errors here
g_io_stream_close(G_IO_STREAM(fileStream.get()), 0, 0); // You might want to use a GError to handle errors here

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:65
> +    GMappedFile* mappedFile = g_mapped_file_new(fileName, TRUE, NULL);
> +    if (!mappedFile)
> +        return 0;

When g_mapped_file_new() fails it returns NULL, so you can simply return g_mapped_file_new(fileName, TRUE, 0);

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:74
> +    GMappedFile* mappedFile = g_mapped_file_new(fileName, writable, NULL);
> +    if (!mappedFile)
> +        return 0;

The same here, I think we don't even need this method, we can just use g_mapped_file_new() directly

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:117


This is not portable, use g_get_tmp_dir()

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:118
> +    if (g_mkstemp(fileName) == -1) // create a unique name

Note that g_mkstemp not only creates a unique name it also opens the file and returns the fd. The file should closed with close (fd).

> WebKit2/Platform/gtk/SharedMemoryGtk.cpp:162
> +    g_mapped_file_unref(m_mappedFile);

We should probably add support for GMappedFile file to GRefPtr
------- Comment #10 From 2011-01-21 10:03:14 PST -------
Created an attachment (id=79755) [details]
Reverted back to mmap implementation

As discussed on IRC, modifications done to the GMappedFile buffer are private to the process which does the modifications. They can't be accessed by another process. So, I reverted back to the mmap implementation. Additionally, usage of g_get_tmp_dir() and closing the fd returned by g_mkstemp are addressed with this patch.
------- Comment #11 From 2011-01-21 11:18:19 PST -------
Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation and we will probably use the Win implementation on Windows and the Mac one on Mac OS X and turn the Qt one into a general UNIX one, which could be used by you as well.
------- Comment #12 From 2011-01-23 06:10:58 PST -------
(In reply to comment #11)
> Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation and we will probably use the Win implementation on Windows and the Mac one on Mac OS X and turn the Qt one into a general UNIX one, which could be used by you as well.

You can find it here: https://bugs.webkit.org/show_bug.cgi?id=51984
------- Comment #13 From 2011-01-23 11:08:52 PST -------
(In reply to comment #11)
> Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation and we will probably use the Win implementation on Windows and the Mac one on Mac OS X and turn the Qt one into a general UNIX one, which could be used by you as well.
Kenneth,
As I see last patch of https://bugs.webkit.org/show_bug.cgi?id=51984, the patch which you approved r=me, the UNIX implementation was still present in SharedMemoryQt.cpp. Is there any patch or plan of moving that UNIX implementation outside? If yes, can you please share some more details?
------- Comment #14 From 2011-01-23 14:05:01 PST -------
> Kenneth,
> As I see last patch of https://bugs.webkit.org/show_bug.cgi?id=51984, the patch which you approved r=me, the UNIX implementation was still present in SharedMemoryQt.cpp. Is there any patch or plan of moving that UNIX implementation outside? If yes, can you please share some more details?

Yes, I talked to Kimmo, and he said that it should be little work doing so. I cc'ed him to this bug report, so he might comment on it tomorrow. We might land the patch as it is, and then to the renaming/moving as another step.
------- Comment #15 From 2011-01-24 00:31:04 PST -------
(In reply to comment #13)
> (In reply to comment #11)
> > Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation 

> Is there any patch or plan of moving that UNIX implementation outside? If yes, can you please share some more details?


So the implementation in bug 51984 can be modified to be totally Linux -only by doing (for example) following:
 - Don't use QThread for the work queue mainloop, rather use pthreads instantiate the work queue thread
 - Use the ConnectionQt::readyReadHandler as the work queue mainloop
 - Use select to poll communication socket in the workqueue mainloop
 - Use additional fd to signal that mainloop should run scheduled work item instead of hanging in select
 - Use mutexes while appending tasks to work queue
------- Comment #16 From 2011-01-24 01:50:31 PST -------
> 
> So the implementation in bug 51984 can be modified to be totally Linux -only by doing (for example) following:
>  - Don't use QThread for the work queue mainloop, rather use pthreads instantiate the work queue thread

It might be possible to use the thread abstraction from WTF as well.
------- Comment #17 From 2011-02-04 12:37:14 PST -------
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Just a heads-up. Our new Qt implementation is in reality just a UNIX implementation 
> 
> > Is there any patch or plan of moving that UNIX implementation outside? If yes, can you please share some more details?
> 
> 
> So the implementation in bug 51984 can be modified to be totally Linux -only by doing (for example) following:
>  - Don't use QThread for the work queue mainloop, rather use pthreads instantiate the work queue thread
>  - Use the ConnectionQt::readyReadHandler as the work queue mainloop
>  - Use select to poll communication socket in the workqueue mainloop
>  - Use additional fd to signal that mainloop should run scheduled work item instead of hanging in select
>  - Use mutexes while appending tasks to work queue

Are you working on this? I have Amruth shared memory patch working with gtk and I was wondering if it made sense to try to land it or wait for this, or help implementing it.
------- Comment #18 From 2011-02-04 23:05:30 PST -------
(In reply to comment #17)
> > So the implementation in bug 51984 can be modified to be totally Linux -only by doing (for example) following:
> >  - Don't use QThread for the work queue mainloop, rather use pthreads instantiate the work queue thread
> >  - Use the ConnectionQt::readyReadHandler as the work queue mainloop
> >  - Use select to poll communication socket in the workqueue mainloop
> >  - Use additional fd to signal that mainloop should run scheduled work item instead of hanging in select
> >  - Use mutexes while appending tasks to work queue
> 
> Are you working on this? I have Amruth shared memory patch working with gtk and I was wondering if it made sense to try to land it or wait for this, or help implementing it.

I'm not sure who you're asking, I at least am not working on this at the moment..
------- Comment #19 From 2011-02-05 02:47:06 PST -------
(In reply to comment #18)
> (In reply to comment #17)
>
> [...]
>
> > Are you working on this? I have Amruth shared memory patch working with gtk and I was wondering if it made sense to try to land it or wait for this, or help implementing it.
> 
> I'm not sure who you're asking, I at least am not working on this at the moment..

Yep, it was you and Amruth, thanks for commenting. Kphrane told me yesterday in the IRC that Amruth is working on it.

Last days I updated all the Minibrowser patches (bug 52805) to compile and work with current HEAD and with gtk2 and gtk3 (I used sharedmemory for gtk3 to avoid gdkpixmaps), so my plan would be to upload all of them with all these changes considering this one is solved (until we know how long it will take to finish the unix solution). With all these patches uploaded we could start working in the gtk webkit2 more easily without blocking, Does that sound good to everybody?
------- Comment #20 From 2011-02-05 05:54:25 PST -------
(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> >
> > [...]
> >
> > > Are you working on this? I have Amruth shared memory patch working with gtk and I was wondering if it made sense to try to land it or wait for this, or help implementing it.
> > 
> > I'm not sure who you're asking, I at least am not working on this at the moment..
> 
> Yep, it was you and Amruth, thanks for commenting. Kphrane told me yesterday in the IRC that Amruth is working on it.
> 
> Last days I updated all the Minibrowser patches (bug 52805) to compile and work with current HEAD and with gtk2 and gtk3 (I used sharedmemory for gtk3 to avoid gdkpixmaps), so my plan would be to upload all of them with all these changes considering this one is solved (until we know how long it will take to finish the unix solution). With all these patches uploaded we could start working in the gtk webkit2 more easily without blocking, Does that sound good to everybody?

This sounds great. The basic implementation that I've put up here works, but we definitely need to address the drawbacks mentioned in bug 51984, like passing fd's instead of filenames etc. This needs a good amount of code change for files that are already checked-in as well(for eg. Connection class, ProcessLauncher class etc.). 
Now that you have a patch based on HEAD, it's better we go ahead with all these. Meanwhile I'll create a separate bug for making the patch at 51984 generic and get it accepted by reviewers from both QT and GTK.
I'm glad that you've retained the GdkPixmap based solution for GTK2 ;)
------- Comment #21 From 2011-02-06 23:45:52 PST -------
(In reply to comment #20)
>
> [...]
>
> I'm glad that you've retained the GdkPixmap based solution for GTK2 ;)

I didn't I'm sorry :), it added a lot of cruft to the code (with ifdef's) when we can have just one solution for both versions.
------- Comment #22 From 2011-02-11 11:35:43 PST -------
Created an attachment (id=82156) [details]
Reference patch

Just for reference this is the patch I'm using for testing the other patches, I did not review it, just little modifications. I upload it here so other people can test the other patches meanwhile Amruth finishes the unix solution.
------- Comment #23 From 2011-03-07 03:41:19 PST -------
Amruth do you have any new version of this patch already?
------- Comment #24 From 2011-03-07 11:52:53 PST -------
Created an attachment (id=84965) [details]
Shared Memory changes

Alexg,
I have a basic version of the patch which pulls in required changes from Qt. I verified these on r80210. There is still some cleanup pending and a few cases to be considered ie. to handle process termination, renaming the files and more testing. I wasn't able to do more cleanup as i am stuck with some other deliverables which are taking all my time.

By the way, I intentionally did not rename the files *Qt.cpp to *Unix.cpp so as to review the delta changes easily. Had I renamed them, it would have been difficult to review the changes since it would become a new file.
------- Comment #25 From 2011-03-08 10:49:59 PST -------
(In reply to comment #24)
> Created an attachment (id=84965) [details] [details]
> Shared Memory changes
> 
> Alexg,
> I have a basic version of the patch which pulls in required changes from Qt. I verified these on r80210. There is still some cleanup pending and a few cases to be considered ie. to handle process termination, renaming the files and more testing. I wasn't able to do more cleanup as i am stuck with some other deliverables which are taking all my time.
> 
> By the way, I intentionally did not rename the files *Qt.cpp to *Unix.cpp so as to review the delta changes easily. Had I renamed them, it would have been difficult to review the changes since it would become a new file.

Thanks for information Amruth :), I'm going to test and check the patch.
------- Comment #26 From 2011-03-11 16:35:19 PST -------
Created an attachment (id=85553) [details]
Dusted up version of Amruth's patch with a ChangeLog
------- Comment #27 From 2011-03-11 16:49:13 PST -------
Created an attachment (id=85555) [details]
Previous patch with file movement intact

webkit-patch did not seem to record the file movement, so I've attached a patch I created manually.
------- Comment #28 From 2011-03-29 07:15:22 PST -------
(From update of attachment 85555 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=85555&action=review

I guess you should remove ConnectionGtk.cpp too.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:105
> +void Connection::platformInvalidate()
> +{

You should call m_connectionQueue.unregisterEventSourceHandler(m_socketDescriptor); for GTK platform here before closing the socket
------- Comment #29 From 2011-03-29 12:34:51 PST -------
Created an attachment (id=87395) [details]
Patch with Carlos' suggestions
------- Comment #30 From 2011-03-29 12:36:11 PST -------
(In reply to comment #28)
> (From update of attachment 85555 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85555&action=review
> 
> I guess you should remove ConnectionGtk.cpp too.
> 
> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:105
> > +void Connection::platformInvalidate()
> > +{
> 
> You should call m_connectionQueue.unregisterEventSourceHandler(m_socketDescriptor); for GTK platform here before closing the socket

Thanks for the review Carlos. I've uploaded a new patch fixing these issues.
------- Comment #31 From 2011-03-29 14:09:41 PST -------
Attachment 87395 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8282535
------- Comment #32 From 2011-03-30 09:10:12 PST -------
Good to see that you have been successfully merged the two implementation.
Please, fix the Qt build and go ahead with the patch! ;)
------- Comment #33 From 2011-03-30 15:55:33 PST -------
After taking a look at #57506 (that looks like a duplicate) I realized that we should think over the terminology. Actually the API's we use are all POSIX so it can be a better naming convention then UNIX. I think file names should follow the style guideline so acronyms should be capitalized (FooPOSIX.cpp instead of FooPosix.cpp). This is how the JIT files are named for example. Maybe a new macro flag would be useful to replace the "PLATFORM(QT) || PLATFORM(GTK)" conditions with smg like USE(POSIX_IPC).
------- Comment #34 From 2011-03-30 15:58:15 PST -------
*** Bug 57506 has been marked as a duplicate of this bug. ***
------- Comment #35 From 2011-03-30 16:42:15 PST -------
(In reply to comment #33)
> After taking a look at #57506 (that looks like a duplicate) I realized that we should think over the terminology. Actually the API's we use are all POSIX so it can be a better naming convention then UNIX. I think file names should follow the style guideline so acronyms should be capitalized (FooPOSIX.cpp instead of FooPosix.cpp). This is how the JIT files are named for example. Maybe a new macro flag would be useful to replace the "PLATFORM(QT) || PLATFORM(GTK)" conditions with smg like USE(POSIX_IPC).

I'm not sure that POSIX is the appropriate terminology for all of these files. ConnectionUnix makes heavy use of ancillary data structures (cmsghdr) to pass file handles through sockets. As far as I know this is not defined in POSIX at all. Perhaps we could simply have a greater distinction:

AttachmentPOSIX.cpp
SharedMemoryPOSIX.cpp
ConnectionUnix.cpp

How does that sound?
------- Comment #36 From 2011-03-31 01:38:23 PST -------
(In reply to comment #35)
> (In reply to comment #33)
> > After taking a look at #57506 (that looks like a duplicate) I realized that we should think over the terminology. Actually the API's we use are all POSIX so it can be a better naming convention then UNIX. I think file names should follow the style guideline so acronyms should be capitalized (FooPOSIX.cpp instead of FooPosix.cpp). This is how the JIT files are named for example. Maybe a new macro flag would be useful to replace the "PLATFORM(QT) || PLATFORM(GTK)" conditions with smg like USE(POSIX_IPC).
> 
> I'm not sure that POSIX is the appropriate terminology for all of these files. ConnectionUnix makes heavy use of ancillary data structures (cmsghdr) to pass file handles through sockets. As far as I know this is not defined in POSIX at all. Perhaps we could simply have a greater distinction:
> 
> AttachmentPOSIX.cpp
> SharedMemoryPOSIX.cpp
> ConnectionUnix.cpp
> 
> How does that sound?

Correct, I was wrong about POSIX. This makes it more complicated. In this case I would better like to just use UNIX everywhere since POSIX is part of it. In that case only the capitalization should be changed in your patch. But your new proposal is also consistent so we can chose it.
------- Comment #37 From 2011-03-31 02:53:50 PST -------
(From update of attachment 87395 [details])
It seems you forgot to add the new files, and remove the Qt ones.
------- Comment #38 From 2011-03-31 08:59:01 PST -------
Created an attachment (id=87737) [details]
Patch with all files

Ah, bitten again by that webkit_patch bug! I've attached the correct patch.
------- Comment #39 From 2011-03-31 09:07:12 PST -------
(From update of attachment 87737 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=87737&action=review

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:122
> +#if PLATFORM(GTK)
> +    m_connectionQueue.unregisterEventSourceHandler(m_socketDescriptor);
> +#endif

This should be called before closing the socket.
------- Comment #40 From 2011-03-31 09:15:12 PST -------
Created an attachment (id=87745) [details]
Patch unregistering event source handler before setting the socket descriptor to -1

Nice catch Carlos! I've updated the patch to unregister the event source *before* setting m_socketDescriptor to -1.
------- Comment #41 From 2011-04-05 06:21:32 PST -------
(From update of attachment 87745 [details])
looks ok to me
------- Comment #42 From 2011-04-06 01:49:58 PST -------
(From update of attachment 87745 [details])
This looks pretty good!
------- Comment #43 From 2011-04-06 05:43:36 PST -------
(From update of attachment 87745 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=87745&action=review

> Source/WebKit2/ChangeLog:20
> +        * Platform/CoreIPC/unix/AttachmentUnix.cpp: Renamed from Source/WebKit2/Platform/CoreIPC/qt/AttachmentQt.cpp.

The Changelog is out of date. The file you add is AttachmentPOSIX.cpp.
Please fix this before landing.

> Source/WebKit2/GNUmakefile.am:100
> +	Source/WebKit2/Platform/CoreIPC/posix/AttachmentPOSIX.cpp \

I'm not a fan of adding two new terminology with new dirs and file name patterns. (Just my personal opinion, should not block the patch).
------- Comment #44 From 2011-04-06 07:14:33 PST -------
(In reply to comment #43)

> > Source/WebKit2/GNUmakefile.am:100
> > +	Source/WebKit2/Platform/CoreIPC/posix/AttachmentPOSIX.cpp \
> 
> I'm not a fan of adding two new terminology with new dirs and file name patterns. (Just my personal opinion, should not block the patch).

I prefer using "Unix" everywhere as well. Kenneth, does your review still hold with that change?
------- Comment #45 From 2011-04-07 05:14:31 PST -------
(In reply to comment #44)
> (In reply to comment #43)
> 
> > > Source/WebKit2/GNUmakefile.am:100
> > > +	Source/WebKit2/Platform/CoreIPC/posix/AttachmentPOSIX.cpp \
> > 
> > I'm not a fan of adding two new terminology with new dirs and file name patterns. (Just my personal opinion, should not block the patch).
> 
> I prefer using "Unix" everywhere as well. Kenneth, does your review still hold with that change?

Yes! :-)
------- Comment #46 From 2011-04-07 15:11:22 PST -------
Committed r83215: <http://trac.webkit.org/changeset/83215>