Bug 159456 - Update HTML*Element class override methods in final classes
Summary: Update HTML*Element class override methods in final classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rawinder Singh
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-06 01:00 PDT by Rawinder Singh
Modified: 2016-07-14 19:18 PDT (History)
5 users (show)

See Also:


Attachments
Patch (85.51 KB, patch)
2016-07-06 01:25 PDT, Rawinder Singh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (600.67 KB, application/zip)
2016-07-06 02:37 PDT, Build Bot
no flags Details
Patch (75.72 KB, patch)
2016-07-13 20:18 PDT, Rawinder Singh
no flags Details | Formatted Diff | Diff
Patch (75.83 KB, patch)
2016-07-14 18:07 PDT, Rawinder Singh
no flags Details | Formatted Diff | Diff
Patch (75.83 KB, patch)
2016-07-14 18:51 PDT, Rawinder Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rawinder Singh 2016-07-06 01:00:15 PDT
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)
Comment 1 Rawinder Singh 2016-07-06 01:25:21 PDT
Created attachment 282859 [details]
Patch
Comment 2 Build Bot 2016-07-06 02:37:50 PDT
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
Comment 3 Build Bot 2016-07-06 02:37:52 PDT
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
Comment 4 Rawinder Singh 2016-07-06 17:30:21 PDT
(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 5 youenn fablet 2016-07-06 23:19:59 PDT
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.
Comment 6 Rawinder Singh 2016-07-06 23:58:52 PDT
(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?
Comment 7 youenn fablet 2016-07-08 06:30:47 PDT
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?
Comment 8 Darin Adler 2016-07-13 09:23:05 PDT
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.
Comment 9 Rawinder Singh 2016-07-13 20:18:28 PDT
Created attachment 283595 [details]
Patch
Comment 10 Rawinder Singh 2016-07-13 20:22:28 PDT
(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 11 youenn fablet 2016-07-14 05:46:29 PDT
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?
Comment 12 Rawinder Singh 2016-07-14 18:07:13 PDT
Created attachment 283722 [details]
Patch
Comment 13 Rawinder Singh 2016-07-14 18:09:25 PDT
(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 14 WebKit Commit Bot 2016-07-14 18:40:24 PDT
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
Comment 15 Rawinder Singh 2016-07-14 18:51:39 PDT
Created attachment 283731 [details]
Patch
Comment 16 Rawinder Singh 2016-07-14 18:55:30 PDT
(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 17 WebKit Commit Bot 2016-07-14 19:18:13 PDT
Comment on attachment 283731 [details]
Patch

Clearing flags on attachment: 283731

Committed r203264: <http://trac.webkit.org/changeset/203264>
Comment 18 WebKit Commit Bot 2016-07-14 19:18:17 PDT
All reviewed patches have been landed.  Closing bug.