130636 2014-03-22 00:43:24 -0700 Add *final* keyword to NavigatorContentUtils class 2014-03-22 18:02:23 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 gyuyoung.kim gyuyoung.kim commit-queue oldest_to_newest 993455 0 gyuyoung.kim 2014-03-22 00:43:24 -0700 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. 993456 1 227538 gyuyoung.kim 2014-03-22 00:45:47 -0700 Created attachment 227538 Patch 993475 2 227538 cdumez 2014-03-22 05:52:32 -0700 Comment on attachment 227538 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. 993478 3 gyuyoung.kim 2014-03-22 06:23:03 -0700 (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. 993482 4 227538 cdumez 2014-03-22 06:44:35 -0700 Comment on attachment 227538 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. 993489 5 227538 darin 2014-03-22 07:52:24 -0700 Comment on attachment 227538 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. 993501 6 227551 gyuyoung.kim 2014-03-22 08:38:40 -0700 Created attachment 227551 Patch 993502 7 gyuyoung.kim 2014-03-22 08:41:25 -0700 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. 993602 8 227551 commit-queue 2014-03-22 18:02:17 -0700 Comment on attachment 227551 Patch Clearing flags on attachment: 227551 Committed r166130: <http://trac.webkit.org/changeset/166130> 993603 9 commit-queue 2014-03-22 18:02:23 -0700 All reviewed patches have been landed. Closing bug. 227538 2014-03-22 00:45:47 -0700 2014-03-22 08:38:32 -0700 Patch bug-130636-20140322164524.patch text/plain 1827 gyuyoung.kim U3VidmVyc2lvbiBSZXZpc2lvbjogMTY1OTQ2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYjM1ZTIzYmY1YzczYWUy MTZiMjgyZGQ1NDRlNzc5N2IwZDYyMTU0MC4uZTg3MWMwMTJhNGM0YjgwN2YwNDQyMzdkOWQ0MWFi MjE3M2FkY2E4NSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE0LTAzLTIyICBHeXV5 b3VuZyBLaW0gIDxneXV5b3VuZy5raW1Ac2Ftc3VuZy5jb20+CisKKyAgICAgICAgUmVtb3ZlICp2 aXJ0dWFsKiBrZXl3b3JkIGluIGRlc3RydWN0b3Igb2YgTmF2aWdhdG9yQ29udGVudFV0aWxzCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMzA2MzYKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaG91Z2ggTmF2 aWdhdG9yQ29udGVudFV0aWxzIGlzbid0IGRlY2xhcmVkIGZpbmFsIGNsYXNzIGFzIHdlbGwgYXMg aXQgZG9lc24ndCBoYXZlIGFueSB2aXJ0dWFsIGZ1bmN0aW9uLAorICAgICAgICB0aGUgZGVzdHJ1 Y3RvciBoYXMgYmVlbiBkZWNsYXJlZCBhcyB2aXJ0dWFsLiBUaGlzIHBhdGNoIGRlY2xhcmVzIHRo ZSBOYXZpZ2F0b3JDb250ZW50VXRpbHMgY2xhc3MgYXMgZmluYWwKKyAgICAgICAgYXMgd2VsbCBh cyByZW1vdmluZyB2aXJ0dWFsIGluIHRoZSBkZXN0cnVjdG9yLgorCisgICAgICAgIE5vIG5ldyB0 ZXN0cywgbm8gYmVoYXZpb3IgY2hhbmdlcy4KKworICAgICAgICAqIE1vZHVsZXMvbmF2aWdhdG9y Y29udGVudHV0aWxzL05hdmlnYXRvckNvbnRlbnRVdGlscy5oOgorCiAyMDE0LTAzLTE5ICBQcmF0 aWsgU29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4KIAogICAgICAgICBbaU9TXSBHZXQgY29k ZSB0byBjb21waWxlIG9uIG9sZGVyIGlPUyB2ZXJzaW9ucwpkaWZmIC0tZ2l0IGEvU291cmNlL1dl YkNvcmUvTW9kdWxlcy9uYXZpZ2F0b3Jjb250ZW50dXRpbHMvTmF2aWdhdG9yQ29udGVudFV0aWxz LmggYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL25hdmlnYXRvcmNvbnRlbnR1dGlscy9OYXZpZ2F0 b3JDb250ZW50VXRpbHMuaAppbmRleCAwZTkxMWQ4OTc5ZWE3OTNiNzc3MzcxYmY4NzdkZDI1Yjkz NTc5YmI5Li4xMDdkYTM2ZGEyNDE4YzM1MGI0ODBlZDk1ZTM3NjUxYTFmNTEyZTM5IDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL25hdmlnYXRvcmNvbnRlbnR1dGlscy9OYXZpZ2F0 b3JDb250ZW50VXRpbHMuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL25hdmlnYXRvcmNv bnRlbnR1dGlscy9OYXZpZ2F0b3JDb250ZW50VXRpbHMuaApAQCAtNDEsOSArNDEsOSBAQCBjbGFz cyBOYXZpZ2F0b3I7CiAKIHR5cGVkZWYgaW50IEV4Y2VwdGlvbkNvZGU7CiAKLWNsYXNzIE5hdmln YXRvckNvbnRlbnRVdGlscyA6IHB1YmxpYyBSZWZDb3VudGVkU3VwcGxlbWVudDxQYWdlLCBOYXZp Z2F0b3JDb250ZW50VXRpbHM+IHsKK2NsYXNzIE5hdmlnYXRvckNvbnRlbnRVdGlscyBmaW5hbCA6 IHB1YmxpYyBSZWZDb3VudGVkU3VwcGxlbWVudDxQYWdlLCBOYXZpZ2F0b3JDb250ZW50VXRpbHM+ IHsKIHB1YmxpYzoKLSAgICB2aXJ0dWFsIH5OYXZpZ2F0b3JDb250ZW50VXRpbHMoKTsKKyAgICB+ TmF2aWdhdG9yQ29udGVudFV0aWxzKCk7CiAKICAgICBzdGF0aWMgY29uc3QgY2hhciogc3VwcGxl bWVudE5hbWUoKTsKICAgICBzdGF0aWMgTmF2aWdhdG9yQ29udGVudFV0aWxzKiBmcm9tKFBhZ2Uq KTsK 227551 2014-03-22 08:38:40 -0700 2014-03-22 18:02:16 -0700 Patch bug-130636-20140323003817.patch text/plain 1539 gyuyoung.kim U3VidmVyc2lvbiBSZXZpc2lvbjogMTY2MTE0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNmYxNDU0OGViNjI3NjRl NDdjMmI5M2MyZjk1YWVkZTUyZWYwZDdhNC4uYTcyM2JkMjhlNmU2YzI0NTQyNGZkNTY5MzNjNWYw NzE0MGNmMmMxNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE0LTAzLTIyICBHeXV5 b3VuZyBLaW0gIDxneXV5b3VuZy5raW1Ac2Ftc3VuZy5jb20+CisKKyAgICAgICAgQWRkICpmaW5h bCoga2V5d29yZCB0byBOYXZpZ2F0b3JDb250ZW50VXRpbHMgY2xhc3MKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEzMDYzNgorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5hdmlnYXRvckNvbnRlbnRVdGlscyB3 b24ndCBiZSB1c2VkIGFzIGEgYmFzZSBjbGFzcy4gU28sIGFkZGVkIGZpbmFsIGtleXdvcmQuCisK KyAgICAgICAgTm8gbmV3IHRlc3RzLCBubyBiZWhhdmlvciBjaGFuZ2VzLgorCisgICAgICAgICog TW9kdWxlcy9uYXZpZ2F0b3Jjb250ZW50dXRpbHMvTmF2aWdhdG9yQ29udGVudFV0aWxzLmg6CisK IDIwMTQtMDMtMjIgIE1hcnRpbiBIb2RvdmFuICA8bWhvZG92YW5AaW5mLnUtc3plZ2VkLmh1Pgog CiAgICAgICAgIEFTU0VSVElPTiBGQUlMRUQ6IHN0ZDo6aXNmaW5pdGUobnVtKSBpbiBXZWJDb3Jl OjpDU1NQcmltaXRpdmVWYWx1ZTo6Q1NTUHJpbWl0aXZlVmFsdWUKZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJDb3JlL01vZHVsZXMvbmF2aWdhdG9yY29udGVudHV0aWxzL05hdmlnYXRvckNvbnRlbnRV dGlscy5oIGIvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9uYXZpZ2F0b3Jjb250ZW50dXRpbHMvTmF2 aWdhdG9yQ29udGVudFV0aWxzLmgKaW5kZXggMGU5MTFkODk3OWVhNzkzYjc3NzM3MWJmODc3ZGQy NWI5MzU3OWJiOS4uYmIxZDkxZDIzMTZmMjIzOWVmNGFlZmY1OTY3NGYzZjMwNGU2NTQ5YiAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9uYXZpZ2F0b3Jjb250ZW50dXRpbHMvTmF2 aWdhdG9yQ29udGVudFV0aWxzLmgKKysrIGIvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9uYXZpZ2F0 b3Jjb250ZW50dXRpbHMvTmF2aWdhdG9yQ29udGVudFV0aWxzLmgKQEAgLTQxLDcgKzQxLDcgQEAg Y2xhc3MgTmF2aWdhdG9yOwogCiB0eXBlZGVmIGludCBFeGNlcHRpb25Db2RlOwogCi1jbGFzcyBO YXZpZ2F0b3JDb250ZW50VXRpbHMgOiBwdWJsaWMgUmVmQ291bnRlZFN1cHBsZW1lbnQ8UGFnZSwg TmF2aWdhdG9yQ29udGVudFV0aWxzPiB7CitjbGFzcyBOYXZpZ2F0b3JDb250ZW50VXRpbHMgZmlu YWwgOiBwdWJsaWMgUmVmQ291bnRlZFN1cHBsZW1lbnQ8UGFnZSwgTmF2aWdhdG9yQ29udGVudFV0 aWxzPiB7CiBwdWJsaWM6CiAgICAgdmlydHVhbCB+TmF2aWdhdG9yQ29udGVudFV0aWxzKCk7CiAK