Platform code might need to get access to its PlatformMessagePortChannel object from a MessagePortChannel. i.e. Chromium's port needs it when sending a message port, so it can get the unique identifier for that port.
Created attachment 32843 [details] Proposed patch
Comment on attachment 32843 [details] Proposed patch It would be nice to mention in the bug why this is useful. > Index: WebCore/dom/MessagePortChannel.h > + // Getter for m_channel. I'd remove this comment as it doesn't really seem to add any information. > + PassRefPtr<PlatformMessagePortChannel> channel(); Since this method isn't really passing the ref count to the caller (it retains the ref counted pointer), just return PlatformMessagePortChannel* (instead of a PassRefPtr). Also, why not just define this inline? return m_channel.get();
I'm also curious about why this is necessary. The whole point behind MessagePortChannel is to create a wrapper around PlatformMessagePortChannel to make sure that you never have a raw PlatformMessagePortChannel floating around that might not get close()'d. Exposing a getter for PlatformMessagePortChannel seems dangerous (it's too easy to inadvertently create leaks) and violates the encapsulation that MessagePortChannel is intended to provide.
(In reply to comment #2) > (From update of attachment 32843 [details]) > It would be nice to mention in the bug why this is useful. I wrote a small description above (i..e re unique identifier), is that not enough? > > > Index: WebCore/dom/MessagePortChannel.h > > + // Getter for m_channel. > > I'd remove this comment as it doesn't really seem to add any information. ah, I thought it was necessary if the implementation was in a different file (which was necessary if I was returning a PassRefPtr, but as you point out below, that's not necessary). > > > + PassRefPtr<PlatformMessagePortChannel> channel(); > > Since this method isn't really passing the ref count to the caller (it retains > the ref counted pointer), just return PlatformMessagePortChannel* (instead of a > PassRefPtr). ah, I still get confused sometimes with all the RefPtr objects. Thanks, I only need a pointer. I wasn't sure if it's ok to pass around raw pointers of ref counted objects. > > Also, why not just define this inline? > return m_channel.get(); this is now possible once there are PassRefPtr (since the PlatformMessagePortChannel header isn't included here).
(In reply to comment #3) > I'm also curious about why this is necessary. The whole point behind > MessagePortChannel is to create a wrapper around PlatformMessagePortChannel to > make sure that you never have a raw PlatformMessagePortChannel floating around > that might not get close()'d. > > Exposing a getter for PlatformMessagePortChannel seems dangerous (it's too easy > to inadvertently create leaks) and violates the encapsulation that > MessagePortChannel is intended to provide. I'm going to upload the chromium port changes soon, which will make things clearer. The reason this is needed, which I hinted to above, is that when sending a message port in a postMessage call, PlatformMessagePortChannel just gets a MessagePortChannel::EventData that contains a MessagePortChannel object. In the single process case, that's enough. However for Chrome, when the object needs to be recreated in a different process, more information from the platform implementation is needed (namely, the unique identifier for this message port channel that will be sent to a different process). Checking in this patch isn't time critical (thanks for the quick review though David!), so committing it can wait until you guys see the chromium platform implementation.
Created attachment 32886 [details] new patch
Ah, that makes sense. Sorry I missed your earlier comment in the bug - I was rushing off to an interview and wanted to get my two cents in before going. And on further consideration, it's not that dangerous to have a bare reference to a PMPC, since the PMPC will get closed whenever the MPC is freed so there's no danger of lingering refs.
Comment on attachment 32886 [details] new patch I missed your earlier comment too. > ah, I still get confused sometimes with all the RefPtr objects. Thanks, I only > need a pointer. I wasn't sure if it's ok to pass around raw pointers of ref > counted objects. Actually, I wasn't aware of this guideline until recently, but it is stated well here: http://webkit.org/coding/RefPtr.html "If a function’s result is an object, but ownership is not being transferred, the result should be a raw pointer. This includes most getter functions."
Assigned to levin for landing.
Committed as http://trac.webkit.org/changeset/45996
(In reply to comment #10) > Committed as http://trac.webkit.org/changeset/45996 Thanks!