Bug 76660 – [Texmap] Divide TextureMapperNode.cpp to 3 files.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 123219 [details]
Patch 1: Animations

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

> > 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?

[Noam Rosenthal]

> > > 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>() ?

[Noam Rosenthal]

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

[Noam Rosenthal]

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

Addressed some of Kenneth's comments.

[Noam Rosenthal]

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

[Noam Rosenthal]

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

[Simon Hausmann]

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?

[Simon Hausmann]

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

[Noam Rosenthal]

(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.

[WebKit Review Bot]

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

[Noam Rosenthal]

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

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 123848 [details]
Patch 1: Animations

[WebKit Review Bot]

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

[Noam Rosenthal]

Comment on attachment 123848 [details]
Patch 1: Animations

Rejected due to a flaky test, trying again.

[WebKit Review Bot]

Comment on attachment 123848 [details]
Patch 1: Animations

Clearing flags on attachment: 123848

Committed r105858: <http://trac.webkit.org/changeset/105858>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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]

[Csaba Osztrogonác]

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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

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

Fixed issue that caused some tests to crash.

[Noam Rosenthal]

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

[Noam Rosenthal]

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

[WebKit Review Bot]

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

Clearing flags on attachment: 124006

Committed r105925: <http://trac.webkit.org/changeset/105925>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

2 more patches coming...

[Eric Seidel (no email)]

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.

[Noam Rosenthal]

Created attachment 126966 [details]
Patch

[Noam Rosenthal]

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

[WebKit Review Bot]

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.

[Kenneth Rohde Christiansen]

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

rs=me

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 127122 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127122 [details]
Patch

Clearing flags on attachment: 127122

Committed r107787: <http://trac.webkit.org/changeset/107787>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.