Reduce the reliance on PseudoElement in the animation code

[Antoine Quint]

Reduce the reliance on PseudoElement in the animation code

[Antoine Quint]

Created attachment 409588 [details]
Patch

[Antoine Quint]

I have a few open questions about this patch:

1. Should we use Optional<const Styleable> or even Optional<Styleable> instead of const Optional<const Styleable>?

2. Should the element member of Styleable be Ref<Element> instead of Element&?

3. Should we expose methods to access ElementAnimationRareData on Styleable directly? This would allow writing styleable.ensureAnimations() instead of styleable.element.ensureAnimations(styleable.pseudoId).

And some notes about places where I couldn't yet find a way out of using PseudoElement:

1. In KeyframeEffect::didChangeTargetStyleable(), in order for Element::invalidateStyleInternal() to be called on the new target, we have to ensure we create a PseudoElement in case the target is a pseudo-element.

2. In Style::TreeResolver::createAnimatedElementUpdate() we also require the use of PseudoElement to call Element::renderOrDisplayContentsStyle() on it. We would need a way to obtain the same RenderStyle object based on an Element/PseudoId combination.

3. In Style::TreeResolver::resolvePseudoStyle() we also require the use of PseudoElement.

4. KeyframeEffect::renderer() is another place where we rely on PseudoElement still in order to get the target RenderElement for the Element/PseudoId pair.

[Antoine Quint]

Created attachment 409592 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/69511682>

[Antoine Quint]

Created attachment 409605 [details]
Patch

[Antti Koivisto]

Comment on attachment 409605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409605&action=review

> Source/WebCore/animation/WebAnimationUtilities.cpp:65
> +        if (pseudoId == PseudoId::None)
> +            return NotPseudo;
> +        if (pseudoId == PseudoId::Marker)
> +            return Marker;
> +        if (pseudoId == PseudoId::Before)
> +            return Before;
> +        if (pseudoId == PseudoId::After)
> +            return After;

switch?

> Source/WebCore/animation/WebAnimationUtilities.cpp:73
> +    auto pseudoIdAsString = [](PseudoId pseudoId) -> String {
> +        TextStream stream;
> +        stream << pseudoId;
> +        return stream.release();
> +    };

I think TextStreams are mostly just for debugging. If there isn't a general function for this you should write it.

However since PseudoId set is fixed you could also just provide sorting order for all of them and avoid the strings for now.

> Source/WebCore/dom/ElementRareData.h:178
> -    std::unique_ptr<ElementAnimationRareData> m_animationRareData;
> +    HashMap<PseudoId, std::unique_ptr<ElementAnimationRareData>, WTF::IntHash<PseudoId>, WTF::StrongEnumHashTraits<PseudoId>> m_animationRareData;

I think this would almost always have zero-to-one and at most a couple of members. HashMap seem bit heavyweight for that. Maybe just use a Vector?

> Source/WebCore/rendering/RenderElement.cpp:2313
> +Optional<Styleable> RenderElement::styleable() const
> +{
> +    if (auto* element = this->element())
> +        return Styleable(*element, element->pseudoId());
> +    return WTF::nullopt;
> +}

Maybe put this to Styleable instead as something like

static Styleable fromRenderer(const RenderElement&)

RenderElement has enough stuff already.

This doesn't seem correct for before/after. 'element' will point to the PseudoElement instead of the host Element which doesn't match how the other code expects Styleable to work.

> Source/WebCore/style/Styleable.h:34
> +struct Styleable {

We might consider putting this to Style namespace but it is ok for now as-is.

> Source/WebCore/style/Styleable.h:42
> +    Styleable(Element& element, PseudoId pseudoId)
> +        : element(element)
> +        , pseudoId(pseudoId)
> +    {
> +    }

Asserting that element is not a PseudoElement would be good. Maybe it can also assert that if pseudo is before/after the PseudoElement exists on the element?

> LayoutTests/imported/w3c/web-platform-tests/css/css-transitions/non-rendered-element-002-expected.txt:2
> -Harness Error (TIMEOUT), message = null
> -
> -TIMEOUT Transitions on ::before/::after pseudo-elements are canceled when the content property is cleared Test timed out
> +PASS Transitions on ::before/::after pseudo-elements are canceled when the content property is cleared 

Would be good to understand this progression.

[Antti Koivisto]

(In reply to Antoine Quint from comment #2)
> I have a few open questions about this patch:
> 
> 1. Should we use Optional<const Styleable> or even Optional<Styleable>
> instead of const Optional<const Styleable>?

Current use seems fine. 

> 2. Should the element member of Styleable be Ref<Element> instead of
> Element&?

Might be a good idea. We'll need to be careful with reference cycles if Stylable is used in containers.
 
> 3. Should we expose methods to access ElementAnimationRareData on Styleable
> directly? This would allow writing styleable.ensureAnimations() instead of
> styleable.element.ensureAnimations(styleable.pseudoId).

Probably better to keep Stylable simple.

[Antoine Quint]

Created attachment 409683 [details]
Patch

[Antoine Quint]

Created attachment 409687 [details]
Patch

[Antoine Quint]

Committed r267571: <https://trac.webkit.org/changeset/267571>

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 409687 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Antoine Quint]

This caused bug 220550.