Bug 62067 – [chromium] Give VoidCallbackClient a virtual destructor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

[chromium] Give VoidCallbackClient a virtual destructor

[Nico Weber]

Created attachment 95986 [details]
Patch

[James Robinson]

Comment on attachment 95986 [details]
Patch

You should add virtual to WebPermissionCallback's destructor declaration as well.

[Nico Weber]

(In reply to comment #2)
> (From update of attachment 95986 [details])
> You should add virtual to WebPermissionCallback's destructor declaration as well.

Why?

[WebKit Review Bot]

Comment on attachment 95986 [details]
Patch

Clearing flags on attachment: 95986

Committed r88106: <http://trac.webkit.org/changeset/88106>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

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.

[Nico Weber]

(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.)

[Nico Weber]

Also, I don't know if it's customary to r- patches that have already been submitted?

[Darin Fisher (:fishd, Google)]

(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.)

[Nico Weber]

> 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