Update HTML*Element classes so that: - Overriden methods in final classes are marked final - virtual on destructors in derived classes are removed (where the destructor is already virtual due to a base class)
Created attachment 282859 [details] Patch
Comment on attachment 282859 [details] Patch Attachment 282859 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1634636 New failing tests: fast/scrolling/ios/scroll-events-back-forward.html
Created attachment 282868 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
(In reply to comment #3) > Created attachment 282868 [details] > Archive of layout-test-results from ews122 for ios-simulator-wk2 > > The attached test failures were seen while running run-webkit-tests on the > ios-sim-ews. > Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4 I don't think the tests failure is due to this patch - I think this may be a flaky result because: 1st time the tests ran these tests gave unexpected results: fast/replaced/table-percent-height.html [ Failure Pass ] fast/scrolling/ios/scroll-event-from-scrollTo.html [ Failure ] 2nd time - tests tests gave unexpected resutls: fast/scrolling/ios/scroll-event-from-scrollTo.html [ Failure Pass ] storage/domstorage/events/basic-setattribute.html [ Failure Pass ] fast/scrolling/ios/scroll-events-back-forward.html [ Failure ] 3rd time - tests tests gave unexpected resutls: fast/scrolling/ios/scroll-events-back-forward.html [ Failure ] Is my thinking correct?
Comment on attachment 282859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=282859&action=review > Source/WebCore/ChangeLog:9 > + - Overriden methods in final classes are marked final. Sounds good to me > Source/WebCore/ChangeLog:10 > + - virtual on destructors in derived classes are removed (where the destructor is already virtual due to a base class). Was it discussed somewhere? This sounds good for final classes. Although this does not change any behaviour, I have a small doubt about removing it for not-final classes.
(In reply to comment #5) > Comment on attachment 282859 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=282859&action=review > > > Source/WebCore/ChangeLog:9 > > + - Overriden methods in final classes are marked final. > > Sounds good to me > > > Source/WebCore/ChangeLog:10 > > + - virtual on destructors in derived classes are removed (where the destructor is already virtual due to a base class). > > Was it discussed somewhere? > This sounds good for final classes. > Although this does not change any behaviour, I have a small doubt about > removing it for not-final classes. It was brought up in an email from Darin (quoted below) > Re: [webkit-dev] Should overridden methods use 'virtual' keyword in addition to 'override'? > ... > - Style guide should discourage virtual on destructors where the destructor is already virtual due to a base class. This is now more consistent with the use of virtual on other member functions, final would be on the class, override doesn’t need to be stated. Agreed?
Checking quickly WebCore, I have not found any case of a being-derived class that does not have its destructor marked virtual explicitly. Since your patch contains many improvements, maybe it could be split so that most of it can be landed as part of this bug?
I suggested that “don’t specify virtual on destructors” but I don’t think we discussed it together and decided on it. So I agree with youenn that we should separate that part of the change out and not do it until we have consensus on it.
Created attachment 283595 [details] Patch
(In reply to comment #7) > Checking quickly WebCore, I have not found any case of a being-derived class > that does not have its destructor marked virtual explicitly. > Since your patch contains many improvements, maybe it could be split so that > most of it can be landed as part of this bug? Okay, I separated the patch - this patch now marks overridden methods in final classes as final
Comment on attachment 283595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283595&action=review > Source/WebCore/ChangeLog:8 > + Update HTML*Element classes so that overriden methods in final classes are marked final. Some of the modified classes are not marked final, HTMLDivElement for instance. I think this is ok, but can you at least update the changelog to mention that?
Created attachment 283722 [details] Patch
(In reply to comment #11) > Comment on attachment 283595 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283595&action=review > > > Source/WebCore/ChangeLog:8 > > + Update HTML*Element classes so that overriden methods in final classes are marked final. > > Some of the modified classes are not marked final, HTMLDivElement for > instance. > I think this is ok, but can you at least update the changelog to mention > that? Fixed.
Comment on attachment 283722 [details] Patch Rejecting attachment 283722 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 283722, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/1683061
Created attachment 283731 [details] Patch
(In reply to comment #14) > Comment on attachment 283722 [details] > Patch > > Rejecting attachment 283722 [details] from commit-queue. > > Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', > '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', > 'validate-changelog', '--check-oops', '--non-interactive', 283722, > '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit > > ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. > > Full output: http://webkit-queues.webkit.org/results/1683061 Uploaded again to indicate: "Reviewed by Youenn Fablet."
Comment on attachment 283731 [details] Patch Clearing flags on attachment: 283731 Committed r203264: <http://trac.webkit.org/changeset/203264>
All reviewed patches have been landed. Closing bug.