Bug 130636 - Add *final* keyword to NavigatorContentUtils class
Summary: Add *final* keyword to NavigatorContentUtils class
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: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-22 00:43 PDT by Gyuyoung Kim
Modified: 2014-03-22 18:02 PDT (History)
1 user (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2014-03-22 00:45 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (1.50 KB, patch)
2014-03-22 08:38 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2014-03-22 00:43:24 PDT
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.
Comment 1 Gyuyoung Kim 2014-03-22 00:45:47 PDT
Created attachment 227538 [details]
Patch
Comment 2 Chris Dumez 2014-03-22 05:52:32 PDT
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.
Comment 3 Gyuyoung Kim 2014-03-22 06:23:03 PDT
(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 4 Chris Dumez 2014-03-22 06:44:35 PDT
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 5 Darin Adler 2014-03-22 07:52:24 PDT
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.
Comment 6 Gyuyoung Kim 2014-03-22 08:38:40 PDT
Created attachment 227551 [details]
Patch
Comment 7 Gyuyoung Kim 2014-03-22 08:41:25 PDT
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 8 WebKit Commit Bot 2014-03-22 18:02:17 PDT
Comment on attachment 227551 [details]
Patch

Clearing flags on attachment: 227551

Committed r166130: <http://trac.webkit.org/changeset/166130>
Comment 9 WebKit Commit Bot 2014-03-22 18:02:23 PDT
All reviewed patches have been landed.  Closing bug.