[webkit-reviews] review denied: [Bug 205694] REGRESSION (r252724): Unable to tap on play button on google video 'See the top search trends of 2019' : [Attachment 386645] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 205694: REGRESSION (r252724): Unable to tap on play button on google video
'See the top search trends of 2019'
https://bugs.webkit.org/show_bug.cgi?id=205694

Attachment 386645: Patch

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




--- Comment #5 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 386645
  --> https://bugs.webkit.org/attachment.cgi?id=386645
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1099
> -    if (m_triggersStackingContext && targetStyle.hasAutoUsedZIndex())
> -	   targetStyle.setUsedZIndex(0);
> +    Style::Adjuster::adjustAnimatedStyle(targetStyle, *m_target,
m_triggersStackingContext);
>  }

A better approach might be to simply mark a bit that this adjustment is needed
and do it from TreeResolver::createAnimatedElementUpdate where you have things
like parent/parent box style available. Alternatively those things need to be
passed through here.

> Source/WebCore/page/animation/CompositeAnimation.cpp:342
> -    if (forceStackingContext && animatedStyle) {
> -	   if (animatedStyle->hasAutoUsedZIndex())
> -	       animatedStyle->setUsedZIndex(0);
> -    }
> +    if (animatedStyle)
> +	   Style::Adjuster::adjustAnimatedStyle(*animatedStyle, element,
forceStackingContext);
>  

Why are we making code changes to the legacy animation engine? Don't we have a
new one enabled by default? Are these changes tested?

> Source/WebCore/style/StyleAdjuster.cpp:538
> +static const RenderStyle* parentBoxStyle(const Element& element)
> +{
> +    auto* renderer = element.renderer();
> +    if (!renderer)
> +	   return nullptr;
> +    
> +    if (is<RenderBox>(renderer) && is<RenderBox>(renderer->parent()))
> +	   return &renderer->parent()->style();
> +
> +    return nullptr;
> +}

We can't look into renderers for style during style resolution (which is where
animations are resolved too). All style changes are resolved in one go and
applied to the render tree as a separate step. A renderer may have stale style,
and it might not even have been created yet. If parent box style is needed, it
needs to passed in, like with other Adjuster functions.

> WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:60
> -		  BuildableName = "libANGLE.a"
> +		  BuildableName = "libANGLE-shared.dylib"

Some random garbage here.