Bug 203561

Summary: [Web Animations] Optimize blending for CSS Transitions
Product: WebKit Reporter: Antoine Quint <graouts>
Component: AnimationsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dino, simon.fraser, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=203588
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch dino: review+

Description Antoine Quint 2019-10-29 06:46:05 PDT
[Web Animations] Optimize blending for CSS Transitions
Comment 1 Antoine Quint 2019-10-29 06:50:30 PDT
Created attachment 382178 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-10-29 06:51:44 PDT
<rdar://problem/56705079>
Comment 3 Antoine Quint 2019-10-29 08:46:40 PDT
Created attachment 382183 [details]
Patch
Comment 4 Simon Fraser (smfr) 2019-10-29 08:53:54 PDT
Comment on attachment 382183 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.h:144
> +    enum class BlendingKeyframesComputationSource : uint8_t { CSSAnimation, CSSTransition, WebAnimation };

This is a bit longwinded. Maybe KeyframesSource?
Comment 5 Antoine Quint 2019-10-29 09:00:44 PDT
(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 382183 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382183&action=review
> 
> > Source/WebCore/animation/KeyframeEffect.h:144
> > +    enum class BlendingKeyframesComputationSource : uint8_t { CSSAnimation, CSSTransition, WebAnimation };
> 
> This is a bit longwinded. Maybe KeyframesSource?

We have two types of keyframes: m_parsedKeyframes and m_blendingKeyframes. So I think the Blending part of the name is important. However, I guess we can drop the  Computation part.
Comment 6 Antoine Quint 2019-10-29 09:09:02 PDT
Created attachment 382185 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2019-10-29 09:10:28 PDT
Comment on attachment 382185 [details]
Patch for landing

Rejecting attachment 382185 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 382185, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Traceback (most recent call last):
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 53, in run
    changelog_entry = ChangeLog(changelog_path).latest_entry()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 379, in latest_entry
    return self.parse_latest_entry_from_file(changelog_file)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 325, in parse_latest_entry_from_file
    return next(cls.parse_entries_from_file(changelog_file))
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 364, in parse_entries_from_file
    yield ChangeLogEntry(''.join(entry_lines[:-1]), revision=most_probable_revision)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 114, in __init__
    self._parse_entry()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/checkout/changelog.py", line 232, in _parse_entry
    self._author = self._committer_list.contributor_by_email(self.author_email()) or self._committer_list.contributor_by_name(self.author_name())
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/config/committers.py", line 386, in contributor_by_email
    return self._email_to_account_map().get(email.lower()) if email else None
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/config/committers.py", line 289, in _email_to_account_map
    assert(email not in self._accounts_by_email)  # We should never have duplicate emails.
AssertionError

Full output: https://webkit-queues.webkit.org/results/13188437
Comment 8 Antoine Quint 2019-10-29 09:17:26 PDT
Committed r251706: <https://trac.webkit.org/changeset/251706>
Comment 9 Truitt Savell 2019-10-29 14:37:37 PDT
The changes in https://trac.webkit.org/changeset/251706/webkit

broke webanimations/empty-keyframes-crash.html casting it to crash on Mac debug.

History: https://results.webkit.org/?suite=layout-tests&test=webanimations%2Fempty-keyframes-crash.html

Log:

Application Specific Information:
CRASHING TEST: webanimations/empty-keyframes-crash.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000002eb1b4810 WTFCrash + 16 (Assertions.cpp:305)
1   com.apple.WebCore             	0x00000002d8006c6b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x00000002d9d78ff8 WebCore::KeyframeEffect::setAnimatedPropertiesInStyle(WebCore::RenderStyle&, double) + 168 (KeyframeEffect.cpp:1070)
3   com.apple.WebCore             	0x00000002d9d78ce3 WebCore::KeyframeEffect::apply(WebCore::RenderStyle&) + 195 (KeyframeEffect.cpp:1029)


https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r251715%20(10173)/webanimations/empty-keyframes-crash-crash-log.txt
Comment 10 Antoine Quint 2019-10-30 02:49:55 PDT
Reopening to attach new patch.
Comment 11 Antoine Quint 2019-10-30 02:49:58 PDT
Created attachment 382292 [details]
Patch
Comment 12 Dean Jackson 2019-10-30 03:19:54 PDT
Comment on attachment 382292 [details]
Patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1075
> +        for (auto cssPropertyId : properties)
> +            CSSPropertyAnimation::blendProperties(this, cssPropertyId, &targetStyle, m_blendingKeyframes[0].style(), m_blendingKeyframes[1].style(), iterationProgress);

I think you can do *properties.begin()