Bug 237553

Summary: Clean up RenderLayer::currentTransform()
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: Layout and RenderingAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, changseok, clopez, darin, esprehn+autocc, ews-watchlist, fred.wang, glenn, kondapallykalyan, ntim, pdr, rbuis, simon.fraser, webkit-bug-importer, youennf, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://github.com/web-platform-tests/wpt/pull/47377
Bug Depends on:    
Bug Blocks: 90738, 237554, 237701, 237711    
Attachments:
Description Flags
Patch, v1
none
Patch, v2 rbuis: review+

Nikolas Zimmermann
Reported 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.
Attachments
Patch, v1 (16.27 KB, patch)
2022-03-07 13:38 PST, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Patch, v2 (16.83 KB, patch)
2022-03-09 03:03 PST, Nikolas Zimmermann 		rbuis: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1 2022-03-07 13:38:10 PST
Created attachment 454024 [details] Patch, v1
EWS Watchlist
Comment 2 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
Rob Buis
Comment 3 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.
Nikolas Zimmermann
Comment 4 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.
Nikolas Zimmermann
Comment 5 2022-03-09 03:03:31 PST
Created attachment 454214 [details] Patch, v2
Rob Buis
Comment 6 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?
Nikolas Zimmermann
Comment 7 2022-03-10 05:37:49 PST
Committed r291105 (248267@trunk): <https://commits.webkit.org/248267@trunk>
Radar WebKit Bug Importer
Comment 8 2022-03-10 05:38:16 PST
<rdar://problem/90093322>
Nikolas Zimmermann
Comment 9 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 :-)
Tim Nguyen (:ntim)
Comment 10 2024-07-30 15:52:00 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/47377
Note You need to log in before you can comment on or make changes to this bug.