Bug 62067 - [chromium] Give VoidCallbackClient a virtual destructor
Summary: [chromium] Give VoidCallbackClient a virtual destructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nico Weber
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-03 16:40 PDT by Nico Weber
Modified: 2011-06-08 17:05 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.45 KB, patch)
2011-06-03 16:41 PDT, Nico Weber
fishd: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2011-06-03 16:40:23 PDT
[chromium] Give VoidCallbackClient a virtual destructor
Comment 1 Nico Weber 2011-06-03 16:41:37 PDT
Created attachment 95986 [details]
Patch
Comment 2 James Robinson 2011-06-03 16:50:11 PDT
Comment on attachment 95986 [details]
Patch

You should add virtual to WebPermissionCallback's destructor declaration as well.
Comment 3 Nico Weber 2011-06-03 16:51:19 PDT
(In reply to comment #2)
> (From update of attachment 95986 [details])
> You should add virtual to WebPermissionCallback's destructor declaration as well.

Why?
Comment 4 WebKit Review Bot 2011-06-04 03:51:04 PDT
Comment on attachment 95986 [details]
Patch

Clearing flags on attachment: 95986

Committed r88106: <http://trac.webkit.org/changeset/88106>
Comment 5 WebKit Review Bot 2011-06-04 03:51:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Fisher (:fishd, Google) 2011-06-08 16:42:11 PDT
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.
Comment 7 Nico Weber 2011-06-08 16:48:11 PDT
(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.)
Comment 8 Nico Weber 2011-06-08 16:48:50 PDT
Also, I don't know if it's customary to r- patches that have already been submitted?
Comment 9 Darin Fisher (:fishd, Google) 2011-06-08 16:58:06 PDT
(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.)
Comment 10 Nico Weber 2011-06-08 17:05:34 PDT
> 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