TextureMapperNode.cpp is becoming too complex to maintain. Essentially, it can be renamed/divided to 3 parts: 1. painting and tree management, should be called TextureMapperLayer (like ChromiumLayer) 2. animation management, should be called TextureMapperAnimation 3. backing-store/tile management, should be called TextureMapperBackingStore. There are several fixes pending for each of these parts, it would make it much easier to perform those fixes after the files are divided.
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: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>
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>