Bug 76660 - [Texmap] Divide TextureMapperNode.cpp to 3 files.
Summary: [Texmap] Divide TextureMapperNode.cpp to 3 files.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords: Qt
Depends on: 77004
Blocks: 75918
  Show dependency treegraph
 
Reported: 2012-01-19 13:36 PST by Noam Rosenthal
Modified: 2012-02-15 00:26 PST (History)
6 users (show)

See Also:


Attachments
Patch 1: Animations (34.65 KB, patch)
2012-01-19 16:12 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 1: Move animations to TextureMapperAnimations (26.16 KB, application/octet-stream)
2012-01-22 18:25 PST, Noam Rosenthal
no flags Details
Patch 1: Move animations to TextureMapperAnimations (27.92 KB, patch)
2012-01-22 18:51 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 1: Move animations to TextureMapperAnimations (34.82 KB, patch)
2012-01-22 18:55 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 1: Move animations to TextureMapperAnimations (34.83 KB, patch)
2012-01-24 06:47 PST, Noam Rosenthal
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch 1: Animations (34.83 KB, patch)
2012-01-24 17:06 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 1: Move animations to TextureMapperAnimations (21.83 KB, patch)
2012-01-25 10:21 PST, Noam Rosenthal
noam: commit-queue-
Details | Formatted Diff | Diff
Patch 1: Move animations to TextureMapperAnimations (24.22 KB, patch)
2012-01-25 13:55 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 1: Move animations to TextureMapperAnimations (34.71 KB, patch)
2012-01-25 13:58 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (54.35 KB, patch)
2012-02-14 05:43 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch 2: Rename TextureMapperNode to TextureMapperLayer (54.34 KB, patch)
2012-02-14 05:44 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (94.41 KB, patch)
2012-02-14 23:25 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-01-19 13:36:49 PST
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.
Comment 1 Noam Rosenthal 2012-01-19 16:12:47 PST
Created attachment 123219 [details]
Patch 1: Animations
Comment 2 Kenneth Rohde Christiansen 2012-01-22 11:40:35 PST
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?
Comment 3 Noam Rosenthal 2012-01-22 11:47:57 PST
> > 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?
Comment 4 Noam Rosenthal 2012-01-22 11:49:00 PST
> > > 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>() ?
Comment 5 Noam Rosenthal 2012-01-22 11:49:59 PST
> Looks fine, but maybe someone else wants to have a look
> 
Jocelyn/Igor, interested in taking a look?
Comment 6 Noam Rosenthal 2012-01-22 18:25:59 PST
Created attachment 123506 [details]
Patch 1: Move animations to TextureMapperAnimations

Addressed some of Kenneth's comments.
Comment 7 Noam Rosenthal 2012-01-22 18:51:14 PST
Created attachment 123507 [details]
Patch 1: Move animations to TextureMapperAnimations
Comment 8 Noam Rosenthal 2012-01-22 18:55:37 PST
Created attachment 123508 [details]
Patch 1: Move animations to TextureMapperAnimations
Comment 9 Simon Hausmann 2012-01-23 01:59:24 PST
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?
Comment 10 Simon Hausmann 2012-01-23 02:01:32 PST
Apart from my comment I think this is a nice move.
Comment 11 Noam Rosenthal 2012-01-23 06:03:52 PST
(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 12 WebKit Review Bot 2012-01-23 18:02:06 PST
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
Comment 13 Noam Rosenthal 2012-01-24 06:47:52 PST
Created attachment 123726 [details]
Patch 1: Move animations to TextureMapperAnimations
Comment 14 WebKit Review Bot 2012-01-24 06:52:13 PST
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
Comment 15 Noam Rosenthal 2012-01-24 17:06:42 PST
Created attachment 123848 [details]
Patch 1: Animations
Comment 16 WebKit Review Bot 2012-01-24 18:42:02 PST
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 17 Noam Rosenthal 2012-01-24 19:30:26 PST
Comment on attachment 123848 [details]
Patch 1: Animations

Rejected due to a flaky test, trying again.
Comment 18 WebKit Review Bot 2012-01-25 03:11:27 PST
Comment on attachment 123848 [details]
Patch 1: Animations

Clearing flags on attachment: 123848

Committed r105858: <http://trac.webkit.org/changeset/105858>
Comment 19 WebKit Review Bot 2012-01-25 03:11:32 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Csaba Osztrogonác 2012-01-25 03:32:29 PST
Reopen, because it made animations/transition-and-animation-2.html and animations/3d/transform-perspective.html crash on Qt. Could you check what happened?
Comment 21 Csaba Osztrogonác 2012-01-25 04:17:27 PST
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]
Comment 22 Csaba Osztrogonác 2012-01-25 04:40:41 PST
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.
Comment 23 Noam Rosenthal 2012-01-25 06:15:16 PST
(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.
Comment 24 Noam Rosenthal 2012-01-25 10:21:07 PST
Created attachment 123969 [details]
Patch 1: Move animations to TextureMapperAnimations

Fixed issue that caused some tests to crash.
Comment 25 Noam Rosenthal 2012-01-25 13:55:27 PST
Created attachment 124003 [details]
Patch 1: Move animations to TextureMapperAnimations
Comment 26 Noam Rosenthal 2012-01-25 13:58:37 PST
Created attachment 124006 [details]
Patch 1: Move animations to TextureMapperAnimations
Comment 27 WebKit Review Bot 2012-01-25 14:13:44 PST
Comment on attachment 124006 [details]
Patch 1: Move animations to TextureMapperAnimations

Clearing flags on attachment: 124006

Committed r105925: <http://trac.webkit.org/changeset/105925>
Comment 28 WebKit Review Bot 2012-01-25 14:13:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Noam Rosenthal 2012-01-25 17:22:11 PST
2 more patches coming...
Comment 30 Eric Seidel (no email) 2012-02-10 10:20:24 PST
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.
Comment 31 Noam Rosenthal 2012-02-14 05:43:44 PST
Created attachment 126966 [details]
Patch
Comment 32 Noam Rosenthal 2012-02-14 05:44:50 PST
Created attachment 126967 [details]
Patch 2: Rename TextureMapperNode to TextureMapperLayer
Comment 33 WebKit Review Bot 2012-02-14 05:46:50 PST
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 34 Kenneth Rohde Christiansen 2012-02-14 05:48:28 PST
Comment on attachment 126967 [details]
Patch 2: Rename TextureMapperNode to TextureMapperLayer

rs=me
Comment 35 WebKit Review Bot 2012-02-14 07:35:07 PST
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
Comment 36 Noam Rosenthal 2012-02-14 23:25:29 PST
Created attachment 127122 [details]
Patch
Comment 37 WebKit Review Bot 2012-02-15 00:26:50 PST
Comment on attachment 127122 [details]
Patch

Clearing flags on attachment: 127122

Committed r107787: <http://trac.webkit.org/changeset/107787>
Comment 38 WebKit Review Bot 2012-02-15 00:26:57 PST
All reviewed patches have been landed.  Closing bug.