27337 2009-07-16 01:23:30 -0700 Add a getter in MessagePortChannel for the PlatformMessagePortChannel 2009-07-16 23:45:56 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 jam levin atwilson fishd levin oldest_to_newest 132193 0 jam 2009-07-16 01:23:30 -0700 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. 132197 1 32843 jam 2009-07-16 01:28:27 -0700 Created attachment 32843 Proposed patch 132201 2 32843 levin 2009-07-16 01:46:52 -0700 Comment on attachment 32843 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(); 132288 3 atwilson 2009-07-16 09:50:57 -0700 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. 132309 4 jam 2009-07-16 12:06:13 -0700 (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). 132313 5 jam 2009-07-16 12:10:24 -0700 (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. 132314 6 32886 jam 2009-07-16 12:10:49 -0700 Created attachment 32886 new patch 132315 7 atwilson 2009-07-16 12:21:13 -0700 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. 132326 8 32886 levin 2009-07-16 13:08:31 -0700 Comment on attachment 32886 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." 132330 9 levin 2009-07-16 13:19:11 -0700 Assigned to levin for landing. 132425 10 levin 2009-07-16 17:38:58 -0700 Committed as http://trac.webkit.org/changeset/45996 132496 11 jam 2009-07-16 23:45:56 -0700 (In reply to comment #10) > Committed as http://trac.webkit.org/changeset/45996 Thanks! 32843 2009-07-16 01:28:27 -0700 2009-07-16 12:10:49 -0700 Proposed patch mp_getter.txt text/plain 1889 jam SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA0NTk2OCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMDktMDctMTYgIEpvaG4gQWJkLUVsLU1hbGVrICA8amFtQGNocm9t aXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAg ICBBZGQgYSBnZXR0ZXIgaW4gTWVzc2FnZVBvcnRDaGFubmVsIGZvciB0aGUgUGxhdGZvcm1NZXNz YWdlUG9ydENoYW5uZWwuCisKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTI3MzM3CisKKyAgICAgICAgKiBkb20vTWVzc2FnZVBvcnRDaGFubmVsLmg6Cisg ICAgICAgICogZG9tL2RlZmF1bHQvUGxhdGZvcm1NZXNzYWdlUG9ydENoYW5uZWwuY3BwOgorICAg ICAgICAoV2ViQ29yZTo6TWVzc2FnZVBvcnRDaGFubmVsOjpjaGFubmVsKToKKwogMjAwOS0wNy0x NSAgU2hpbmljaGlybyBIYW1hamkgIDxoYW1hamlAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJl dmlld2VkIGJ5IEVyaWMgU2VpZGVsLgpJbmRleDogV2ViQ29yZS9kb20vTWVzc2FnZVBvcnRDaGFu bmVsLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9kb20vTWVzc2FnZVBvcnRDaGFubmVsLmgJKHJl dmlzaW9uIDQ1OTY3KQorKysgV2ViQ29yZS9kb20vTWVzc2FnZVBvcnRDaGFubmVsLmgJKHdvcmtp bmcgY29weSkKQEAgLTk1LDYgKzk1LDkgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogCiAgICAgICAg IH5NZXNzYWdlUG9ydENoYW5uZWwoKTsKIAorICAgICAgICAvLyBHZXR0ZXIgZm9yIG1fY2hhbm5l bC4KKyAgICAgICAgUGFzc1JlZlB0cjxQbGF0Zm9ybU1lc3NhZ2VQb3J0Q2hhbm5lbD4gY2hhbm5l bCgpOworCiAgICAgcHJpdmF0ZToKICAgICAgICAgTWVzc2FnZVBvcnRDaGFubmVsKFBhc3NSZWZQ dHI8UGxhdGZvcm1NZXNzYWdlUG9ydENoYW5uZWw+KTsKICAgICAgICAgUmVmUHRyPFBsYXRmb3Jt TWVzc2FnZVBvcnRDaGFubmVsPiBtX2NoYW5uZWw7CkluZGV4OiBXZWJDb3JlL2RvbS9kZWZhdWx0 L1BsYXRmb3JtTWVzc2FnZVBvcnRDaGFubmVsLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJDb3JlL2Rv bS9kZWZhdWx0L1BsYXRmb3JtTWVzc2FnZVBvcnRDaGFubmVsLmNwcAkocmV2aXNpb24gNDU5Njcp CisrKyBXZWJDb3JlL2RvbS9kZWZhdWx0L1BsYXRmb3JtTWVzc2FnZVBvcnRDaGFubmVsLmNwcAko d29ya2luZyBjb3B5KQpAQCAtOTgsNiArOTgsMTEgQEAgTWVzc2FnZVBvcnQqIE1lc3NhZ2VQb3J0 Q2hhbm5lbDo6bG9jYWxseQogICAgIHJldHVybiBtX2NoYW5uZWwtPmxvY2FsbHlFbnRhbmdsZWRQ b3J0KGNvbnRleHQpOwogfQogCitQYXNzUmVmUHRyPFBsYXRmb3JtTWVzc2FnZVBvcnRDaGFubmVs PiBNZXNzYWdlUG9ydENoYW5uZWw6OmNoYW5uZWwoKQoreworICAgIHJldHVybiBtX2NoYW5uZWw7 Cit9CisKIFBhc3NSZWZQdHI8UGxhdGZvcm1NZXNzYWdlUG9ydENoYW5uZWw+IFBsYXRmb3JtTWVz c2FnZVBvcnRDaGFubmVsOjpjcmVhdGUoUGFzc1JlZlB0cjxNZXNzYWdlUG9ydFF1ZXVlPiBpbmNv bWluZywgUGFzc1JlZlB0cjxNZXNzYWdlUG9ydFF1ZXVlPiBvdXRnb2luZykKIHsKICAgICByZXR1 cm4gYWRvcHRSZWYobmV3IFBsYXRmb3JtTWVzc2FnZVBvcnRDaGFubmVsKGluY29taW5nLCBvdXRn b2luZykpOwo= 32886 2009-07-16 12:10:49 -0700 2009-07-16 13:08:31 -0700 new patch mp_getter2.txt text/plain 0 jam