Bug 25030 – Need to refactor MessagePort to support multi-process implementations

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Andrew Wilson]

The current implementation of MessagePort has a direct reference between a port and its entangled pair. This works when both ports are running in the same process, but won't work when the entangled port lives in a separate process.

We need to refactor the MessagePort code to expose a RemotePort interface - this allows multi-process implementations to use a proxy object as the entangled pair.

[Andrew Wilson]

Created attachment 29232 [details]
proposed patch

proposed patch

No new layout tests, as this is just a refactoring with no bug fixes/new functionality.

[Andrew Wilson]

Created attachment 29241 [details]
proposed patch

[Alexey Proskuryakov]

Comment on attachment 29241 [details]
proposed patch

> +        * dom/RemotePort.h: Added.
> +        (WebCore::RemotePort::~RemotePort):

RemotePort doesn't look like a great name to me - first, it doesn't say that it's all about MessagePorts, and then, it's not clear in which sense it is "remote". Other similar interfaces have the word Proxy in them, maybe MessagePortProxy would fit better?

> +    m_entangledPort->deliverRemoteMessage(message, newMessagePort);

Again, a naming nitpick. Are there any non-remote messages? This function name makes it look like there are.

> +        virtual void deliverRemoteMessage(const String& message, PassRefPtr<MessagePort>) = 0;

There is no need to include PlatformString.h just for this, a forward declaration would suffice.

These are all nitpicks, but the patch mostly does renaming, so I think it's fair to be picky about names.

Besides renaming, there are changes to how entangling is implemented, it would be nice to provide a rationale in the ChangeLog.

[Andrew Wilson]

Created attachment 29293 [details]
proposed patch with requested changes

[Alexey Proskuryakov]

Something I didn't notice first time: please add the new file to projects (at least Mac, Windows and GNUmakefile.am). Looks good otherwise.

Did you enable and run the existing MessagePort and MessageChannel tests in fast/events? I'd be interested in results of a "run-webkit-tests --leaks"  run for those.

[Andrew Wilson]

Created attachment 29352 [details]
proposed patch

Added MessagePortProxy.h to the various project files.

Also ran all of the disabled-by-default MessagePort tests. They all passed (including the one in http/tests/security/MessagePort) - I swear that one was failing yesterday but I can't reproduce any problems today after an update + clean build.

[Alexey Proskuryakov]

Comment on attachment 29352 [details]
proposed patch

r=me. Please add the modified project files to ChangeLog.

[David Levin]

Assigned to me for landing.

[David Levin]

Committed <http://trac.webkit.org/changeset/42367>