Bug 106144 – Consolidate process initialization in ChildProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Consolidate process initialization in ChildProcess

[Sam Weinig]

Created attachment 181402 [details]
Patch

[Early Warning System Bot]

Comment on attachment 181402 [details]
Patch

Attachment 181402 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15709366

[EFL EWS Bot]

Comment on attachment 181402 [details]
Patch

Attachment 181402 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15705406

[Sam Weinig]

Created attachment 181404 [details]
Patch

[Sam Weinig]

Created attachment 181408 [details]
Patch

[Early Warning System Bot]

Comment on attachment 181408 [details]
Patch

Attachment 181408 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15701407

[EFL EWS Bot]

Comment on attachment 181408 [details]
Patch

Attachment 181408 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15699418

[Alexey Proskuryakov]

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

[Sam Weinig]

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

:(

[Sam Weinig]

Actually, this patch is big enough as is.  Since I got the r+, I'll do the big clean up in a follow up.

[Sam Weinig]

Committed r138868: <http://trac.webkit.org/changeset/138868>

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Ryosuke Niwa]

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

[Sam Weinig]

Should be fixed in http://trac.webkit.org/changeset/138883.