Bug 49791 – [GTK] Implement SharedMemory for WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Amruth Raj]

SharedMemory implementation for WebKit2 GTK

[Amruth Raj]

Created attachment 74365 [details]
Implements shared memory for GTK port

[Amruth Raj]

Created attachment 78229 [details]
Shared Memory Implementation on latest revision

[Martin Robinson]

Comment on attachment 78229 [details]
Shared Memory Implementation on latest revision

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.

[Amruth Raj]

Created attachment 78527 [details]
Addressed the review comments

[Amruth Raj]

(In reply to comment #3)
> (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.
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.

[Martin Robinson]

Comment on attachment 78527 [details]
Addressed the review comments

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.

[Amruth Raj]

Created attachment 78694 [details]
Used GLib API for mmap

[Amruth Raj]

(In reply to comment #6)
> (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?
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.

[Carlos Garcia Campos]

Comment on attachment 78694 [details]
Used GLib API for mmap

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

[Amruth Raj]

Created attachment 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.

[Kenneth Rohde Christiansen]

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.

[Balazs Kelemen]

(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

[Ravi Phaneendra Kasibhatla]

(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?

[Kenneth Rohde Christiansen]

> 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.

[Kimmo Kinnunen]

(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

[Kenneth Rohde Christiansen]

> 
> 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.

[Alejandro G. Castro]

(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.

[Kimmo Kinnunen]

(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..

[Alejandro G. Castro]

(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?

[Amruth Raj]

(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 ;)

[Alejandro G. Castro]

(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.

[Alejandro G. Castro]

Created attachment 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.

[Alejandro G. Castro]

Amruth do you have any new version of this patch already?

[Amruth Raj]

Created attachment 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.

[Alejandro G. Castro]

(In reply to comment #24)
> 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.

Thanks for information Amruth :), I'm going to test and check the patch.

[Martin Robinson]

Created attachment 85553 [details]
Dusted up version of Amruth's patch with a ChangeLog

[Martin Robinson]

Created attachment 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.

[Carlos Garcia Campos]

Comment on attachment 85555 [details]
Previous patch with file movement intact

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

[Martin Robinson]

Created attachment 87395 [details]
Patch with Carlos' suggestions

[Martin Robinson]

(In reply to comment #28)
> (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

Thanks for the review Carlos. I've uploaded a new patch fixing these issues.

[Early Warning System Bot]

Attachment 87395 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8282535

[Balazs Kelemen]

Good to see that you have been successfully merged the two implementation.
Please, fix the Qt build and go ahead with the patch! ;)

[Balazs Kelemen]

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).

[Balazs Kelemen]

*** Bug 57506 has been marked as a duplicate of this bug. ***

[Martin Robinson]

(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?

[Balazs Kelemen]

(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.

[Carlos Garcia Campos]

Comment on attachment 87395 [details]
Patch with Carlos' suggestions

It seems you forgot to add the new files, and remove the Qt ones.

[Martin Robinson]

Created attachment 87737 [details]
Patch with all files

Ah, bitten again by that webkit_patch bug! I've attached the correct patch.

[Carlos Garcia Campos]

Comment on attachment 87737 [details]
Patch with all files

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.

[Martin Robinson]

Created attachment 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.

[Kimmo Kinnunen]

Comment on attachment 87745 [details]
Patch unregistering event source handler before setting the socket descriptor to -1

looks ok to me

[Kenneth Rohde Christiansen]

Comment on attachment 87745 [details]
Patch unregistering event source handler before setting the socket descriptor to -1

This looks pretty good!

[Balazs Kelemen]

Comment on attachment 87745 [details]
Patch unregistering event source handler before setting the socket descriptor to -1

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).

[Martin Robinson]

(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?

[Kenneth Rohde Christiansen]

(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! :-)

[Martin Robinson]

Committed r83215: <http://trac.webkit.org/changeset/83215>