[style] Fix serialization for CSS will-change. [chromium/src : main]
Ian Kilpatrick (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Anders Hartvoll Ruud
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Kilpatrick
| Commit-Queue | +1 |
1 comment
static bool HasPropertyThatCreatesStackingContext(@pdr I could precompute this as well in WillChange::ApplyValue let me know if you think its worth it.
Related details
- Anders Hartvoll Ruud
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
StyleWillChangeData(const Vector<WillChangeValue>& values,I think this can be optimized a bit? WDYT of one of the following?
- Take these by value and std::move them into the members. I.e., `StyleWillChangeData(Vector<WillChangeValue> values,`
- Pass by r-value references, and std::move them into the members. I.e., `StyleWillChangeData(Vector<WillChangeValue>&& values,`
Related details
- Anders Hartvoll Ruud
- Ian Kilpatrick
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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
static bool HasPropertyThatCreatesStackingContext(@pdr I could precompute this as well in WillChange::ApplyValue let me know if you think its worth it.
It seems like it could be a small win to do the pre-computation, but I looked up the profiles and this looks to be insignificant. I think better to not do this now.
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
StyleWillChangeData(const Vector<WillChangeValue>& values,I think this can be optimized a bit? WDYT of one of the following?
- Take these by value and std::move them into the members. I.e., `StyleWillChangeData(Vector<WillChangeValue> values,`
- Pass by r-value references, and std::move them into the members. I.e., `StyleWillChangeData(Vector<WillChangeValue>&& values,`
- Anders Hartvoll Ruud
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
Hi Ian, this looks good to me at a high level. Can I defer to Anders for the final review?
Related details
- Anders Hartvoll Ruud
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
2 comments
// The bitset only contains resolved CSSPropertyID(s). No aliases.Maybe also document whether it contains only longhands, only shorthands (not likely), or both.
uint16_t property_id : kCSSPropertyIDBitLength =
static_cast<uint16_t>(CSSPropertyID::kInvalid);Looks like this should actually be `AtomicString`, since (apparently) e.g. `will-change: not-supported` should compute to `not-supported` (though it has no effect).
_But_, this CL is about fixing _serialization_ order, and the above would scope-creep the CL to also modify parsing, probably.
Related details
- Ian Kilpatrick
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. |
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/1481a702090000
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted and added 7 comments![]( "Open in Gerrit")
Votes added by Ian Kilpatrick
| Commit-Queue | +1 |
7 comments
has_will_change_other = true;@pdr What is the purpose of CompositingReason::kWillChangeOther ?
E.g. I created a patch to remove kWillChangeOther - and it seemed fine?
https://chromium-review.googlesource.com/c/chromium/src/+/7654382
Its a bit weird that if top:0px isn't a compositing reason, but will-change:top is.
DCHECK(!IsPropertyAlias(id));@andruud - is there an easy way to DCEHCK a CSSPropertyID is a longhand?
(except for: `DCHECK(!shorthandForProperty(resolved_id).length())` )
case CSSPropertyID::kWebkitMaskBoxImage: // Matches longhand@pdr I removed kMask, and kWebkitMaskBoxImage as now we only have longhands.
// The bitset only contains resolved CSSPropertyID(s). No aliases.Maybe also document whether it contains only longhands, only shorthands (not likely), or both.
Ah thanks for the reminder - I missed that.
See diff https://chromium-review.googlesource.com/c/chromium/src/+/7650675/8..11
I expand out the shorthand in ApplyValue now.
uint16_t property_id : kCSSPropertyIDBitLength =
static_cast<uint16_t>(CSSPropertyID::kInvalid);Looks like this should actually be `AtomicString`, since (apparently) e.g. `will-change: not-supported` should compute to `not-supported` (though it has no effect).
_But_, this CL is about fixing _serialization_ order, and the above would scope-creep the CL to also modify parsing, probably.
So this should be relatively simple after this patch. I'll do a followup. The parsing already works, but we just drop the values (we also lose the caseness), e.g. `will-change: TOP` (but `will-change: SCROLL-POSITION` shouldn't work?)
"Has a will-change: mask-image compositing hint."},@pdr Now that we are just iterating over resolved longhands it didn't make much sense to keep this. Do you agree?
Related details
- Anders Hartvoll Ruud
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
Hi Ian, this looks good to me at a high level. Can I defer to Anders for the final review?
Thanks for the review - just need your input on a couple more things.
Related details
- Anders Hartvoll Ruud
- Philip Rogers
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. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
2 comments
DCHECK(!IsPropertyAlias(id));@andruud - is there an easy way to DCEHCK a CSSPropertyID is a longhand?
(except for: `DCHECK(!shorthandForProperty(resolved_id).length())` )
Hmm, I guess this would read well enough:
```
DCHECK_NE(id, CSSPropertyID::kInvalid);
DCHECK(CSSProperty::Get(id).IsShorthand());
```
If that's still not easy enough, you could add an additional `IsShorthand()` util to `CSSProperty` that accepts `CSSPropertyID` and doesn't explode on `kInvalid`.
// The bitset only contains resolved CSSPropertyID(s). No aliases.Ian KilpatrickMaybe also document whether it contains only longhands, only shorthands (not likely), or both.
Ah thanks for the reminder - I missed that.
See diff https://chromium-review.googlesource.com/c/chromium/src/+/7650675/8..11
I expand out the shorthand in ApplyValue now.
- Ian Kilpatrick
- Philip Rogers
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. |