Bug 159456 – Update HTML*Element class override methods in final classes

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

[Rawinder Singh]

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)

[Rawinder Singh]

Created attachment 282859 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Rawinder Singh]

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

[youenn fablet]

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.

[Rawinder Singh]

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

[youenn fablet]

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?

[Darin Adler]

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.

[Rawinder Singh]

Created attachment 283595 [details]
Patch

[Rawinder Singh]

(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

[youenn fablet]

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?

[Rawinder Singh]

Created attachment 283722 [details]
Patch

[Rawinder Singh]

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

[WebKit Commit Bot]

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

[Rawinder Singh]

Created attachment 283731 [details]
Patch

[Rawinder Singh]

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

[WebKit Commit Bot]

Comment on attachment 283731 [details]
Patch

Clearing flags on attachment: 283731

Committed r203264: <http://trac.webkit.org/changeset/203264>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.