[chromium] Give VoidCallbackClient a virtual destructor
Created attachment 95986 [details] Patch
Comment on attachment 95986 [details] Patch You should add virtual to WebPermissionCallback's destructor declaration as well.
(In reply to comment #2) > (From update of attachment 95986 [details]) > You should add virtual to WebPermissionCallback's destructor declaration as well. Why?
Comment on attachment 95986 [details] Patch Clearing flags on attachment: 95986 Committed r88106: <http://trac.webkit.org/changeset/88106>
All reviewed patches have been landed. Closing bug.
Comment on attachment 95986 [details] 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.
(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.)
Also, I don't know if it's customary to r- patches that have already been submitted?
(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.)
> 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