Details

Reviewers

emilio

Group Reviewers

devtools-reviewers

Commits

rMOZILLACENTRALa48fa41db5cf: Bug 1598158 - Support offset-path:url() in style. r=devtools-reviewers,emilio

Bugzilla Bug ID

1598158

Summary

In layout, we build a default path("m 0 0") for now. We will implement
it later.

Besides, we don't support compositor animations for url(), so we don't
have to serialize it for IPC.

Note:
<url> includes url() and src(). For now we only support url().
We should revisit src() in Bug 1845390.

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Build Status

Buildable 567589
Build 665256: arc lint + arc unit

Event Timeline

boris created this revision.Jul 24 2023, 11:01 PM

Herald added a project: secure-revision. · View Herald TranscriptJul 24 2023, 11:01 PM

Harbormaster completed remote builds in B562501: Diff 746247.Jul 24 2023, 11:01 PM

boris added a child revision: D184430: Bug 1598158 - Add IsClosedLoop() for SVGGeometryElement..Jul 24 2023, 11:01 PM

phab-bot published this revision for review.Jul 24 2023, 11:02 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Herald added subscribers: zrhoffman, emilio. · View Herald TranscriptJul 24 2023, 11:02 PM

boris updated this revision to Diff 746640.Jul 25 2023, 8:06 PM

boris edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B562837: Diff 746640.Jul 25 2023, 8:06 PM

Rebased and added tests for non-svg element

Harbormaster completed remote builds in B563383: Diff 747248.Jul 26 2023, 10:39 PM

boris added reviewers: layout-reviewers, Restricted Project.Jul 28 2023, 6:27 PM

Herald added a reviewer: devtools-reviewers. · View Herald TranscriptJul 28 2023, 6:27 PM

boris added 1 blocking reviewer(s): Restricted Project.Jul 28 2023, 6:28 PM

nchevobbe accepted this revision as: devtools-reviewers.Jul 31 2023, 5:04 AM

I'm fairly confused about the deserialization stuff... Unless I'm missing something we should definitely not crash.

servo/components/style/values/generics/motion.rs
113	I'm confused, seems like `#[serde(skip_deserializing)]` should work? But at the very least we should return a proper error.
119	This will crash the parent / GPU process when deserializing right? That doesn't seem acceptable?
servo/ports/geckolib/cbindgen.toml
1045	This only has one caller, so probably I'd just remove it?

This revision now requires changes to proceed.Jul 31 2023, 5:41 PM

boris added inline comments.Jul 31 2023, 9:33 PM

servo/components/style/values/generics/motion.rs
113	It should work, but it seems it makes `IPC::ReadParam<::mozilla::StyleOffsetPath>(aReader)` returns error. Maybe our serde version is too old. I just noticed it may be a bug but a newer version fixed it: https://github.com/serde-rs/serde/releases/tag/v1.0.165.

boris added inline comments.Jul 31 2023, 9:50 PM

servo/components/style/values/generics/motion.rs
113	This happens even if the input type of `OffsetPathFunction` is not `url()`. So I suspect this is a serde bug. Anyway, I will make this return an Err() instead to work around it.

Addressed comments

Harbormaster completed remote builds in B564765: Diff 748818.Jul 31 2023, 11:55 PM

emilio accepted this revision as: emilio.Aug 7 2023, 2:00 PM

emilio added a project: testing-approved.

emilio removed reviewers: layout-reviewers, Restricted Project.

emilio added inline comments.

servo/components/style/values/generics/motion.rs
113	Can you `cargo update -p serde`, then `./mach vendor rust` instead? https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3aa65b84ebc70e40d1537582edf3df55 seems to work. Otherwise, mind filing a serde issue and referencing it here?

This revision is now accepted and ready to land.Aug 7 2023, 2:00 PM

boris added inline comments.Aug 7 2023, 6:54 PM

servo/components/style/values/generics/motion.rs
113	Updating serde doesn't work. I notices this doesn't happen when I use `ray()`. The assertion happen only when I use basic shape, `path()`, `circle()`, ...etc. I'm still trying to figure out what happen. So I would like to file a Gecko bug first to figure out what happen, so I could file a serde issue with a reduced example. Of course, I will reference the Gecko bug here.

Add follow-up bug numbers.

Harbormaster completed remote builds in B567065: Diff 751554.Aug 7 2023, 7:17 PM

boris marked an inline comment as done.Aug 7 2023, 7:17 PM

Closed by commit rMOZILLACENTRALa48fa41db5cf: Bug 1598158 - Support offset-path:url() in style. r=devtools-reviewers,emilio (authored by boris). · Explain WhyAug 7 2023, 11:41 PM

This revision was automatically updated to reflect the committed changes.

boris added a commit: rMOZILLACENTRALa48fa41db5cf: Bug 1598158 - Support offset-path:url() in style. r=devtools-reviewers,emilio.

Harbormaster completed remote builds in B567589: Diff 751639.Sep 27 2023, 4:40 PM

Bug 1598158 - Support offset-path:url() in style.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 751639

devtools/shared/css/generated/properties-db.js

dom/animation/KeyframeEffect.cpp

layout/base/MotionPathUtils.cpp

layout/style/StyleAnimationValue.h

layout/style/StyleAnimationValue.cpp

layout/style/test/mochitest.ini

layout/style/test/property_database.js

modules/libpref/init/StaticPrefList.yaml

servo/components/style/values/computed/motion.rs

servo/components/style/values/generics/motion.rs

servo/components/style/values/specified/motion.rs

servo/ports/geckolib/cbindgen.toml

servo/ports/geckolib/glue.rs

testing/web-platform/meta/css/motion/dir.ini

testing/web-platform/meta/css/motion/parsing/offset-path-parsing-valid.html.ini

Bug 1598158 - Support offset-path:url() in style.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 751639

devtools/shared/css/generated/properties-db.js

dom/animation/KeyframeEffect.cpp

layout/base/MotionPathUtils.cpp

layout/style/StyleAnimationValue.h

layout/style/StyleAnimationValue.cpp

layout/style/test/mochitest.ini

layout/style/test/property_database.js

modules/libpref/init/StaticPrefList.yaml

servo/components/style/values/computed/motion.rs

servo/components/style/values/generics/motion.rs

servo/components/style/values/specified/motion.rs

servo/ports/geckolib/cbindgen.toml

servo/ports/geckolib/glue.rs

testing/web-platform/meta/css/motion/__dir__.ini

testing/web-platform/meta/css/motion/parsing/offset-path-parsing-valid.html.ini

Bug 1598158 - Support offset-path:url() in style.
ClosedPublic
Actions

Revision Contents
Changeset List

testing/web-platform/meta/css/motion/dir.ini