Bug 25030 - Need to refactor MessagePort to support multi-process implementations
Summary: Need to refactor MessagePort to support multi-process implementations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-03 10:41 PDT by Andrew Wilson
Modified: 2009-04-09 13:47 PDT (History)
2 users (show)

See Also:


Attachments
proposed patch (10.20 KB, patch)
2009-04-03 10:51 PDT, Andrew Wilson
no flags Details | Formatted Diff | Diff
proposed patch (10.23 KB, patch)
2009-04-03 14:07 PDT, Andrew Wilson
ap: review-
Details | Formatted Diff | Diff
proposed patch with requested changes (10.87 KB, patch)
2009-04-06 17:10 PDT, Andrew Wilson
no flags Details | Formatted Diff | Diff
proposed patch (15.09 KB, patch)
2009-04-08 18:17 PDT, Andrew Wilson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Wilson 2009-04-03 10:41:07 PDT
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.
Comment 1 Andrew Wilson 2009-04-03 10:51:30 PDT
Created attachment 29232 [details]
proposed patch

proposed patch

No new layout tests, as this is just a refactoring with no bug fixes/new functionality.
Comment 2 Andrew Wilson 2009-04-03 14:07:39 PDT
Created attachment 29241 [details]
proposed patch
Comment 3 Alexey Proskuryakov 2009-04-06 03:55:21 PDT
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.
Comment 4 Andrew Wilson 2009-04-06 17:10:20 PDT
Created attachment 29293 [details]
proposed patch with requested changes
Comment 5 Alexey Proskuryakov 2009-04-07 00:26:23 PDT
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.
Comment 6 Andrew Wilson 2009-04-08 18:17:37 PDT
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.
Comment 7 Alexey Proskuryakov 2009-04-08 22:45:26 PDT
Comment on attachment 29352 [details]
proposed patch

r=me. Please add the modified project files to ChangeLog.
Comment 8 David Levin 2009-04-09 11:15:43 PDT
Assigned to me for landing.
Comment 9 David Levin 2009-04-09 13:47:43 PDT
Committed <http://trac.webkit.org/changeset/42367>