136613 2014-09-07 14:33:24 -0700 Make OSObjectPtr a bit more like RefPtr 2014-10-03 17:37:46 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 136636 1 sam sam benjamin cmarcelo commit-queue darin koivisto mitz thorton oldest_to_newest 1033752 0 sam 2014-09-07 14:33:24 -0700 Make OSObjectPtr a bit more like RefPtr 1033753 1 237757 sam 2014-09-07 14:35:49 -0700 Created attachment 237757 Patch 1033755 2 sam 2014-09-07 14:36:56 -0700 This bug is for addressing the feedback in https://bugs.webkit.org/show_bug.cgi?id=136613. 1033756 3 sam 2014-09-07 14:39:27 -0700 Two things I didn't do were: - Remove the specialization for nullptr as I don't trust compilers :). - Made it share code with RefPtr, as that would be a much bigger task. 1033764 4 darin 2014-09-07 18:01:07 -0700 (In reply to comment #3) > - Remove the specialization for nullptr as I don't trust compilers :). But you didn’t add one for RefPtr. You trust compilers only with a class that we use extensively across the entire project, not with a class that’s far less often used? 1033766 5 237757 darin 2014-09-07 18:03:08 -0700 Comment on attachment 237757 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237757&action=review > Source/WTF/wtf/OSObjectPtr.h:52 > struct AdoptOSObject { }; Now that this is not part of the public API of OSObjectPtr, could make this struct a private member of the OSObjectPtr class. Would be tidier. > Source/WTF/wtf/OSObjectPtr.h:122 > + OSObjectPtr& operator=(const OSObjectPtr&& other) Unwanted const here prevents the patch from compiling. 1033882 6 sam 2014-09-08 10:15:26 -0700 (In reply to comment #4) > (In reply to comment #3) > > - Remove the specialization for nullptr as I don't trust compilers :). > > But you didn’t add one for RefPtr. You trust compilers only with a class that we use extensively across the entire project, not with a class that’s far less often used? I'm writing some test code to figure out if it really is the same. When I finish that experiment, I will either add the code to RefPtr, or remove it from OSObjectPtr. 1033883 7 sam 2014-09-08 10:16:09 -0700 (In reply to comment #5) > (From update of attachment 237757 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=237757&action=review > > > Source/WTF/wtf/OSObjectPtr.h:52 > > struct AdoptOSObject { }; > > Now that this is not part of the public API of OSObjectPtr, could make this struct a private member of the OSObjectPtr class. Would be tidier. Will fix. > > > Source/WTF/wtf/OSObjectPtr.h:122 > > + OSObjectPtr& operator=(const OSObjectPtr&& other) > > Unwanted const here prevents the patch from compiling. Do'h. 1033888 8 sam 2014-09-08 10:26:44 -0700 Committed r173383: <http://trac.webkit.org/changeset/173383> 1033893 9 thorton 2014-09-08 10:38:20 -0700 Hopeful build fix in http://trac.webkit.org/changeset/173384 1033896 10 commit-queue 2014-09-08 11:00:50 -0700 Re-opened since this is blocked by bug 136636 1037483 11 koivisto 2014-09-26 08:25:13 -0700 How is this better than just having adoptOSObject() function for RetainPtr? That's what I was doing before discovering this and it seemed to work fine. 1037549 12 sam 2014-09-26 13:44:36 -0700 (In reply to comment #11) > How is this better than just having adoptOSObject() function for RetainPtr? That's what I was doing before discovering this and it seemed to work fine. It's not. And I will be rolling this out pretty soon. 1037645 13 koivisto 2014-09-27 00:10:03 -0700 This seemed to work on all platforms: + template<typename T> inline RetainPtr<T> adoptSystem(T ptr) + { +#if OS_OBJECT_USE_OBJC + return adoptNS(ptr); +#else + return adoptCF(ptr); +#endif + } + 1039420 14 sam 2014-10-03 17:37:46 -0700 (In reply to comment #13) > This seemed to work on all platforms: > > + template<typename T> inline RetainPtr<T> adoptSystem(T ptr) > + { > +#if OS_OBJECT_USE_OBJC > + return adoptNS(ptr); > +#else > + return adoptCF(ptr); > +#endif > + } > + Actually, I don't believe this actually works. In 32-bit, OS-objects are not objective-c objects, but also not CFObjects, so they can't be CFRetain()/CFReleased(). I think we are going to need to stick with OSObjectPtr for now. 237757 2014-09-07 14:35:49 -0700 2014-09-07 18:03:07 -0700 Patch bug-136613-20140907143544.patch text/plain 2372 sam SW5kZXg6IFNvdXJjZS9XVEYvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XVEYvQ2hh bmdlTG9nCShyZXZpc2lvbiAxNzMzNjgpCisrKyBTb3VyY2UvV1RGL0NoYW5nZUxvZwkod29ya2lu ZyBjb3B5KQpAQCAtMSwzICsxLDIwIEBACisyMDE0LTA5LTA3ICBTYW0gV2VpbmlnICA8c2FtQHdl YmtpdC5vcmc+CisKKyAgICAgICAgTWFrZSBPU09iamVjdFB0ciBhIGJpdCBtb3JlIGxpa2UgUmVm UHRyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzY2 MTMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBZGRy ZXNzIHNvbWUgb2YgRGFyaW4ncyBmZWVkYmFjayBieToKKyAgICAgICAgLSBNYWtpbmcgdGhlIGFk b3B0aW5nIGNvbnN0cnVjdG9yIHByaXZhdGUgYW5kIGZyaWVuZGluZyBhZG9wdE9TT2JqZWN0KCku CisgICAgICAgIC0gSW1wbGVtZW50aW5nIHRoZSBhc3NpZ25tZW50IG9wZXJhdG9yIHVzaW5nIHN3 YXAuCisgICAgICAgIC0gQWRkaW5nIGEgbW92ZSBhc3NpZ25tZW50IG9wZXJhdG9yLgorCisgICAg ICAgICogd3RmL09TT2JqZWN0UHRyLmg6CisgICAgICAgIChXVEY6Ok9TT2JqZWN0UHRyOjpvcGVy YXRvcj0pOgorICAgICAgICAoV1RGOjpPU09iamVjdFB0cjo6c3dhcCk6CisgICAgICAgIChXVEY6 Ok9TT2JqZWN0UHRyOjpPU09iamVjdFB0cik6CisKIDIwMTQtMDktMDYgIFNhbSBXZWluaWcgIDxz YW1Ad2Via2l0Lm9yZz4KIAogICAgICAgICBYUENQdHIgc2hvdWxkIGJlIGNvbnZlcnRlZCBpbnRv IGFuIGFsbCBwdXJwb3NlIHNtYXJ0IHBvaW50ZXIgZm9yIG9zX29iamVjdHMKSW5kZXg6IFNvdXJj ZS9XVEYvd3RmL09TT2JqZWN0UHRyLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dURi93dGYvT1NP YmplY3RQdHIuaAkocmV2aXNpb24gMTczMzY3KQorKysgU291cmNlL1dURi93dGYvT1NPYmplY3RQ dHIuaAkod29ya2luZyBjb3B5KQpAQCAtNTAsNiArNTAsOCBAQCBleHRlcm4gIkMiIHZvaWQgX29z X29iamVjdF9yZWxlYXNlKF9vc19vCiBuYW1lc3BhY2UgV1RGIHsKIAogc3RydWN0IEFkb3B0T1NP YmplY3QgeyB9OwordGVtcGxhdGU8dHlwZW5hbWUgVD4gY2xhc3MgT1NPYmplY3RQdHI7Cit0ZW1w bGF0ZTx0eXBlbmFtZSBUPiBPU09iamVjdFB0cjxUPiBhZG9wdE9TT2JqZWN0KFQpOwogCiB0ZW1w bGF0ZTx0eXBlbmFtZSBUPgogc3RhdGljIGlubGluZSB2b2lkIHJldGFpbk9TT2JqZWN0KFQgcHRy KQpAQCAtODYsMTEgKzg4LDYgQEAgcHVibGljOgogICAgIHsKICAgICB9CiAKLSAgICBPU09iamVj dFB0cihBZG9wdE9TT2JqZWN0LCBUIHB0cikKLSAgICAgICAgOiBtX3B0cihwdHIpCi0gICAgewot ICAgIH0KLQogICAgIH5PU09iamVjdFB0cigpCiAgICAgewogICAgICAgICBpZiAobV9wdHIpCkBA IC0xMTcsMTYgKzExNCwxNSBAQCBwdWJsaWM6CiAKICAgICBPU09iamVjdFB0ciYgb3BlcmF0b3I9 KGNvbnN0IE9TT2JqZWN0UHRyJiBvdGhlcikKICAgICB7Ci0gICAgICAgIFQgb3B0ciA9IG90aGVy LmdldCgpOwotICAgICAgICBpZiAob3B0cikKLSAgICAgICAgICAgIHJldGFpbk9TT2JqZWN0KG9w dHIpOwotCi0gICAgICAgIFQgcHRyID0gbV9wdHI7Ci0gICAgICAgIG1fcHRyID0gb3B0cjsKLQot ICAgICAgICBpZiAocHRyKQotICAgICAgICAgICAgcmVsZWFzZU9TT2JqZWN0KHB0cik7CisgICAg ICAgIE9TT2JqZWN0UHRyIHB0ciA9IG90aGVyOworICAgICAgICBzd2FwKHB0cik7CisgICAgICAg IHJldHVybiAqdGhpczsKKyAgICB9CiAKKyAgICBPU09iamVjdFB0ciYgb3BlcmF0b3I9KGNvbnN0 IE9TT2JqZWN0UHRyJiYgb3RoZXIpCisgICAgeworICAgICAgICBPU09iamVjdFB0ciBwdHIgPSBX VEY6Om1vdmUob3RoZXIpOworICAgICAgICBzd2FwKHB0cik7CiAgICAgICAgIHJldHVybiAqdGhp czsKICAgICB9CiAKQEAgLTEzOCw3ICsxMzQsMjAgQEAgcHVibGljOgogCiAgICAgICAgIHJldHVy biAqdGhpczsKICAgICB9CisKKyAgICB2b2lkIHN3YXAoT1NPYmplY3RQdHImIG90aGVyKQorICAg IHsKKyAgICAgICAgc3RkOjpzd2FwKG1fcHRyLCBvdGhlci5tX3B0cik7CisgICAgfQorCisgICAg ZnJpZW5kIE9TT2JqZWN0UHRyIGFkb3B0T1NPYmplY3Q8VD4oVCk7CisKIHByaXZhdGU6CisgICAg T1NPYmplY3RQdHIoQWRvcHRPU09iamVjdCwgVCBwdHIpCisgICAgICAgIDogbV9wdHIocHRyKQor ICAgIHsKKyAgICB9CisKICAgICBUIG1fcHRyOwogfTsKIAo=