Reland "Fix getPropertyValue() and setProperty() behavior for 'all'" [chromium/src : main]
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/blink_perf.css complete.
- CSSPropertyUpdateValue: base median = 25353.128994718958 -> patched median = 25480.101536695685
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17139b40090000
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
Related details
- Jinho Bang
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
Adding sesse@ who should be a better reviewer for this.
Related details
- Jinho Bang
- Steinar H Gunderson
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
Reland "Fix getPropertyValue() and setProperty() behavior for 'all'"Do you want me to review this as a reland with minor changes, or fully from scratch? The latter would be quite a bit more involved.
Related details
- Jinho Bang
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 11 comments![]( "Open in Gerrit")
using HasAllField = MayHaveLogicalPropertiesField::DefineNextValue<bool, 1>;Do we need to define this for immutable sets? We should be consistent with MayHaveLogicalPropertiesField, I guess.
bits_.set<HasAllField>(bits_.get<HasAllField>() ||I know MayHaveLogicalPropertiesField is already written this way, but I don't think it makes sense to do all this bit manipulation per-property. Shouldn't we just have a bool instead, and set it after the loop? Same below, unless we take it out of the immutable sets. (And same for MayHaveLogicalPropertiesField.)
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {All other code here uses ++i, not i++.
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Why do you start only after all_index? Do you have some sort of guarantee of ordering here?
if (property.IsAffectedByAll() &&This definitely requires some sort of comment.
if constexpr (std::is_same_v<T, CSSPropertyID>) {Why does this not hold for CSSProperty?
if (bits_.get<HasAllField>() &&This needs a comment. What is the logic here?
if (bits_.get<HasAllField>()) [[unlikely]] {Please don't add [[likely]]/[[unlikely]].
if (property_id == CSSPropertyID::kAll ||This needs a comment.
bits_.set<HasAllField>(bits_.get<HasAllField>() ||Isn't this more logically written as
```
if (id == CSSPropertyID::kAll) {
bits_.set<HasAllField>(true);
}
```
? Same below.
But we need a comment saying why “all” figures here in a longhand function, despite being a shorthand in the spec.
Same below.
property_vector_.clear();Should we perhaps call Clear() instead?
Suyeon Ji (Gerrit)
Message from Suyeon Ji![]( "Open in Gerrit")
- Jinho Bang
- Steinar H Gunderson
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 12 comments![]( "Open in Gerrit")
Reland "Fix getPropertyValue() and setProperty() behavior for 'all'"Do you want me to review this as a reland with minor changes, or fully from scratch? The latter would be quite a bit more involved.
Thank you for your detailed review!
using HasAllField = MayHaveLogicalPropertiesField::DefineNextValue<bool, 1>;Do we need to define this for immutable sets? We should be consistent with MayHaveLogicalPropertiesField, I guess.
I'm not sure if this is exactly what you were asking for, but unlike MayHaveLogicalPropertiesField, this is required even for immutable sets, for instance, during SerializeShorthand(). Since Chromium treats the all shorthand as a longhand, handling it at runtime is the intended way to fix this bug and align with the spec.
bits_.set<HasAllField>(bits_.get<HasAllField>() ||I know MayHaveLogicalPropertiesField is already written this way, but I don't think it makes sense to do all this bit manipulation per-property. Shouldn't we just have a bool instead, and set it after the loop? Same below, unless we take it out of the immutable sets. (And same for MayHaveLogicalPropertiesField.)
I updated it to use a bool and set it after the loop as you commented.
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {All other code here uses ++i, not i++.
Done
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Why do you start only after all_index? Do you have some sort of guarantee of ordering here?
The CSS shorthand all resets every property it affects. With this patch, the index of all and the properties affected by it are guaranteed to be in a consistent order. This means that properties from index 0 up to all's index have already been reset by all, so they can be treated as having the same value as all.
For example,
Given `left: 10px; all: revert; right: 20px`:
vector: [left: 10px, all: revert, right: 20px]
left (index 0 < 1) is reset by all, right (index 2 > 1) keeps its own value.
No need to store all's value into each property, index comparison is sufficient.
Given `left: 10px; all: revert; right: 20px; all: inherit`:
vector: [left: 10px, right: 20px, all: inherit]
all is removed and re-appended, so both left and right are now before all and both are reset.
This definitely requires some sort of comment.
I added a comment before the `for` loop. Thanks!
if constexpr (std::is_same_v<T, CSSPropertyID>) {Why does this not hold for CSSProperty?
I assume it wasn't needed here because this template is only instantiated for CSSPropertyID, AtRuleDescriptorID, and AtomicString, but not for CSSProperty. Does this align with your understanding?
This needs a comment. What is the logic here?
I added a comment. please take a look again.
Please don't add [[likely]]/[[unlikely]].
I saw a 1.5% performance regression in blink_perf.css/CSSPropertyUpdateValue.html without [[unlikely]] when I ran it on my Mac. Since this code is called so frequently, I'm wondering, is a 1.5% regression small enough to ignore, or will PGO automatically optimize this anyway?
if (property_id == CSSPropertyID::kAll ||Suyeon JiThis needs a comment.
Done
bits_.set<HasAllField>(bits_.get<HasAllField>() ||Isn't this more logically written as
```
if (id == CSSPropertyID::kAll) {
bits_.set<HasAllField>(true);
}
```? Same below.
But we need a comment saying why “all” figures here in a longhand function, despite being a shorthand in the spec.
Same below.
Done, thank you for detailed.
Should we perhaps call Clear() instead?
Done
Related details
- Jinho Bang
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Suyeon JiWhy do you start only after all_index? Do you have some sort of guarantee of ordering here?
The CSS shorthand all resets every property it affects. With this patch, the index of all and the properties affected by it are guaranteed to be in a consistent order. This means that properties from index 0 up to all's index have already been reset by all, so they can be treated as having the same value as all.
For example,
Given `left: 10px; all: revert; right: 20px`:
vector: [left: 10px, all: revert, right: 20px]left (index 0 < 1) is reset by all, right (index 2 > 1) keeps its own value.
No need to store all's value into each property, index comparison is sufficient.Given `left: 10px; all: revert; right: 20px; all: inherit`:
vector: [left: 10px, right: 20px, all: inherit]all is removed and re-appended, so both left and right are now before all and both are reset.
What is this consistent order? Is it documented anywhere? I don't think I see any new ordering code here.
More importantly, I think this is just too subtle. Some properties are removed on insert (and we try to maintain that invariant, seemingly without documenting it) but some are not and must be filtered on get? Is there a good reason why we cannot choose one or the other, consistently? It would seem reasonable to me that set('all', ...) removes the offending elements, so that they also disappear on iteration.
Related details
- Jinho Bang
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Suyeon JiWhy do you start only after all_index? Do you have some sort of guarantee of ordering here?
Steinar H GundersonThe CSS shorthand all resets every property it affects. With this patch, the index of all and the properties affected by it are guaranteed to be in a consistent order. This means that properties from index 0 up to all's index have already been reset by all, so they can be treated as having the same value as all.
For example,
Given `left: 10px; all: revert; right: 20px`:
vector: [left: 10px, all: revert, right: 20px]left (index 0 < 1) is reset by all, right (index 2 > 1) keeps its own value.
No need to store all's value into each property, index comparison is sufficient.Given `left: 10px; all: revert; right: 20px; all: inherit`:
vector: [left: 10px, right: 20px, all: inherit]all is removed and re-appended, so both left and right are now before all and both are reset.
What is this consistent order? Is it documented anywhere? I don't think I see any new ordering code here.
More importantly, I think this is just too subtle. Some properties are removed on insert (and we try to maintain that invariant, seemingly without documenting it) but some are not and must be filtered on get? Is there a good reason why we cannot choose one or the other, consistently? It would seem reasonable to me that set('all', ...) removes the offending elements, so that they also disappear on iteration.
Sorry for the delay. I've updated the patch after carefully reviewing your feedback.
In a previous patchset[1], I had implemented this using RemovePropertiesAffectedByAll(). I was initially concerned about potential side effects due to a subtle difference in cascade_expansion_test[2], but since removeProperty("all") already uses the same approach, I followed your suggestion and adopted it here as well.
I may not fully understand your intention, but even with RemovePropertiesAffectedByAll(), the filtering is still necessary to detect when a property set after 'all' overrides it.
```
style.setProperty("all", "revert"); // [all:revert]
style.setProperty("width", "50px"); // [all:revert, width:50px]
style.getPropertyValue("width"); // "50px"
style.getPropertyValue("color"); // "revert"
style.getPropertyValue("all"); // ""
```
[1] https://chromium-review.googlesource.com/c/chromium/src/+/7579493/9
[2] https://chromium-review.googlesource.com/c/chromium/src/+/7579493/9/third_party/blink/renderer/core/css/resolver/cascade_expansion_test.cc
Related details
- Jinho Bang
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Let me try to understand what the strategy is. Are you saying that
1) “all” is kept as a longhand, even though it is a shorthand by the standard. (We expand it in the cascade)
2) Properties are always kept in order of appearance (whether that is as written in the stylesheet, or through some CSSOM modification).
3) As an optimization, we filter out all longhand properties that are redundant by later “all” properties, but we cannot do the same filtering by properties set after the last “all”. This affects storage space only, not really performance.
4) When doing get on a longhand (that is affected by “all”), we must see both whether there is a relevant “all” (with a fast path in the negative case by means of a flag) _and_ whether it is overridden by the actual longhand
5) When doing get on a shorthand, we need to do the same operation for each constituent longhand somehow (I haven't tried to figure out the details yet)
Is my understanding right? If so, this needs to be written down in a (class) comment somewhere, because it's not obvious at all. Someone needs to be able to come here in five years and read up on why we are doing all of these things. :-)
Related details
- Jinho Bang
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 1 comment![]( "Open in Gerrit")
for (unsigned i = all_index + 1; i < property_set.PropertyCount(); i++) {Thanks for your review. You're right.
I've left comments at the top of CSSPropertyValueSet class.
Please take another look. Thank you.
Related details
- Jinho Bang
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 5 comments![]( "Open in Gerrit")
Generally looks good now. Please run Speedometer3 on Pinpoint (typically on M1) to check that there are no important regressions.
// Since 'all' is stored as a single entry instead of expanding intoexpanding -> expanded
if constexpr (std::is_same_v<T, CSSPropertyID>) {Why does this not hold for CSSProperty?
I assume it wasn't needed here because this template is only instantiated for CSSPropertyID, AtRuleDescriptorID, and AtomicString, but not for CSSProperty. Does this align with your understanding?
I see. Perhaps this is unduly magical, though? Can we perhaps do something like this instead?
if (IsAffectedByAll(property) && bits_.get<HasAllField>()) { … }and then
```
static bool IsAffectedByAll(CSSPropertyID id) { return CSSProperty::Get(id).IsAffectedByAll(); }
// Custom properties and descriptors are never affected by 'all'.
static bool IsAffectedByAll(const AtomicString& property) { return false; }
static bool IsAffectedByAll(AtRuleDescriptorID id) { return false; }
```
Because right now, it sure seems like one could call this function with a CSSProperty and get the expected result, but it would silently fail.
Does it make sense?
if (bits_.get<HasAllField>()) [[unlikely]] {Please don't add [[likely]]/[[unlikely]].
I saw a 1.5% performance regression in blink_perf.css/CSSPropertyUpdateValue.html without [[unlikely]] when I ran it on my Mac. Since this code is called so frequently, I'm wondering, is a 1.5% regression small enough to ignore, or will PGO automatically optimize this anyway?
PGO optimizes likely/unlikely for you and discards the manual annotations.
// must check whether 'all' is present. This flag makes that check fast.I don't think you need to mention this here (repeatedly), since you don't actually do any such checks here. If you want to note that, the .h file is a better place. But I think it can just be deleted.
Related details
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji added 5 comments![]( "Open in Gerrit")
Generally looks good now. Please run Speedometer3 on Pinpoint (typically on M1) to check that there are no important regressions.
Thanks for the review. Here are the Pinpoint results.
Please take a look when you have a chance.
[1] https://pinpoint-dot-chromeperf.appspot.com/job/17bb1976090000
[2] https://pinpoint-dot-chromeperf.appspot.com/job/161f2439090000
[3] https://pinpoint-dot-chromeperf.appspot.com/job/14ec220d090000
// Since 'all' is stored as a single entry instead of expanding intoSuyeon Jiexpanding -> expanded
Done. Thank you.
if constexpr (std::is_same_v<T, CSSPropertyID>) {Suyeon JiWhy does this not hold for CSSProperty?
Steinar H GundersonI assume it wasn't needed here because this template is only instantiated for CSSPropertyID, AtRuleDescriptorID, and AtomicString, but not for CSSProperty. Does this align with your understanding?
I see. Perhaps this is unduly magical, though? Can we perhaps do something like this instead?
if (IsAffectedByAll(property) && bits_.get<HasAllField>()) { … }and then
```
static bool IsAffectedByAll(CSSPropertyID id) { return CSSProperty::Get(id).IsAffectedByAll(); }
// Custom properties and descriptors are never affected by 'all'.
static bool IsAffectedByAll(const AtomicString& property) { return false; }
static bool IsAffectedByAll(AtRuleDescriptorID id) { return false; }
```Because right now, it sure seems like one could call this function with a CSSProperty and get the expected result, but it would silently fail.
Does it make sense?
It makes sense. I've updated it as you said and swapped the order, as IsAffectedByAll() returns true for most properties. Thanks!
if (bits_.get<HasAllField>()) [[unlikely]] {Suyeon JiPlease don't add [[likely]]/[[unlikely]].
Steinar H GundersonI saw a 1.5% performance regression in blink_perf.css/CSSPropertyUpdateValue.html without [[unlikely]] when I ran it on my Mac. Since this code is called so frequently, I'm wondering, is a 1.5% regression small enough to ignore, or will PGO automatically optimize this anyway?
PGO optimizes likely/unlikely for you and discards the manual annotations.
Thanks, that's good to know.
// must check whether 'all' is present. This flag makes that check fast.I don't think you need to mention this here (repeatedly), since you don't actually do any such checks here. If you want to note that, the .h file is a better place. But I think it can just be deleted.
- Steinar H Gunderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16a9255d090000
Related details
- Steinar H Gunderson
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Steinar H Gunderson
| Code-Review | +1 |
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jinho Bang (Gerrit)
Jinho Bang voted![]( "Open in Gerrit")
| Code-Review | +1 |
| Commit-Queue | +2 |
Related details
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/58806.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Suyeon Ji
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Suyeon Ji (Gerrit)
Suyeon Ji voted Commit-Queue+2![]( "Open in Gerrit")
- Jinho Bang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Reland "Fix getPropertyValue() and setProperty() behavior for 'all'"
This is a reland of commit dba0609c231bcea64dd43ed2ab79dc8f509e23e6
Reason of reland:
This patch replaces the reordering of 'all' and its dependencies with
RemovePropertiesAffectedByAll() to align spec. It also introduces bitflags to ensure no further regressions occur.
This was also verified it on Linux, and the Pinpoint results are attached below.
Pinpoint:
On my local Linux machine:
Before After
1st 4956 runs/s 4948 runs/s
2nd 4949 runs/s 4960 runs/s
3rd 4962 runs/s 4955 runs/s
4th 4918 runs/s 4931 runs/s
5th 4942 runs/s 4928 runs/s
avg 4945 runs/s 4945 runs/s
Original change's description:
> Fix getPropertyValue() and setProperty() behavior for 'all'
>
> According to the CSS spec[1], the 'all' property is a shorthand.
> Shorthands, especially 'all', can reset all CSS properties (except 'direction', 'unicode-bidi', custom properties) to a given value.
>
> However, Chromium currently does not follow the spec.
> First, setProperty('all') does not reset all properties. So when
> querying properties affected by 'all', the correct value is not
> returned.
> Second, for a shorthand, getPropertyValue()[2] should return the
> serialized value of its longhands, but currently 'all' does not
> do this.
>
> This patch fixes the behavior to match the specification.
>
> [1]: https://drafts.csswg.org/css-cascade/#all-shorthand
> [2]: https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue
>
> Bug: 388039764, 387828392
> Change-Id: Iaf1c7d6ca403dc57863ed72c5c79a5be4fc293c5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7579493
> Reviewed-by: Rune Lillesveen <fut...@chromium.org>
> Reviewed-by: Jinho Bang <zi...@chromium.org>
> Commit-Queue: Jinho Bang <zi...@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1593609}
- M third_party/blink/renderer/core/css/css_property_value_set.cc
- M third_party/blink/renderer/core/css/css_property_value_set.h
- M third_party/blink/renderer/core/css/resolver/cascade_expansion_test.cc
- M third_party/blink/renderer/core/css/style_property_serializer.cc
- D third_party/blink/web_tests/external/wpt/css/cssom/cssstyledeclaration-all-shorthand-expected.txt
- M third_party/blink/web_tests/external/wpt/css/cssom/cssstyledeclaration-all-shorthand.html
Code-Review: +1 by Steinar H Gunderson, +1 by Jinho Bang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
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/58806
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |