Bug 76660

Created attachment 123219 [details] Patch 1: Animations

Comment on attachment 123219 [details] Patch 1: Animations View in context: https://bugs.webkit.org/attachment.cgi?id=123219&action=review Looks fine, but maybe someone else wants to have a look > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:368 > + bool listsMatch, hasBigRotation; two lines please > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:32 > + if (!duration) > + return 0; > + const int loopCount = runningTime / duration; I would normally add newlines after returns like this one > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:80 > + return solveCubicBezierFunction(ctf->x1(), > + ctf->y1(), > + ctf->x2(), > + ctf->y2(), > + progress, duration); why not just keep that one line, is should be no longer than the line above > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:94 > + // Optimization: special case the edge values (0 and 1). > + if (progress == 1.0 || !progress) { uh I dont like our coding style for this :-) > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.h:38 > + enum PlayState { PlayingState, PausedState, StoppedState }; why not AnimationState? > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.h:76 > + bool hasActiveAnimationsOfType(AnimatedPropertyID type) const; Would it make sense as a template?

> > > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.h:76 > > > + bool hasActiveAnimationsOfType(AnimatedPropertyID type) const; > > > > Would it make sense as a template? I don't really understand what that would give us. Something like bool hasActiveAnimationsOfType<AnimatedPropertyID>() ?

> Looks fine, but maybe someone else wants to have a look > Jocelyn/Igor, interested in taking a look?

Created attachment 123506 [details] Patch 1: Move animations to TextureMapperAnimations Addressed some of Kenneth's comments.

Created attachment 123507 [details] Patch 1: Move animations to TextureMapperAnimations

Created attachment 123508 [details] Patch 1: Move animations to TextureMapperAnimations

Comment on attachment 123508 [details] Patch 1: Move animations to TextureMapperAnimations View in context: https://bugs.webkit.org/attachment.cgi?id=123508&action=review > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:-381 > - for (size_t i = 0; i < m_animations.size(); ++i) { > - // The same animation name can be used for two animations with different properties. > - if (m_animations[i]->name != keyframesName || m_animations[i]->keyframes.property() != valueList.property()) > - continue; > - > - // We already have a copy of this animation, that means that we're resuming it rather than adding it. > - RefPtr<TextureMapperAnimation>& animation = m_animations[i]; > - animation->animation = Animation::create(anim); > - animation->paused = false; > - animation->startTime = WTF::currentTime() - timeOffset; > - notifyChange(TextureMapperNode::AnimationChange); > - m_animationStartedTimer.startOneShot(0); > - return true; > - } I don't see a functional equivalent of this part. Is it intentional?

Apart from my comment I think this is a nice move.

(In reply to comment #9) > (From update of attachment 123508 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123508&action=review > > > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:-381 > > - for (size_t i = 0; i < m_animations.size(); ++i) { > > - // The same animation name can be used for two animations with different properties. > > - if (m_animations[i]->name != keyframesName || m_animations[i]->keyframes.property() != valueList.property()) > > - continue; > > - > > - // We already have a copy of this animation, that means that we're resuming it rather than adding it. > > - RefPtr<TextureMapperAnimation>& animation = m_animations[i]; > > - animation->animation = Animation::create(anim); > > - animation->paused = false; > > - animation->startTime = WTF::currentTime() - timeOffset; > > - notifyChange(TextureMapperNode::AnimationChange); > > - m_animationStartedTimer.startOneShot(0); > > - return true; > > - } > > I don't see a functional equivalent of this part. Is it intentional? The functional equivalent part was in GraphicsLayerTextureMapper::addAnimation.

Comment on attachment 123508 [details] Patch 1: Move animations to TextureMapperAnimations Rejecting attachment 123508 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: ueue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors self._run(tool, options, state) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run step(tool, options).run(state) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 53, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: http://queues.webkit.org/results/11275149

Created attachment 123726 [details] Patch 1: Move animations to TextureMapperAnimations

Comment on attachment 123726 [details] Patch 1: Move animations to TextureMapperAnimations Rejecting attachment 123726 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 Last 500 characters of output: ueue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 70, in run_and_handle_errors self._run(tool, options, state) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run step(tool, options).run(state) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 53, in run if changelog_entry.has_valid_reviewer(): AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer' Full output: http://queues.webkit.org/results/11336331

Created attachment 123848 [details] Patch 1: Animations

Comment on attachment 123848 [details] Patch 1: Animations Rejecting attachment 123848 [details] from commit-queue. New failing tests: media/audio-garbage-collect.html Full output: http://queues.webkit.org/results/11177146

Comment on attachment 123848 [details] Patch 1: Animations Rejected due to a flaky test, trying again.

Comment on attachment 123848 [details] Patch 1: Animations Clearing flags on attachment: 123848 Committed r105858: <http://trac.webkit.org/changeset/105858>

All reviewed patches have been landed. Closing bug.

Reopen, because it made animations/transition-and-animation-2.html and animations/3d/transform-perspective.html crash on Qt. Could you check what happened?

http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Debug/r105858%20%2820616%29/results.html ASSERTION FAILED: i < size() ../../../../Source/JavaScriptCore/wtf/Vector.h(520) : T& WTF::Vector<T, inlineCapacity>::at(size_t) [with T = WTF::RefPtr<WebCore::TransformOperation>, long unsigned int inlineCapacity = 0ul]

I rolled out it to make bots able to catch new regressions again: http://trac.webkit.org/changeset/105868 Please fix the bug and then feel free to reland the fixed patch.

(In reply to comment #20) > Reopen, because it made animations/transition-and-animation-2.html and animations/3d/transform-perspective.html crash on Qt. Could you check what happened? Looking.

Created attachment 123969 [details] Patch 1: Move animations to TextureMapperAnimations Fixed issue that caused some tests to crash.

Created attachment 124003 [details] Patch 1: Move animations to TextureMapperAnimations

Created attachment 124006 [details] Patch 1: Move animations to TextureMapperAnimations

Comment on attachment 124006 [details] Patch 1: Move animations to TextureMapperAnimations Clearing flags on attachment: 124006 Committed r105925: <http://trac.webkit.org/changeset/105925>

All reviewed patches have been landed. Closing bug.

2 more patches coming...

Comment on attachment 123508 [details] Patch 1: Move animations to TextureMapperAnimations Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 123508 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Created attachment 126966 [details] Patch

Created attachment 126967 [details] Patch 2: Rename TextureMapperNode to TextureMapperLayer

Attachment 126967 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 126967 [details] Patch 2: Rename TextureMapperNode to TextureMapperLayer rs=me

Comment on attachment 126967 [details] Patch 2: Rename TextureMapperNode to TextureMapperLayer Rejecting attachment 126967 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: file Source/WebKit/qt/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/qt/WebCoreSupport/PageClientQt.cpp patching file Source/WebKit/qt/WebCoreSupport/PageClientQt.h patching file Source/WebKit2/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit2/UIProcess/qt/LayerTreeHostProxyQt.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kenneth Ro..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11514335

Created attachment 127122 [details] Patch

Comment on attachment 127122 [details] Patch Clearing flags on attachment: 127122 Committed r107787: <http://trac.webkit.org/changeset/107787>

All reviewed patches have been landed. Closing bug.