Bug 226694

Created attachment 430678 [details] Patch

Tried to cc people who might be interested in work in this area. There is so much we can do to simplify our CSS object model implementation, but this correctness change is more important than that since it’s causing us to be different from the other browsers and fail WPT tests.

Comment on attachment 430678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430678&action=review > Source/WebCore/animation/CSSAnimation.cpp:51 > - , m_animationName(backingAnimation.name()) > + , m_animationName(backingAnimation.name().string) In the future we should see if m_animationName is necessary given the base class holds onto the backingAnimation and could pull the string from there. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3426 > + for (size_t i = 0; i < t->size(); ++i) { Would probably be a good idea for AnimationList to have an animations() function that return an IteratorRange<> (or just the underlying Vector) at some point to avoid this. > Source/WebCore/css/CSSUnits.h:71 > + CustomIdent, Daring, I like it!

Comment on attachment 430678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430678&action=review >> Source/WebCore/animation/CSSAnimation.cpp:51 >> + , m_animationName(backingAnimation.name().string) > > In the future we should see if m_animationName is necessary given the base class holds onto the backingAnimation and could pull the string from there. That’s a really good point. Seems like it is not. I am torn about what to pursue next. Further cleanup of things I encountered here like this would be great. The mission that led me here, though, was finding easy ways to pass a lot more WPT, and I was trying to start with counter-list-style improvements, which led me to this work. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3426 >> + for (size_t i = 0; i < t->size(); ++i) { > > Would probably be a good idea for AnimationList to have an animations() function that return an IteratorRange<> (or just the underlying Vector) at some point to avoid this. Absolutely! Well worth it. >> Source/WebCore/css/CSSUnits.h:71 >> + CustomIdent, > > Daring, I like it! Took a lot of discipline for me to not rename everything in the class right now. This code needs to be changed a lot more to be good long term.

Created attachment 430700 [details] Patch

Comment on attachment 430678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430678&action=review >>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3426 >>> + for (size_t i = 0; i < t->size(); ++i) { >> >> Would probably be a good idea for AnimationList to have an animations() function that return an IteratorRange<> (or just the underlying Vector) at some point to avoid this. > > Absolutely! Well worth it. I think the best idea here is to make AnimationList derive from Vector instead of holding one. This really just is a Vector with reference counting and one special function "fillUnsetProperties". Or maybe we can use use RefCountedArray instead of a Vector. Unsure how often these are resized.

Comment on attachment 430678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=430678&action=review >>>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3426 >>>> + for (size_t i = 0; i < t->size(); ++i) { >>> >>> Would probably be a good idea for AnimationList to have an animations() function that return an IteratorRange<> (or just the underlying Vector) at some point to avoid this. >> >> Absolutely! Well worth it. > > I think the best idea here is to make AnimationList derive from Vector instead of holding one. This really just is a Vector with reference counting and one special function "fillUnsetProperties". Or maybe we can use use RefCountedArray instead of a Vector. Unsure how often these are resized. To make this call site work as a modern for loop, all we’d have to do is add AnimationList::begin/end.

Committed r278540 (238538@main): <https://commits.webkit.org/238538@main>

<rdar://problem/78931824>

(In reply to Darin Adler from comment #2) > Tried to cc people who might be interested in work in this area. There is so > much we can do to simplify our CSS object model implementation, but this > correctness change is more important than that since it’s causing us to be > different from the other browsers and fail WPT tests. Yay! To check I'm understanding code I don't actually know(!): The reason why we have a separate CSSUnitType::CustomIdent to CSSUnitType::CSS_IDENT is because we effectively split CSS_IDENT up into smaller categories (with CSS_VALUE_ID and CSS_PROPERTY_ID too)?

(In reply to Sam Sneddon [:gsnedders] from comment #10) > The reason why we have a separate CSSUnitType::CustomIdent to > CSSUnitType::CSS_IDENT is because we effectively split CSS_IDENT up into > smaller categories (with CSS_VALUE_ID and CSS_PROPERTY_ID too)? That’s right. If we clean this up a bit, I’m not sure we will CSS_IDENT itself at all. The actual web-exposed CSS unit numbers are not using CSSUnitType; it’s entirely an internal concept. We should really separate the numeric types which are used for calculations, and conversion from one numeric type to another, from the other types that just let the internal primitive values hold the different sorts of values. And there’s no need for either of those to be a single enumeration that is exposed.

*** Bug 204506 has been marked as a duplicate of this bug. ***