Though NavigatorContentUtils isn't declared final class as well as it doesn't have any virtual function, the destructor has been declared as virtual. We don't need to declare the destructor as virtual.
Created attachment 227538 [details] Patch
Comment on attachment 227538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227538&action=review > Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.h:46 > + ~NavigatorContentUtils(); The destructor *is* virtual as RefCountedSupplement has a virtual destructor. I don't think it is a good idea to remove virtual here as it makes it less obvious the destructor really is virtual.
(In reply to comment #2) > (From update of attachment 227538 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227538&action=review > > > Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.h:46 > > + ~NavigatorContentUtils(); > > The destructor *is* virtual as RefCountedSupplement has a virtual destructor. I don't think it is a good idea to remove virtual here as it makes it less obvious the destructor really is virtual. I see. Do you think *final* keyword is not good as well ? I think NavigatorContentUtils won't become a parent class.
Comment on attachment 227538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227538&action=review >>> Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.h:46 >>> + ~NavigatorContentUtils(); >> >> The destructor *is* virtual as RefCountedSupplement has a virtual destructor. I don't think it is a good idea to remove virtual here as it makes it less obvious the destructor really is virtual. > > I see. Do you think *final* keyword is not good as well ? I think NavigatorContentUtils won't become a parent class. No, the final seems fine to me but I am not aware of the WebKit policy when it comes to final keyword.
Comment on attachment 227538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=227538&action=review >>>> Source/WebCore/Modules/navigatorcontentutils/NavigatorContentUtils.h:46 >>>> + ~NavigatorContentUtils(); >>> >>> The destructor *is* virtual as RefCountedSupplement has a virtual destructor. I don't think it is a good idea to remove virtual here as it makes it less obvious the destructor really is virtual. >> >> I see. Do you think *final* keyword is not good as well ? I think NavigatorContentUtils won't become a parent class. > > No, the final seems fine to me but I am not aware of the WebKit policy when it comes to final keyword. Yes, it’s fine to add final here and it’s wrong to remove virtual. In fact, I would approve patches adding virtual to destructors in cases like this. I don’t think there’s any benefit to adding final here, but it’s correct and not harmful.
Created attachment 227551 [details] Patch
I missed to virtual dtor in RefCountedSupplement. Sorry about it. Though it it not big benefit to add *final*, I also think it is correct. So, I update the patch again.
Comment on attachment 227551 [details] Patch Clearing flags on attachment: 227551 Committed r166130: <http://trac.webkit.org/changeset/166130>
All reviewed patches have been landed. Closing bug.