FINALizing of the Node hierarchy.

FINALizing of the Node hierarchy.

A couple of the stable DOM/ microbenchmarks are perf positive on Linux clang:
- CloneNodes time reduced from 162.4 to 156.5 ms.
- CreateNodes time reduced from 113.9 to 104.9 ms.

(Don't get too excited; official Linux builds still use GCC, v4.6 as far as I
know, which does not support final yet. The GCC 4.6 scores are 170ms and 98ms.
Interesting that GCC + nofinal is faster on CreateNodes that clang + final,
and slower than clang + nofinal on CloneNodes).

TEST=wktry compiles
R=eseidel@chromium.org, esprehn@chromium.org

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

[Chris Evans, 2013-04-29 20:50:36]

(Adding more reviewers; not sure Elliot is around)

[eseidel, 2013-04-29 20:57:21]

All the FINAL addisions lgtm.  The rest is questionable.

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h
File Source/core/dom/Element.h (left):

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h#old...
Source/core/dom/Element.h:420: virtual void willAddAuthorShadowRoot() { }
Why is this OK to remove?

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h#new...
Source/core/dom/Element.h:576: bool containsFullScreenElement() const;
I would rather you separated changes like this out into a separate patch.  Why
are these OK to devirtualize?

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h#new...
Source/core/dom/Element.h:588: bool isSpellCheckingEnabled() const;
Same question.  It's not that I don't believe you, it's just easy to trip up
with changes like these and it's hard to tell they're correct while reading this
patch which is nominally about adding the FINAL keyword to places. :)

[esprehn, 2013-04-29 22:02:55]

LGTM. Lets move all the devirtualization and dead code removal to a second patch
though. It looks fine, but I want it as two separate steps since this patch is
gigantic.

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h
File Source/core/dom/Element.h (left):

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h#old...
Source/core/dom/Element.h:420: virtual void willAddAuthorShadowRoot() { }
On 2013/04/29 20:57:21, Eric Seidel (Google) wrote:
> Why is this OK to remove?

Nothing seems to call this now. Weird, I wonder when it was removed and why this
was left behind.

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h#new...
Source/core/dom/Element.h:576: bool containsFullScreenElement() const;
On 2013/04/29 20:57:21, Eric Seidel (Google) wrote:
> I would rather you separated changes like this out into a separate patch.  Why
> are these OK to devirtualize?

Ditto. It's clear that these don't need to be virtual now, but it seems really
unrelated to this patch.

https://codereview.chromium.org/14022022/diff/1/Source/core/html/HTMLElement.h
File Source/core/html/HTMLElement.h (right):

https://codereview.chromium.org/14022022/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.h:46: virtual String title() const FINAL;
Missing OVERRIDE

https://codereview.chromium.org/14022022/diff/1/Source/core/html/HTMLElement....
Source/core/html/HTMLElement.h:118: virtual String nodeName() const FINAL;
Missing OVERRIDE

[Chris Evans, 2013-04-30 10:17:14]

On 2013/04/29 22:02:55, esprehn wrote:
> LGTM. Lets move all the devirtualization and dead code removal to a second
patch
> though. It looks fine, but I want it as two separate steps since this patch is
> gigantic.

Got it, thanks.

For the curious, the typical workflow to discovery of needlessly virtual methods
is:
- Add FINAL to a method.
- If it compiles, also add OVERRIDE.
- If that fails, the method has no business being virtual ;-)

> 
> https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h
> File Source/core/dom/Element.h (left):
> 
>
https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h#old...
> Source/core/dom/Element.h:420: virtual void willAddAuthorShadowRoot() { }
> On 2013/04/29 20:57:21, Eric Seidel (Google) wrote:
> > Why is this OK to remove?
> 
> Nothing seems to call this now. Weird, I wonder when it was removed and why
this
> was left behind.
> 
> https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h
> File Source/core/dom/Element.h (right):
> 
>
https://codereview.chromium.org/14022022/diff/1/Source/core/dom/Element.h#new...
> Source/core/dom/Element.h:576: bool containsFullScreenElement() const;
> On 2013/04/29 20:57:21, Eric Seidel (Google) wrote:
> > I would rather you separated changes like this out into a separate patch. 
Why
> > are these OK to devirtualize?
> 
> Ditto. It's clear that these don't need to be virtual now, but it seems really
> unrelated to this patch.
> 
> https://codereview.chromium.org/14022022/diff/1/Source/core/html/HTMLElement.h
> File Source/core/html/HTMLElement.h (right):
> 
>
https://codereview.chromium.org/14022022/diff/1/Source/core/html/HTMLElement....
> Source/core/html/HTMLElement.h:46: virtual String title() const FINAL;
> Missing OVERRIDE

Done

> 
>
https://codereview.chromium.org/14022022/diff/1/Source/core/html/HTMLElement....
> Source/core/html/HTMLElement.h:118: virtual String nodeName() const FINAL;
> Missing OVERRIDE

Done

[esprehn, 2013-04-30 19:13:24]

LGTM.

[Chris Evans, 2013-04-30 19:14:14]

Committed patchset #2 manually as r149454 (presubmit successful).

[eseidel, 2013-04-30 19:43:56]

That sounds like something simple enough one could write a script to do! :)