Bug 109032

Summary: [WK2][Unix][Refactoring] Get rid of socket creation code duplicates
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: PlatformAssignee: Balazs Kelemen <kbalazs>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: abecsi, andersca, beidson, cgarcia, cmarcelo, gyuyoung.kim, hausmann, kenneth, laszlo.gombos, menard, mrobinson, rakuco, tmpsantos, webkit-ews, webkit.review.bot, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108832    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Balazs Kelemen
Reported 2013-02-06 02:37:27 PST
The socket creation code is duplicated in a lot places, let's get rid of that.
Attachments
Patch (19.53 KB, patch)
2013-02-06 05:55 PST, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Patch (19.45 KB, patch)
2013-02-06 07:07 PST, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Patch (19.42 KB, patch)
2013-02-09 13:43 PST, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Thiago Marcos P. Santos
Comment 1 2013-02-06 02:45:55 PST
(In reply to comment #0) > The socket creation code is duplicated in a lot places, let's get rid of that. What socket creation code are you referring to here?
Balazs Kelemen
Comment 2 2013-02-06 05:55:43 PST
Created attachment 186838 [details] Patch
Early Warning System Bot
Comment 3 2013-02-06 06:05:55 PST
Comment on attachment 186838 [details] Patch Attachment 186838 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/16389253
Balazs Kelemen
Comment 4 2013-02-06 07:07:01 PST
Created attachment 186849 [details] Patch
Balazs Kelemen
Comment 5 2013-02-08 07:18:36 PST
Anybody? Although we have owners I think it's better if somebody how is a maintainer of an affected port would do an informal review first.
Martin Robinson
Comment 6 2013-02-08 07:48:35 PST
Carlos, the one who would know the most about this code, is on vacation until Monday. Do you think you could get a review from him then?
Balazs Kelemen
Comment 7 2013-02-08 07:52:55 PST
(In reply to comment #6) > Carlos, the one who would know the most about this code, is on vacation until Monday. Do you think you could get a review from him then? Sure, that's fine.
Carlos Garcia Campos
Comment 8 2013-02-09 03:16:24 PST
Comment on attachment 186849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186849&action=review This is a great cleanup, thanks!. Every time I see that code duplicated I think we should refactor it. I have a couple of questions, though. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:142 > + while (fcntl(sockets[0], F_SETFD, FD_CLOEXEC) == -1) { I guess this should be sockets[i]. > Source/WebKit2/PluginProcess/PluginProcess.cpp:162 > - RefPtr<WebProcessConnection> connection = WebProcessConnection::create(sockets[1]); > + RefPtr<WebProcessConnection> connection = WebProcessConnection::create(serverIdentifier); Is there any reason why you are doing the opposite of what the previous code did? serverIdentifier is sockets[0], it doesn't really matter, since both sockets are indistinguishable, but I'm curious. > Source/WebKit2/PluginProcess/PluginProcess.cpp:165 > - CoreIPC::Attachment clientSocket(sockets[0]); > + CoreIPC::Attachment clientSocket(clientIdentifier); Ditto.
Balazs Kelemen
Comment 9 2013-02-09 09:01:51 PST
(In reply to comment #8) > (From update of attachment 186849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186849&action=review > > This is a great cleanup, thanks!. Every time I see that code duplicated I think we should refactor it. I have a couple of questions, though. > > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:142 > > + while (fcntl(sockets[0], F_SETFD, FD_CLOEXEC) == -1) { > > I guess this should be sockets[i]. Oops. Sure, will fix. > > > Source/WebKit2/PluginProcess/PluginProcess.cpp:162 > > - RefPtr<WebProcessConnection> connection = WebProcessConnection::create(sockets[1]); > > + RefPtr<WebProcessConnection> connection = WebProcessConnection::create(serverIdentifier); > > Is there any reason why you are doing the opposite of what the previous code did? serverIdentifier is sockets[0], it doesn't really matter, since both sockets are indistinguishable, but I'm curious. No reason, just I was finding it more straightforward to assign the first one to the first argument.
Balazs Kelemen
Comment 10 2013-02-09 13:43:45 PST
Created attachment 187444 [details] Patch
Carlos Garcia Campos
Comment 11 2013-02-10 23:55:49 PST
Comment on attachment 187444 [details] Patch LGTM, thanks! We need an owner to r+ this, though.
Brady Eidson
Comment 12 2013-02-21 14:04:27 PST
Anders should look at this one.
Balazs Kelemen
Comment 13 2013-02-27 07:10:49 PST
As requested in https://bugs.webkit.org/show_bug.cgi?id=110093#c6 I am going to unify these ipc stuff across every port. *** This bug has been marked as a duplicate of bug 110978 ***
Note You need to log in before you can comment on or make changes to this bug.