Clean up virtual function in Source/html

Clean up virtual function in Source/html

This cl was based on  http://trac.webkit.org/changeset/175328 

Clean up virtual functions in Source/html by:
1. Making isXXX() virtual functions private on XXX classes to avoid derived class use that.
2. De-virtualizing function that do not need to be virtual
3. Dropping final for virtual functions in classes already marked as final

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185469

[zhaoze.zhou, 2014-11-03 18:32:46]

zhaoze.zhou@partner.samsung.com changed reviewers:
+ rob.buis@samsung.com

[zhaoze.zhou, 2014-11-03 21:15:52]

Patchset #2 (id:20001) has been deleted

[rwlbuis, 2014-11-04 17:38:08]

On 2014/11/03 18:32:46, zhaoze.zhou wrote:
> mailto:zhaoze.zhou@partner.samsung.com changed reviewers:
> + mailto:rob.buis@samsung.com

Great but you should state the changeset this was based on.

[kbalazs, 2014-11-04 19:08:45]

b.kelemen@samsung.com changed reviewers:
+ b.kelemen@samsung.com

[kbalazs, 2014-11-04 19:08:45]

https://codereview.chromium.org/699703002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/MouseEventHitRegion.h (right):

https://codereview.chromium.org/699703002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/MouseEventHitRegion.h:15: class MouseEventHitRegion
final {
IMHO this is just noise. Using final is only valuable when the class has virtual
functions.

[zhaoze.zhou, 2014-11-04 19:14:17]

https://codereview.chromium.org/699703002/diff/40001/Source/core/html/canvas/...
File Source/core/html/canvas/MouseEventHitRegion.h (right):

https://codereview.chromium.org/699703002/diff/40001/Source/core/html/canvas/...
Source/core/html/canvas/MouseEventHitRegion.h:15: class MouseEventHitRegion
final {
On 2014/11/04 19:08:45, kbalazs wrote:
> IMHO this is just noise. Using final is only valuable when the class has
virtual
> functions.

Done.

[zhaoze.zhou, 2014-11-07 18:50:49]

zhaoze.zhou@partner.samsung.com changed reviewers:
+ adamk@chromium.org

[zhaoze.zhou, 2014-11-11 15:07:36]

@adamk hi, can you take a look? thanks very much :)

[adamk, 2014-11-11 17:52:37]

I don't think your CL description is entirely accurate. Point (1) claims this
reduces type checks, but I don't see any type check reductions, just moving
around of protected to private. As for (3), as I point out below, you'd still
need to leave "override", so I don't see much point in this transformation.

In general, I'd expect a CL description for a change like this to accurately
describe the reasoning/benefits of a change, and I don't see that here.

https://codereview.chromium.org/699703002/diff/50001/Source/core/html/HTMLMar...
File Source/core/html/HTMLMarqueeElement.h (left):

https://codereview.chromium.org/699703002/diff/50001/Source/core/html/HTMLMar...
Source/core/html/HTMLMarqueeElement.h:35: virtual void attributeWillChange(const
QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue)
final;
I don't think dropping these finals is worthwhile. If you remove them, you'd
have to add "override". But "final" already implies override, so I'd leave these
be.

[zhaoze.zhou, 2014-11-17 21:19:34]

On 2014/11/11 17:52:37, adamk wrote:
> I don't think your CL description is entirely accurate. Point (1) claims this
> reduces type checks, but I don't see any type check reductions, just moving
> around of protected to private. As for (3), as I point out below, you'd still
> need to leave "override", so I don't see much point in this transformation.
> 
> In general, I'd expect a CL description for a change like this to accurately
> describe the reasoning/benefits of a change, and I don't see that here.
> 
>
https://codereview.chromium.org/699703002/diff/50001/Source/core/html/HTMLMar...
> File Source/core/html/HTMLMarqueeElement.h (left):
> 
>
https://codereview.chromium.org/699703002/diff/50001/Source/core/html/HTMLMar...
> Source/core/html/HTMLMarqueeElement.h:35: virtual void
attributeWillChange(const
> QualifiedName&, const AtomicString& oldValue, const AtomicString& newValue)
> final;
> I don't think dropping these finals is worthwhile. If you remove them, you'd
> have to add "override". But "final" already implies override, so I'd leave
these
> be.

Hi, I have modified the cl decription and put some final back. thanks.

[adamk, 2014-11-17 21:29:13]

lgtm

[zhaoze.zhou, 2014-11-17 21:36:16]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2014-11-17 21:36:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699703002/70001

[commit-bot: I haz the power, 2014-11-17 22:43:36]

Committed patchset #4 (id:70001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185469

[zhaoze.zhou, 2014-11-18 16:34:24]

On 2014/11/17 21:29:13, adamk wrote:
> lgtm
Hi, I found the source of "type-checking", I guess this can make code design
better. Thanks.
https://bugs.webkit.org/show_bug.cgi?id=138190