Bug 237553 - Cleanup RenderLayer::currentTransform()
Summary: Cleanup RenderLayer::currentTransform()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar
Depends on:
Blocks: 90738 237554 237701 237711
  Show dependency treegraph
 
Reported: 2022-03-07 13:30 PST by Nikolas Zimmermann
Modified: 2022-03-10 05:58 PST (History)
15 users (show)

See Also:


Attachments
Patch, v1 (16.27 KB, patch)
2022-03-07 13:38 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v2 (16.83 KB, patch)
2022-03-09 03:03 PST, Nikolas Zimmermann
rbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2022-03-07 13:30:03 PST
RenderLayer::currentTransform() is hard to read due to the code duplication in the branches that re-compute the transform - fix that.
Comment 1 Nikolas Zimmermann 2022-03-07 13:38:10 PST
Created attachment 454024 [details]
Patch, v1
Comment 2 EWS Watchlist 2022-03-07 13:40:17 PST
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
Comment 3 Rob Buis 2022-03-09 02:08:31 PST
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.
Comment 4 Nikolas Zimmermann 2022-03-09 03:02:34 PST
(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.
Comment 5 Nikolas Zimmermann 2022-03-09 03:03:31 PST
Created attachment 454214 [details]
Patch, v2
Comment 6 Rob Buis 2022-03-10 01:43:24 PST
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?
Comment 7 Nikolas Zimmermann 2022-03-10 05:37:49 PST
Committed r291105 (248267@trunk): <https://commits.webkit.org/248267@trunk>
Comment 8 Radar WebKit Bug Importer 2022-03-10 05:38:16 PST
<rdar://problem/90093322>
Comment 9 Nikolas Zimmermann 2022-03-10 05:38:58 PST
(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 :-)