Bug 130636 – Add *final* keyword to NavigatorContentUtils class

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

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

Created attachment 227538 [details]
Patch

[Chris Dumez]

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.

[Gyuyoung Kim]

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

[Chris Dumez]

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.

[Darin Adler]

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.

[Gyuyoung Kim]

Created attachment 227551 [details]
Patch

[Gyuyoung Kim]

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.

[WebKit Commit Bot]

Comment on attachment 227551 [details]
Patch

Clearing flags on attachment: 227551

Committed r166130: <http://trac.webkit.org/changeset/166130>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.