Details
- Reviewers
emilio smaug - Group Reviewers
webidl Restricted Project layout-reviewers - Commits
- rFIREFOXAUTOLAND1cfa63de1db8: Bug 2020327 - Part 1: @view-transition rule scaffolding. r=emilio,webidl…
- Bugzilla Bug ID
- 2020327
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
- Build Status
Buildable 935587 Build 1036738: arc lint + arc unit
Event Timeline
| layout/inspector/InspectorUtils.cpp | ||
|---|---|---|
| 555 | This rule doesn't make sense in the panel at least but please file? | |
| layout/style/CSSViewTransitionRule.cpp | ||
| 41 | You can just pass an nsTArray<nsCString> to rust, rather than this. Let's make this a single Servo_ViewTransitionRule_GetTypes` call | |
| servo/components/style/stylesheets/stylesheet.rs | ||
| 379 | I think treating it the same as counter styles is probably fine. There is some test coverage for this but it's not standard no... See bug 1780361 for some context. | |
| servo/components/style/stylesheets/view_transition_rule.rs | ||
| 28 | These should probably be optional, right? You need to distinguish "unset" from set for cascading. Consider: @view-transition {
navigation: auto;
}
@view-transition {
types: ...;
}Right? | |
| 55 | You can get this auto-written by using: #[derive(ToCss)] pub struct Types(#[css(iterable, if_empty="none")] ThinVec<CustomIdent>); | |
| 80 | This seems wrong, this serializes it back even if it wasn't specified. Please add a test? Ironically, I was working into making it a bit easier / less error-prone in bug 2021942. If you reuse that set-up then it autogenerates the Descriptors struct and the parsing and serialization just fine. | |
| servo/ports/geckolib/glue.rs | ||
| 9713 | Should they really be CSS-escaped tho? That seems wrong if we're exposing them as an array to JS. Think of types: foo\ bar;. This would return "foo\\ bar" to JS, even though I think it should return "foo bar", right? | |
Code analysis found 3 defects in diff 1219910:
- 3 defects found by rustfmt (Mozlint)
You can run this analysis locally with:
- ./mach lint --warnings --outgoing
For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with hg import or git apply -p0).
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1219910.
Bug 2021942 simplified things indeed. I hope I copied the right parts of font-face etc.
| layout/style/CSSViewTransitionRule.cpp | ||
|---|---|---|
| 41 | I'm unsure how I would best convert Atom to an u8 string. Also, when we later pass the types to ViewTransition as TypeList, we need atoms. Would nsTArray<*mut nsAtom> and convering to RefPtr<nsAtom> on the Gecko side make sense? | |
| servo/components/style/stylesheets/view_transition_rule.rs | ||
| 80 | Used Descriptors. There is a test in testing/web-platform/tests/css/css-view-transitions/navigation/at-rule-cssom.html. | |
| servo/ports/geckolib/glue.rs | ||
| 9713 | Chrome CSSOM returns "foo\\ bar". In the current state, the patch stack leads to "foo bar".
I don't know what would be correct. I added tests in part 2 to testing/web-platform/tests/css/css-view-transitions/navigation/with-types/at-rule-with-types-parsing.html. Is that fine, or does that require a test change proposal? | |
LGTM, thanks!
| dom/webidl/CSSViewTransitionRule.webidl | ||
|---|---|---|
| 15 | Add a todo pointing to bug 1236777? | |
| servo/ports/geckolib/glue.rs | ||
|---|---|---|
| 9713 | For types: abc\\ xyz;, Safari has .types = ["foo bar"] which would make sense and align with us. So I'll keep that as expectation in the test in part 2, causing Chrome to fail it. | |
Code analysis found 7 defects in diff 1227578:
- 7 build errors found by clang-tidy
You can run this analysis locally with:
- ./mach static-analysis check --outgoing (C/C++)
If you see a problem in this automated review, please report it here.
You can view these defects in the Diff Detail section of Phabricator diff 1227578.
| servo/ports/geckolib/glue.rs | ||
|---|---|---|
| 9709 | ( Oh, after rebase it has to be .value. as ViewTransitionClass got wrapped with a TreeScoped<>. ) | |