Implement mixin parameters. [chromium/src : main]
0 views
Skip to first unread message
Steinar H Gunderson (Gerrit)
Sep 17, 2025, 8:25:57 AM (13 days ago) Sep 17
to chrom...@appspot.gserviceaccount.com, Anders Hartvoll Ruud, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, blink-rev...@chromium.org, blink-revie...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org
Attention needed from Anders Hartvoll Ruud
Steinar H Gunderson added 3 comments![]( "Open in Gerrit")
File third_party/blink/renderer/core/css/parser/css_parser_impl.cc
Line 2494, Patchset 14:
arguments_ok = stream.AtEnd();Anders Hartvoll Ruud . unresolved
Steinar H GundersonWe don't need `arguments_ok`?
```suggestion
if (!steam.AtEnd()) {
ConsumeErroneousAtRule(stream, CSSAtRuleID::kCSSAtRuleApplyMixin);
return nullptr;
}
```
No, that won't consume the rest of the at rule, since we're still in the block guard?
File third_party/blink/renderer/core/css/resolver/style_cascade.cc
Line 2072, Patchset 14:
// We need to resolve the argument in the context of this function,
// so that we can do type coercion on the resolved value before the call.
// In particular, we want any var() within the argument to be resolved
// in our context; e.g., --foo(var(--a)) should be our a, not foo's a
// (if that even exists).
//
// Note that if this expression comes from directly a function call,
// as in the example above (and if the return and argument types are the
// same), we will effectively do type parsing of exactly the same data
// twice. This is wasteful, and it's possible that we should do something
// about it if it proves to be a common case.Anders Hartvoll Ruud . resolved
Steinar H GundersonThis comment does not make sense here.
Done
File third_party/blink/web_tests/external/wpt/css/css-mixins/parameters.html
Line 210, Patchset 14:
Anders Hartvoll Ruud . unresolved
Steinar H GundersonWe should also test:
- That env(--custom) inside if() works.
- That env(--custom) inside a custom function does *not* resolve against the bindings of the applied mixin that made the function call. (I.e. your comment at line 1807 in style_cascade.cc.)
- That env(--custom) works in attr().
Added the two former, I thought I already had a test for the latter?
Related details
Attention is currently required from:
- Anders Hartvoll Ruud
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Sep 18, 2025, 7:03:30 AM (12 days ago) Sep 18
to Steinar H Gunderson, chrom...@appspot.gserviceaccount.com, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, blink-rev...@chromium.org, blink-revie...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org
Attention needed from Steinar H Gunderson
Anders Hartvoll Ruud voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
2 comments
File third_party/blink/renderer/core/css/parser/css_parser_impl.cc
Line 2494, Patchset 14:
arguments_ok = stream.AtEnd();Anders Hartvoll Ruud . resolved
Steinar H GundersonWe don't need `arguments_ok`?
```suggestion
if (!steam.AtEnd()) {
ConsumeErroneousAtRule(stream, CSSAtRuleID::kCSSAtRuleApplyMixin);
return nullptr;
}
```
No, that won't consume the rest of the at rule, since we're still in the block guard?
Anders Hartvoll Ruud
My bad.
File third_party/blink/web_tests/external/wpt/css/css-mixins/parameters.html
Steinar H GundersonWe should also test:
- That env(--custom) inside if() works.
- That env(--custom) inside a custom function does *not* resolve against the bindings of the applied mixin that made the function call. (I.e. your comment at line 1807 in style_cascade.cc.)
- That env(--custom) works in attr().
Added the two former, I thought I already had a test for the latter?
Attention is currently required from:
- Steinar H Gunderson
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Sep 18, 2025, 7:05:45 AM (12 days ago) Sep 18
to Anders Hartvoll Ruud, chrom...@appspot.gserviceaccount.com, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, blink-rev...@chromium.org, blink-revie...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org
Steinar H Gunderson voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Sep 18, 2025, 8:06:37 AM (12 days ago) Sep 18
to Steinar H Gunderson, Anders Hartvoll Ruud, chrom...@appspot.gserviceaccount.com, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, blink-rev...@chromium.org, blink-revie...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
Implement mixin parameters.
Applying a mixin now instantiates a “custom @env binding”, and all
style rules that we create point back to that binding. (Bindings
can point back to parent bindings, if we are applying mixins within
mixins.) Like tree scopes, we send this pointer through the cascade
so that we know what values env() should look up into when we apply
the winning value. In other words, we substitute custom env() at
computed-value time, like all other similar functions.
This costs us a bit of extra RAM (~50 kB in a typical page),
but we haven't seen any regressions on the style perftest.
We generally follow the spec whenever it is clear, but there are some
unknowns involved; in particular, the spec isn't clear on what happens
if there are too few parameters and no defaults. We don't attempt
to support mixins (and thus also custom env resolving) together with
@keyframe, @function, @page and a number of other at-rules that would
cause edge cases.
This should also build a reasonable foundation for implementing @env
later, although the spec there is also a bit in flux.
We also don't have any devtools support.
Change-Id: I2188700352cb706184d3e0d7f85ab358d24dd936
Commit-Queue: Steinar H Gunderson <se...@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <and...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1517189}
Files:
- M third_party/blink/renderer/core/animation/css_interpolation_environment.cc
- M third_party/blink/renderer/core/css/container_query_test.cc
- M third_party/blink/renderer/core/css/css_scope_rule.cc
- M third_party/blink/renderer/core/css/css_style_rule.cc
- M third_party/blink/renderer/core/css/css_syntax_definition.h
- M third_party/blink/renderer/core/css/element_rule_collector.cc
- M third_party/blink/renderer/core/css/page_rule_collector.cc
- M third_party/blink/renderer/core/css/parser/css_parser_impl.cc
- M third_party/blink/renderer/core/css/resolver/cascade_expansion_test.cc
- M third_party/blink/renderer/core/css/resolver/match_result.cc
- M third_party/blink/renderer/core/css/resolver/match_result.h
- M third_party/blink/renderer/core/css/resolver/match_result_test.cc
- M third_party/blink/renderer/core/css/resolver/matched_properties_cache.cc
- M third_party/blink/renderer/core/css/resolver/matched_properties_cache.h
- M third_party/blink/renderer/core/css/resolver/matched_properties_cache_test.cc
- M third_party/blink/renderer/core/css/resolver/style_cascade.cc
- M third_party/blink/renderer/core/css/resolver/style_cascade.h
- M third_party/blink/renderer/core/css/resolver/style_cascade_test.cc
- M third_party/blink/renderer/core/css/resolver/style_resolver.cc
- M third_party/blink/renderer/core/css/resolver/style_resolver.h
- M third_party/blink/renderer/core/css/rule_set.cc
- M third_party/blink/renderer/core/css/rule_set.h
- M third_party/blink/renderer/core/css/style_rule.cc
- M third_party/blink/renderer/core/css/style_rule.h
- M third_party/blink/renderer/core/css/style_rule_test.cc
- M third_party/blink/renderer/core/css/style_scope.cc
- M third_party/blink/renderer/core/css/style_sheet_contents.cc
- M third_party/blink/renderer/core/inspector/inspector_css_agent.cc
- M third_party/blink/renderer/core/style/computed_style_test.cc
- D third_party/blink/web_tests/external/wpt/css/css-mixins/parameters-expected.txt
- M third_party/blink/web_tests/external/wpt/css/css-mixins/parameters.html
Change size: XL
Delta: 31 files changed, 805 insertions(+), 250 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Anders Hartvoll Ruud
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Sep 18, 2025, 8:59:24 AM (12 days ago) Sep 18
to Chromium LUCI CQ, Steinar H Gunderson, Anders Hartvoll Ruud, chrom...@appspot.gserviceaccount.com, Alexis Menard, chromium...@chromium.org, Olga Gerchikov, blink-rev...@chromium.org, blink-revie...@chromium.org, apavlo...@chromium.org, blink-revie...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, devtools-re...@chromium.org
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/54923
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages