[webkit-reviews] review granted: [Bug 216931] Reduce the reliance on PseudoElement in the animation code : [Attachment 409605] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Antoine Quint
<graouts at apple.com>'s request for review:
Bug 216931: Reduce the reliance on PseudoElement in the animation code
https://bugs.webkit.org/show_bug.cgi?id=216931

Attachment 409605: Patch

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




--- Comment #6 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 409605
  --> https://bugs.webkit.org/attachment.cgi?id=409605
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-el
ement-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.