Cleanup RenderLayer::currentTransform()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

RenderLayer::currentTransform() is hard to read due to the code duplication in the branches that re-compute the transform - fix that.

[Nikolas Zimmermann]

Created attachment 454024 [details]
Patch, v1

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Rob Buis]

Comment on attachment 454024 [details]
Patch, v1

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

> Source/WebCore/rendering/RenderLayer.cpp:1361
> +    std::unique_ptr<RenderStyle> animatedStyle = renderBox.animatedStyle();

Why not move this into the if block?

> LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-box-will-change-transform-layer.html:57
> +    await waitForNextFrame();

Doesn't this work? await new Promise(requestAnimationFrame);

> LayoutTests/platform/mac/TestExpectations:2439
> +webkit.org/b/237166 [ BigSur+ ] webgl/pending/conformance/textures/misc/tex-image-video-repeated.html [ Pass Timeout ]

You probably do not want this change.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #3)
> Comment on attachment 454024 [details]
> Patch, v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454024&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1361
> > +    std::unique_ptr<RenderStyle> animatedStyle = renderBox.animatedStyle();
> 
> Why not move this into the if block?
Leftover, fixed -- it should be there, to avoid querying animatedStyle() if not necessary.

> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-box-will-change-transform-layer.html:57
> > +    await waitForNextFrame();
> 
> Doesn't this work? await new Promise(requestAnimationFrame);
I kept the style from the other tests in that directory, that do it like this.

> 
> > LayoutTests/platform/mac/TestExpectations:2439
> > +webkit.org/b/237166 [ BigSur+ ] webgl/pending/conformance/textures/misc/tex-image-video-repeated.html [ Pass Timeout ]
> 
> You probably do not want this change.

Right.

[Nikolas Zimmermann]

Created attachment 454214 [details]
Patch, v2

[Rob Buis]

Comment on attachment 454214 [details]
Patch, v2

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-box.html:25
> +  <div id="switchTransformBox" class="block"></div>

The name seems not so good. I do not like the 'switch' and not only transform box is changed, right? Maybe something like transformBoxTarget?

[Nikolas Zimmermann]

Committed r291105 (248267@trunk): <https://commits.webkit.org/248267@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/90093322>

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #6)
> Comment on attachment 454214 [details]
> Patch, v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454214&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-transforms/animation/transform-box.html:25
> > +  <div id="switchTransformBox" class="block"></div>
> 
> The name seems not so good. I do not like the 'switch' and not only
> transform box is changed, right? Maybe something like transformBoxTarget?

Fixed before landing.

I'll report about the WPT upstreaming status, haven't contributed so far, and need to tests this process once now :-)