Always provide a CSSSelectorParserContext to pseudoElementIdentifierFromString in WebCore/animation#59160
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
webkit-ews-buildbot
added
the
merging-blocked
| bool compareAnimationEventsByCompositeOrder(const AnimationEventBase&, const AnimationEventBase&); | ||
| String pseudoElementIdentifierAsString(const std::optional<Style::PseudoElementIdentifier>&); | ||
| std::pair<bool, std::optional<Style::PseudoElementIdentifier>> pseudoElementIdentifierFromString(const String&, Document*); | ||
| std::pair<bool, std::optional<Style::PseudoElementIdentifier>> pseudoElementIdentifierFromString(const String&, Document&); |
There was a problem hiding this comment.
I would just provide the CSSSelectorParserContext. (I would probably move this function and its twin, pseudoElementIdentifierAsString, to less animation specific files, but that's for another time).
There was a problem hiding this comment.
I am interested in doing this, but I first want to know what pseudo-element API gaps the animation API still has.
| if (RefPtr document = context.client.document()) | ||
| return LineWidth::snapLengthAsBorderWidth(blendedValue, document->deviceScaleFactor()); | ||
| return blendedValue; | ||
| return LineWidth::snapLengthAsBorderWidth(blendedValue, context.client.document().deviceScaleFactor()); |
annevk
changed the title
annevk
force-pushed
the
pseudoElementIdentifierFromString
branch
from
2b64801 to
f1a7222
Compare
|
EWS run on current version of this PR (hash f1a7222) Details |
| { | ||
| RefPtr node = dynamicDowncast<Node>(target()); | ||
| auto [parsed, pseudoElementIdentifier] = pseudoElementIdentifierFromString(m_pseudoElement, node ? protect(node->document()).ptr() : nullptr); | ||
| auto [parsed, pseudoElementIdentifier] = pseudoElementIdentifierFromString(m_pseudoElement, CSSSelectorParserContext { node ? protect(node->document()).get() : document }); |
There was a problem hiding this comment.
I don't think using the node is useful here anymore
There was a problem hiding this comment.
They can be different though. Would be a somewhat obscure test if one can be constructed at all.
There was a problem hiding this comment.
It doesn't make a difference for the CSSParserContext. It's also pretty arbitrary in this context that we end up branching on the existance of the target node, that means the exact document doesn't really matter to us in this specific case.
There was a problem hiding this comment.
I'm not sure why it doesn't make a difference? If we ever added some kind of pseudo-element quirk it could matter, no?
There was a problem hiding this comment.
I guess so, but my second point still stands
There was a problem hiding this comment.
Okay, but then it should be a separate change from this refactor.
There was a problem hiding this comment.
seems ok if you want to do this in a followup, but it shouldn't be risky to do it in the same change
|
|
||
| ASSERT(document()); | ||
| auto& settings = document()->settings(); | ||
| auto& settings = document().settings(); |
There was a problem hiding this comment.
Can you Ref that baby while you're nearby?
| if (RefPtr document = this->document()) { | ||
| auto& settings = document->settings(); | ||
| auto isMarker = m_pseudoElementIdentifier && m_pseudoElementIdentifier->type == PseudoElementType::Marker; | ||
| auto& settings = document().settings(); |
graouts
left a comment
graouts
left a comment
There was a problem hiding this comment.
You could makes the Ref changes when referencing settings if you like, but I do realize it would mean further changes to where those settings are used.
|
|
||
| ASSERT(document()); | ||
| auto& settings = document()->settings(); | ||
| auto& settings = document().settings(); |
|
|
||
| ASSERT(document()); | ||
| auto& settings = document()->settings(); | ||
| auto& settings = document().settings(); |
|
|
||
| ASSERT(document()); | ||
| auto& settings = document()->settings(); | ||
| auto& settings = document().settings(); |
| bool isPropertyAdditiveOrCumulative(KeyframeInterpolation::Property) const final; | ||
|
|
||
| WeakPtr<Document, WeakPtrImplWithEventTargetData> m_document; | ||
| WeakRef<Document, WeakPtrImplWithEventTargetData> m_document; |
|
|
||
| ASSERT(effect.document()); | ||
| auto& settings = effect.document()->settings(); | ||
| auto& settings = effect.document().settings(); |
|
@graouts according to expectation files (Eventually we'll get an anti-ref checker that would highlight where we could have avoided taking a reference.) |
annevk
added
the
unsafe-merge-queue
…romString in WebCore/animation https://bugs.webkit.org/show_bug.cgi?id=308385 Reviewed by Antoine Quint and Tim Nguyen. For KeyframeEffect the document can in fact not be null and for synthetic animation events we can forward a document from JavaScript. Canonical link: https://commits.webkit.org/308181@main
webkit-commit-queue
force-pushed
the
pseudoElementIdentifierFromString
branch
from
f1a7222 to
7663646
Compare
|
Committed 308181@main (7663646): https://commits.webkit.org/308181@main Reviewed commits have been landed. Closing PR #59160 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
7 participants
7663646
f1a7222