62067 2011-06-03 16:40:23 -0700 [chromium] Give VoidCallbackClient a virtual destructor 2011-06-08 17:05:34 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 thakis thakis evan fishd webkit.review.bot oldest_to_newest 415281 0 thakis 2011-06-03 16:40:23 -0700 [chromium] Give VoidCallbackClient a virtual destructor 415282 1 95986 thakis 2011-06-03 16:41:37 -0700 Created attachment 95986 Patch 415290 2 95986 jamesr 2011-06-03 16:50:11 -0700 Comment on attachment 95986 Patch You should add virtual to WebPermissionCallback's destructor declaration as well. 415293 3 thakis 2011-06-03 16:51:19 -0700 (In reply to comment #2) > (From update of attachment 95986 [details]) > You should add virtual to WebPermissionCallback's destructor declaration as well. Why? 415451 4 95986 webkit.review.bot 2011-06-04 03:51:04 -0700 Comment on attachment 95986 Patch Clearing flags on attachment: 95986 Committed r88106: <http://trac.webkit.org/changeset/88106> 415452 5 webkit.review.bot 2011-06-04 03:51:08 -0700 All reviewed patches have been landed. Closing bug. 417685 6 95986 fishd 2011-06-08 16:42:11 -0700 Comment on attachment 95986 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95986&action=review > Source/WebKit/chromium/src/NotificationPresenterImpl.cpp:59 > + virtual ~VoidCallbackClient() { } this should be protected or private!! notice how this class deletes itself. no one else should be deleting this class. 417690 7 thakis 2011-06-08 16:48:11 -0700 (In reply to comment #6) > this should be protected or private!! notice how this class deletes itself. > no one else should be deleting this class. https://bugs.webkit.org/show_bug.cgi?id=62341 (Note that it was public before, so I didn't really change this.) 417691 8 thakis 2011-06-08 16:48:50 -0700 Also, I don't know if it's customary to r- patches that have already been submitted? 417697 9 fishd 2011-06-08 16:58:06 -0700 (In reply to comment #7) > (In reply to comment #6) > > this should be protected or private!! notice how this class deletes itself. > > no one else should be deleting this class. > > https://bugs.webkit.org/show_bug.cgi?id=62341 > > (Note that it was public before, so I didn't really change this.) Right, but a quick inspection reveals that this callback is intended to only delete itself. I just think that's an important convention to preserve. Sorry if the R- comes off as harsh. It is just how I would have responded had the patch not yet been committed. (Didn't realize the patch was in the tree.) 417699 10 thakis 2011-06-08 17:05:34 -0700 > Right, but a quick inspection reveals that this callback is intended to only delete itself. I just think that's an important convention to preserve. I went through all these changes I did in the last few days and made all destructors private that look like they should be private: https://bugs.webkit.org/show_bug.cgi?id=62341 95986 2011-06-03 16:41:37 -0700 2011-06-08 16:42:11 -0700 Patch bug-62067-20110603164136.patch text/plain 1482 thakis U3VidmVyc2lvbiBSZXZpc2lvbjogODc2MjcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKaW5kZXgg OGMwZjIwMGNmZTI3NzdmMzVjNTMwNTE1ZWY5ZGVkMGMxMzBjY2E3ZC4uYzgwOTIxOTM0YmQ5YzQ1 Y2E3MjNjYWU3MGU0OWQ0ZTkzNmU5ZjliMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9jaHJv bWl1bS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKQEAg LTEsMyArMSwxOCBAQAorMjAxMS0wNi0wMyAgTmljbyBXZWJlciAgPHRoYWtpc0BjaHJvbWl1bS5v cmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgW2No cm9taXVtXSBHaXZlIFZvaWRDYWxsYmFja0NsaWVudCBhIHZpcnR1YWwgZGVzdHJ1Y3RvcgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjIwNjcKKworICAg ICAgICBUaGlzIGlzIF9ub3RfIHRvIGZpeCBhIHJlYWwgYnVnLCBqdXN0IHRvIG1ha2UgY2xhbmcn cworICAgICAgICAtV2RlbGV0ZS1ub24tdmlydHVhbC1kdG9yIGhhcHB5LiBBcyBkaXNjdXNzZWQg YXQKKyAgICAgICAgaHR0cDovL2NvZGVyZXZpZXcuY2hyb21pdW0ub3JnLzcwOTQwMDUvLCB3ZSBw cmVmZXIgbWFraW5nIGxlYWYgY2xhc3MKKyAgICAgICAgZGVzdHJ1Y3RvcnMgdmlydHVhbCBvdmVy IG1ha2luZyB0aGUgbGVhZiBjbGFzc2VzIGZpbmFsLgorCisgICAgICAgICogc3JjL05vdGlmaWNh dGlvblByZXNlbnRlckltcGwuY3BwOgorICAgICAgICAoV2ViS2l0OjpWb2lkQ2FsbGJhY2tDbGll bnQ6On5Wb2lkQ2FsbGJhY2tDbGllbnQpOgorCiAyMDExLTA1LTI3ICBKb2NoZW4gRWlzaW5nZXIg IDxqb2NoZW5AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0gQmFydGgu CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9Ob3RpZmljYXRpb25QcmVz ZW50ZXJJbXBsLmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL05vdGlmaWNhdGlvblBy ZXNlbnRlckltcGwuY3BwCmluZGV4IDE5MzE0NjViNjE0NjU0MTM3YmExMmFlMGIyNDIwODQzNzJj N2Q4MmIuLjE3OWY3NDk2YTc3YmYzMDFhMTNkN2NiZjlhNjU3ZjdhMjM2NjUzODIgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL05vdGlmaWNhdGlvblByZXNlbnRlckltcGwu Y3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3JjL05vdGlmaWNhdGlvblByZXNlbnRl ckltcGwuY3BwCkBAIC01Niw2ICs1Niw4IEBAIHB1YmxpYzoKICAgICB7CiAgICAgfQogCisgICAg dmlydHVhbCB+Vm9pZENhbGxiYWNrQ2xpZW50KCkgeyB9CisKICAgICB2aXJ0dWFsIHZvaWQgcGVy bWlzc2lvblJlcXVlc3RDb21wbGV0ZSgpCiAgICAgewogICAgICAgICBpZiAobV9jYWxsYmFjaykK