Bug 184077 – [Web Animations] Implement more CSSPropertyBlendingClient methods

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[Web Animations] Implement more CSSPropertyBlendingClient methods

[Antoine Quint]

Created attachment 336651 [details]
Patch

[EWS Watchlist]

Comment on attachment 336651 [details]
Patch

Attachment 336651 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7123663

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html
animations/trigger-container-scroll-empty.html

[EWS Watchlist]

Created attachment 336652 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 336651 [details]
Patch

Attachment 336651 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7123668

New failing tests:
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/addition-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property.html
imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/accumulation-per-property.html

[EWS Watchlist]

Created attachment 336653 [details]
Archive of layout-test-results from ews105 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Antoine Quint]

Created attachment 336655 [details]
Patch

[Jon Lee]

Comment on attachment 336655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336655&action=review

> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:733
> +        // An emtpy transform list matches anything.

Typo empty x 3

Is there a way to unify these matching implementations

> LayoutTests/fast/shapes/shape-outside-floats/shape-outside-shape-image-threshold-animation.html:1
> +<!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->

Missing doctype?

[Antoine Quint]

(In reply to Jon Lee from comment #7)
> Comment on attachment 336655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336655&action=review
> 
> > Source/WebCore/animation/KeyframeEffectReadOnly.cpp:733
> > +        // An emtpy transform list matches anything.
> 
> Typo empty x 3

I blame whoever wrote that code originally :)

> Is there a way to unify these matching implementations

I'll look into it.

> > LayoutTests/fast/shapes/shape-outside-floats/shape-outside-shape-image-threshold-animation.html:1
> > +<!-- webkit-test-runner [ enableCSSAnimationsAndCSSTransitionsBackedByWebAnimations=true ] -->
> 
> Missing doctype?

Some tests just don't have them. I'd like to make minimal changes as much as possible.

[Dean Jackson]

Comment on attachment 336655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336655&action=review

Shouldn't we be copying all these tests into a new directory, and have that version include the flag to use Web Animations? Otherwise we're only testing one of the code paths.

>>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:733
>>> +        // An emtpy transform list matches anything.
>> 
>> Typo empty x 3
>> 
>> Is there a way to unify these matching implementations
> 
> I blame whoever wrote that code originally :)

50% chance it was me.

[Dean Jackson]

Comment on attachment 336655 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336655&action=review

> LayoutTests/platform/mac-sierra/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property-expected.txt:141
> +FAIL filter: percentage or numeric-specifiable functions (number value) assert_equals: The value should be brightness(0.3) contrast(0.3) grayscale(0.3) invert(0.3) opacity(0.3) saturate(0.3) sepia(0.3) at 500ms expected "brightness(0.3) contrast(0.3) grayscale(0.3) invert(0.3) opacity(0.3) saturate(0.3) sepia(0.3)" but got "brightness(0.30000000000000004) contrast(0.30000000000000004) grayscale(0.30000000000000004) invert(0.30000000000000004) opacity(0.30000000000000004) saturate(0.30000000000000004) sepia(0.30000000000000004)"
> +FAIL filter: percentage or numeric-specifiable functions (percentage value) assert_equals: The value should be brightness(0.3) contrast(0.3) grayscale(0.3) invert(0.3) opacity(0.3) saturate(0.3) sepia(0.3) at 500ms expected "brightness(0.3) contrast(0.3) grayscale(0.3) invert(0.3) opacity(0.3) saturate(0.3) sepia(0.3)" but got "brightness(0.30000000000000004) contrast(0.30000000000000004) grayscale(0.30000000000000004) invert(0.30000000000000004) opacity(0.30000000000000004) saturate(0.30000000000000004) sepia(0.30000000000000004)"

These changes are a bit concerning. Why?

[Antoine Quint]

(In reply to Dean Jackson from comment #9)
> Comment on attachment 336655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336655&action=review
> 
> Shouldn't we be copying all these tests into a new directory, and have that
> version include the flag to use Web Animations? Otherwise we're only testing
> one of the code paths.

We don't care about the old code path, it'll go away as soon as we're done moving all the tests.

> >>> Source/WebCore/animation/KeyframeEffectReadOnly.cpp:733
> >>> +        // An emtpy transform list matches anything.
> >> 
> >> Typo empty x 3
> >> 
> >> Is there a way to unify these matching implementations
> > 
> > I blame whoever wrote that code originally :)
> 
> 50% chance it was me.

One could find out :)

[Antoine Quint]

(In reply to Dean Jackson from comment #10)
> Comment on attachment 336655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336655&action=review
> 
> > LayoutTests/platform/mac-sierra/imported/w3c/web-platform-tests/web-animations/animation-model/animation-types/interpolation-per-property-expected.txt:141
> > +FAIL filter: percentage or numeric-specifiable functions (number value) assert_equals: The value should be brightness(0.3) contrast(0.3) grayscale(0.3) invert(0.3) opacity(0.3) saturate(0.3) sepia(0.3) at 500ms expected "brightness(0.3) contrast(0.3) grayscale(0.3) invert(0.3) opacity(0.3) saturate(0.3) sepia(0.3)" but got "brightness(0.30000000000000004) contrast(0.30000000000000004) grayscale(0.30000000000000004) invert(0.30000000000000004) opacity(0.30000000000000004) saturate(0.30000000000000004) sepia(0.30000000000000004)"
> > +FAIL filter: percentage or numeric-specifiable functions (percentage value) assert_equals: The value should be brightness(0.3) contrast(0.3) grayscale(0.3) invert(0.3) opacity(0.3) saturate(0.3) sepia(0.3) at 500ms expected "brightness(0.3) contrast(0.3) grayscale(0.3) invert(0.3) opacity(0.3) saturate(0.3) sepia(0.3)" but got "brightness(0.30000000000000004) contrast(0.30000000000000004) grayscale(0.30000000000000004) invert(0.30000000000000004) opacity(0.30000000000000004) saturate(0.30000000000000004) sepia(0.30000000000000004)"
> 
> These changes are a bit concerning. Why?

We'll fix this test eventually, we want to get 100% conformance on the WPT web-animations tests.

[Antoine Quint]

Committed r230033: <https://trac.webkit.org/changeset/230033>

[Radar WebKit Bug Importer]

<rdar://problem/38962430>

[Ryan Haddad]

The tests modified with this change are failing an assertion:

ASSERTION FAILED: !frame().animation().hasAnimations()
./page/FrameView.cpp(593) : void WebCore::FrameView::didDestroyRenderTree()
1   0x10ba8361d WTFCrash
2   0x11d3f3e37 WebCore::FrameView::didDestroyRenderTree()
3   0x11ca5da7e WebCore::Document::destroyRenderTree()
4   0x11ca5dd21 WebCore::Document::prepareForDestruction()
5   0x11d3d5a60 WebCore::Frame::setView(WTF::RefPtr<WebCore::FrameView, WTF::DumbPtrTraits<WebCore::FrameView> >&&)
6   0x1162ca053 WebFrameLoaderClient::transitionToCommittedForNewPage()
7   0x11d23431f WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*)
8   0x11d2333ba WebCore::FrameLoader::commitProvisionalLoad()
9   0x11d1dcb3c WebCore::DocumentLoader::commitIfReady()
10  0x11d1dcea2 WebCore::DocumentLoader::finishedLoading()
11  0x11d1e6d99 WebCore::DocumentLoader::maybeLoadEmpty()
12  0x11d1e6f0e WebCore::DocumentLoader::startLoadingMainResource()
13  0x11d252884 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL)::$_14::operator()() const
14  0x11d2525c9 WTF::Function<void ()>::CallableWrapper<WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL)::$_14>::call()
15  0x11ac5be2b WTF::Function<void ()>::operator()() const
16  0x11d23164c WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WebCore::FormState*, bool, WebCore::AllowNavigationToInvalidURL)
17  0x11d24f4ac WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL, WTF::CompletionHandler<void ()>&&)::$_9::operator()(WebCore::ResourceRequest const&, WebCore::FormState*, bool) const
18  0x11d24f392 WTF::Function<void (WebCore::ResourceRequest&&, WebCore::FormState*, bool)>::CallableWrapper<WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL, WTF::CompletionHandler<void ()>&&)::$_9>::call(WebCore::ResourceRequest&&, WebCore::FormState*, bool)
19  0x11d28208d WTF::Function<void (WebCore::ResourceRequest&&, WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest&&, WebCore::FormState*, bool) const
20  0x11d274e19 WTF::CompletionHandler<void (WebCore::ResourceRequest&&, WebCore::FormState*, bool)>::operator()(WebCore::ResourceRequest&&, WebCore::FormState*, bool) const
21  0x11d27463c WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest&&, bool, WebCore::DocumentLoader*, WebCore::FormState*, WTF::CompletionHandler<void (WebCore::ResourceRequest&&, WebCore::FormState*, bool)>&&)
22  0x11d230e4c WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WebCore::FormState*, WebCore::AllowNavigationToInvalidURL, WTF::CompletionHandler<void ()>&&)
23  0x11d22d46b WebCore::FrameLoader::load(WebCore::DocumentLoader*)
24  0x11d23024d WebCore::FrameLoader::load(WebCore::FrameLoadRequest&&)
25  0x1162ba996 -[WebFrame loadRequest:]
26  0x10b10e577 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)
27  0x10b10c233 runTestingServerLoop()
28  0x10b10b742 dumpRenderTree(int, char const**)
29  0x10b10ea22 DumpRenderTreeMain(int, char const**)
30  0x10b190922 main
31  0x7fff4fed8115 start

https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/r230034%20(3046)/results.html

[Ryan Haddad]

Reverted r230033 for reason:

The LayoutTests modified in this change fail an assertion on WK1.

Committed r230043: <https://trac.webkit.org/changeset/230043>

[Antoine Quint]

Wish EWS had caught that earlier.

Anyway, we shouldn't even be calling into the CSSAnimationController in this codepath, I will conditionalize this ASSERT.

[Antoine Quint]

Committed r230068: <https://trac.webkit.org/changeset/230068>