[webkit-reviews] review granted: [Bug 106144] Consolidate process initialization in ChildProcess : [Attachment 181408] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 106144: Consolidate process initialization in ChildProcess
https://bugs.webkit.org/show_bug.cgi?id=106144

Attachment 181408: Patch
https://bugs.webkit.org/attachment.cgi?id=181408&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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(clie
ntPort));

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