Bug 184077 - [Web Animations] Implement more CSSPropertyBlendingClient methods
Summary: [Web Animations] Implement more CSSPropertyBlendingClient methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-03-28 05:16 PDT by Antoine Quint
Modified: 2018-03-28 23:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (72.10 KB, patch)
2018-03-28 05:19 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.28 MB, application/zip)
2018-03-28 06:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-sierra-wk2 (2.67 MB, application/zip)
2018-03-28 06:27 PDT, Build Bot
no flags Details
Patch (102.66 KB, patch)
2018-03-28 06:42 PDT, Antoine Quint
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2018-03-28 05:16:24 PDT
[Web Animations] Implement more CSSPropertyBlendingClient methods
Comment 1 Antoine Quint 2018-03-28 05:19:03 PDT
Created attachment 336651 [details]
Patch
Comment 2 Build Bot 2018-03-28 06:22:45 PDT
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
Comment 3 Build Bot 2018-03-28 06:22:46 PDT
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
Comment 4 Build Bot 2018-03-28 06:27:32 PDT
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
Comment 5 Build Bot 2018-03-28 06:27:34 PDT
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
Comment 6 Antoine Quint 2018-03-28 06:42:38 PDT
Created attachment 336655 [details]
Patch
Comment 7 Jon Lee 2018-03-28 09:31:02 PDT
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?
Comment 8 Antoine Quint 2018-03-28 09:58:59 PDT
(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.
Comment 9 Dean Jackson 2018-03-28 10:19:01 PDT
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.
Comment 10 Dean Jackson 2018-03-28 10:20:16 PDT
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?
Comment 11 Antoine Quint 2018-03-28 10:36:00 PDT
(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 :)
Comment 12 Antoine Quint 2018-03-28 10:36:18 PDT
(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.
Comment 13 Antoine Quint 2018-03-28 10:37:51 PDT
Committed r230033: <https://trac.webkit.org/changeset/230033>
Comment 14 Radar WebKit Bug Importer 2018-03-28 10:38:28 PDT
<rdar://problem/38962430>
Comment 15 Ryan Haddad 2018-03-28 13:19:03 PDT
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
Comment 16 Ryan Haddad 2018-03-28 13:31:34 PDT
Reverted r230033 for reason:

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

Committed r230043: <https://trac.webkit.org/changeset/230043>
Comment 17 Antoine Quint 2018-03-28 14:15:16 PDT
Wish EWS had caught that earlier.

Anyway, we shouldn't even be calling into the CSSAnimationController in this codepath, I will conditionalize this ASSERT.
Comment 18 Antoine Quint 2018-03-28 23:48:26 PDT
Committed r230068: <https://trac.webkit.org/changeset/230068>