Consolidate process initialization in ChildProcess
Created attachment 181402 [details] Patch
Comment on attachment 181402 [details] Patch Attachment 181402 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15709366
Comment on attachment 181402 [details] Patch Attachment 181402 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15705406
Created attachment 181404 [details] Patch
Created attachment 181408 [details] Patch
Comment on attachment 181408 [details] Patch Attachment 181408 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15701407
Comment on attachment 181408 [details] Patch Attachment 181408 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15699418
Comment on attachment 181408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181408&action=review > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:91 > - if (m_messageReceiverMap.dispatchMessage(connection, messageID, decoder)) > + if (messageReceiverMap().dispatchMessage(connection, messageID, decoder)) It's sad that we have to do this. Direct access to member variables is more transparent. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:125 > - return m_uiConnection.get(); > + return connection(); Can the name be more descriptive? Perhaps parentProcessConnection? > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:169 > + send(Messages::NetworkProcessProxy::DidCreateNetworkConnectionToWebProcess(clientPort)); I'd say that brevity is not helpful here. NetworkProcess mostly talks to WebProcess, so having a nondescript send() function that talks to an unusual receiver is confusing. > Source/WebKit2/PluginProcess/PluginProcess.cpp:169 > - m_connection->send(Messages::PluginProcessProxy::DidCreateWebProcessConnection(clientPort, m_supportsAsynchronousPluginInitialization), 0); > + send(Messages::PluginProcessProxy::DidCreateWebProcessConnection(clientPort, m_supportsAsynchronousPluginInitialization)); Here as well, send() is confusing. > Source/WebKit2/PluginProcess/PluginProcess.h:45 > -class PluginProcess : ChildProcess { > +class PluginProcess : public ChildProcess { I didn't notice why this is needed. ChildProcess is an implementation detail of the processes, and hopefully doesn't need to be exposed as a base class. > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:105 > - WebKit::PluginProcess::shared().initializeConnection(identifier); > + > + WebKit::ChildProcessInitializationParameters parameters; I don't understand these namespace prefixes. > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:110 > - WebKit::PluginProcess::shared().initializeConnection(socket); > + > + WebKit::ChildProcessInitializationParameters parameters; I don't understand these namespace prefixes. > Source/WebKit2/Shared/ChildProcess.cpp:43 > // FIXME: The termination timer should not be scheduled on the main run loop. > // It won't work with the threaded mode, but it's not really useful anyway as is. Do we still need this FIXME? > Source/WebKit2/Shared/ChildProcess.cpp:53 > + // We use _exit here since the watchdog callback is called from another thread and we don't want > + // global destructors or atexit handlers to be called from this thread while the main thread is busy This is a surprising explanation. Do we have global destructors? > Source/WebKit2/Shared/ChildProcess.cpp:62 > + static const double watchdogDelay = 10.0; Coding style: no ".0". Also, "static" is not useful for C++ constants, especially function local ones. > Source/WebKit2/Shared/ChildProcess.h:83 > + uint64_t destinationID() const { return 0; } How is this used? A dummy non-virtual function is surprising, unless a template makes use of it. > Source/WebKit2/SharedWorkerProcess/SharedWorkerProcess.h:44 > -class SharedWorkerProcess : ChildProcess { > +class SharedWorkerProcess : public ChildProcess { public :(
(In reply to comment #8) > (From update of attachment 181408 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181408&action=review > > > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:91 > > - if (m_messageReceiverMap.dispatchMessage(connection, messageID, decoder)) > > + if (messageReceiverMap().dispatchMessage(connection, messageID, decoder)) > > It's sad that we have to do this. Direct access to member variables is more transparent. Indeed, but this will be removed relatively soon. > > > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:125 > > - return m_uiConnection.get(); > > + return connection(); > > Can the name be more descriptive? Perhaps parentProcessConnection? It could be, but we would also need a function called connection() for the MessageSender template, I'll add it. > > > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:169 > > + send(Messages::NetworkProcessProxy::DidCreateNetworkConnectionToWebProcess(clientPort)); > > I'd say that brevity is not helpful here. NetworkProcess mostly talks to WebProcess, so having a nondescript send() function that talks to an unusual receiver is confusing. Ok, I'll use the new parentProcessConnection() method to make it clearer. > > > Source/WebKit2/PluginProcess/PluginProcess.cpp:169 > > - m_connection->send(Messages::PluginProcessProxy::DidCreateWebProcessConnection(clientPort, m_supportsAsynchronousPluginInitialization), 0); > > + send(Messages::PluginProcessProxy::DidCreateWebProcessConnection(clientPort, m_supportsAsynchronousPluginInitialization)); > > Here as well, send() is confusing. Ok. > > > Source/WebKit2/PluginProcess/PluginProcess.h:45 > > -class PluginProcess : ChildProcess { > > +class PluginProcess : public ChildProcess { > > I didn't notice why this is needed. > > ChildProcess is an implementation detail of the processes, and hopefully doesn't need to be exposed as a base class. It has the initialization() function on it that is called. I'll figure out a way to keep it private. > > > Source/WebKit2/PluginProcess/qt/PluginProcessMainQt.cpp:105 > > - WebKit::PluginProcess::shared().initializeConnection(identifier); > > + > > + WebKit::ChildProcessInitializationParameters parameters; > > I don't understand these namespace prefixes. This is Qt code that isn't inside the WebKit namespace for some reason, I think (will check). > > > Source/WebKit2/PluginProcess/unix/PluginProcessMainUnix.cpp:110 > > - WebKit::PluginProcess::shared().initializeConnection(socket); > > + > > + WebKit::ChildProcessInitializationParameters parameters; > > I don't understand these namespace prefixes. Same. > > > Source/WebKit2/Shared/ChildProcess.cpp:43 > > // FIXME: The termination timer should not be scheduled on the main run loop. > > // It won't work with the threaded mode, but it's not really useful anyway as is. > > Do we still need this FIXME? Probably not, will remove. > > > Source/WebKit2/Shared/ChildProcess.cpp:53 > > + // We use _exit here since the watchdog callback is called from another thread and we don't want > > + // global destructors or atexit handlers to be called from this thread while the main thread is busy > > This is a surprising explanation. Do we have global destructors? I think frameworks we link against might, but I am just guessing, this code that is just being moved. > > > Source/WebKit2/Shared/ChildProcess.cpp:62 > > + static const double watchdogDelay = 10.0; > > Coding style: no ".0". Also, "static" is not useful for C++ constants, especially function local ones. Will fix. > > > Source/WebKit2/Shared/ChildProcess.h:83 > > + uint64_t destinationID() const { return 0; } > > How is this used? A dummy non-virtual function is surprising, unless a template makes use of it. Yeah. its for MessageSender, which I am not a huge fan of, but is currently being used. > > > Source/WebKit2/SharedWorkerProcess/SharedWorkerProcess.h:44 > > -class SharedWorkerProcess : ChildProcess { > > +class SharedWorkerProcess : public ChildProcess { > > public :( :(
Actually, this patch is big enough as is. Since I got the r+, I'll do the big clean up in a follow up.
Committed r138868: <http://trac.webkit.org/changeset/138868>
This patch appears to have broken WKConnectionTest: Pass: http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/4329 Fail: http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/4330
Similarly, Passed: http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29/builds/5681 Failed: http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29/builds/5682
Also on WebKit2 bots: Pass: http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK2%20%28Tests%29/builds/5466 Fail: http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK2%20%28Tests%29/builds/5467
Should be fixed in http://trac.webkit.org/changeset/138883.