SVGTests should not leave detached elements in the tree

SVGTests should not leave detached elements in the tree

Previously SVGTests would detach() elements when they were invalid. This was not
correct though since then calling getComputedStyle assumed that the element
was not in the document and would return an empty style. Worse, if anything
triggered a style recalc it would reattach this element and the style would not
be empty anymore.

As part of the effort to make the concept of "attached" implicit and stop using
confusingAndOftenMisusedAttached this patch switches to the correct model
of never leaving detached nodes in the tree and also fixes all the tests that
were asserting that the style was empty (which should never actually happen
if the element is inside the tree).

This also exposes that the SVGTests feature doesn't actually work for the
SVGSVGElement. As soon as a style recalc happened the element would
reappear. This patch makes the test for it correctly print FAIL exposing that
bug.

This is blocking https://codereview.chromium.org/26270004/

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

[pdr., 2013-10-25 06:13:11]

SVGTests is a dumb and useless feature I wish would go away.

LGTM with nits.

https://codereview.chromium.org/43223002/diff/30001/Source/core/svg/SVGElemen...
File Source/core/svg/SVGElementRareData.h (right):

https://codereview.chromium.org/43223002/diff/30001/Source/core/svg/SVGElemen...
Source/core/svg/SVGElementRareData.h:78: bool isValid() const { return
m_isValid; }
I'm worried this name will confuse us in the future because it's pretty generic
and overloaded.

Could we go with something more descriptive here such as isValidForSVGTests and
isSVGTestsValidityCached?

https://codereview.chromium.org/43223002/diff/30001/Source/core/svg/SVGGraphi...
File Source/core/svg/SVGGraphicsElement.cpp (right):

https://codereview.chromium.org/43223002/diff/30001/Source/core/svg/SVGGraphi...
Source/core/svg/SVGGraphicsElement.cpp:124: bool
SVGGraphicsElement::cachedValidity() const
Ditto for this name.

https://codereview.chromium.org/43223002/diff/30001/Source/core/svg/SVGGraphi...
Source/core/svg/SVGGraphicsElement.cpp:143: bool validity =
static_cast<SVGElement*>(this)->isValid();
Please use toSVGElement here instead of the static cast.

[esprehn, 2013-10-25 07:33:57]

We could simplify this by just always reattaching whenever one of these
attributes change. That would simplify this silly feature and I bet it's very
rare that anyone changes the values anyway. I'll post a patch like that, I think
it's fine to treat this like display CSS changes.

[esprehn, 2013-10-25 07:53:17]

Okay look again, this is much simpler. It does mean changing the attribute from
one valid value to another will still reattach, but that seems fine to me. I
doubt this is a perf critical feature.

[pdr., 2013-10-25 19:37:50]

On 2013/10/25 07:53:17, esprehn wrote:
> Okay look again, this is much simpler. It does mean changing the attribute
from
> one valid value to another will still reattach, but that seems fine to me. I
> doubt this is a perf critical feature.

LGTM

[commit-bot: I haz the power, 2013-10-25 20:02:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/esprehn@chromium.org/43223002/110001

[commit-bot: I haz the power, 2013-10-25 21:22:10]

Change committed as 160625