Bug 84978 - Move PropertyWrapper out of the AnimationBase
Summary: Move PropertyWrapper out of the AnimationBase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Igor Trindade Oliveira
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-26 11:33 PDT by Igor Trindade Oliveira
Modified: 2012-04-30 17:32 PDT (History)
7 users (show)

See Also:


Attachments
Patch (146.41 KB, patch)
2012-04-26 11:46 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (147.09 KB, patch)
2012-04-26 12:15 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (150.23 KB, patch)
2012-04-26 12:22 PDT, Igor Trindade Oliveira
simon.fraser: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (149.83 KB, patch)
2012-04-26 17:48 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (149.85 KB, patch)
2012-04-26 17:57 PDT, Igor Trindade Oliveira
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.06 MB, application/zip)
2012-04-27 12:56 PDT, WebKit Review Bot
no flags Details
Patch (149.77 KB, patch)
2012-04-27 14:10 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff
Patch (149.79 KB, patch)
2012-04-27 14:21 PDT, Igor Trindade Oliveira
dino: review+
Details | Formatted Diff | Diff
Patch (155.86 KB, patch)
2012-04-28 11:03 PDT, Igor Trindade Oliveira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Igor Trindade Oliveira 2012-04-26 11:33:38 PDT
Removing PropertyWrapper makes the AnimationBase simpler.
Comment 1 Igor Trindade Oliveira 2012-04-26 11:46:22 PDT
Created attachment 139034 [details]
Patch

Proposed patch.
Comment 2 Igor Trindade Oliveira 2012-04-26 11:47:33 PDT
With this change will be easier to make gPropertyWrappers ref counted. It is the next step.
Comment 3 Simon Fraser (smfr) 2012-04-26 11:48:45 PDT
(In reply to comment #2)
> With this change will be easier to make gPropertyWrappers ref counted. It is the next step.

Why do you want to do this?
Comment 4 Igor Trindade Oliveira 2012-04-26 11:54:01 PDT
Right now gPropertyWrappers is not deleted, we need to find a way to delete it. Instead of be ref counted we can just create a destroy method in the AnimationPropertyHandler and call it when ~AnimationController is called.

(In reply to comment #3)
> (In reply to comment #2)
> > With this change will be easier to make gPropertyWrappers ref counted. It is the next step.
> 
> Why do you want to do this?
Comment 5 Simon Fraser (smfr) 2012-04-26 11:59:06 PDT
Why does it have to be deletable?
Comment 6 Igor Trindade Oliveira 2012-04-26 12:15:55 PDT
Created attachment 139039 [details]
Patch

Updated patch
Comment 7 Igor Trindade Oliveira 2012-04-26 12:17:38 PDT
As discussed in the IRC, does not make sense we make gPropertyWrappers deletable. However, this patch fixes the layer violation about AnimationController accessing AnimationBase property handlers and make AnimationBase simpler.
(In reply to comment #5)
> Why does it have to be deletable?
Comment 8 Igor Trindade Oliveira 2012-04-26 12:22:30 PDT
Created attachment 139042 [details]
Patch

Proposed patch.
Comment 9 Early Warning System Bot 2012-04-26 12:37:36 PDT
Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12547004
Comment 10 Build Bot 2012-04-26 12:43:05 PDT
Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12523955
Comment 11 Gustavo Noronha (kov) 2012-04-26 12:44:10 PDT
Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12528956
Comment 12 Igor Trindade Oliveira 2012-04-26 12:45:22 PDT
Comment on attachment 139042 [details]
Patch

Clear flags. A new patch is being cooked.
Comment 13 Build Bot 2012-04-26 12:50:12 PDT
Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12490898
Comment 14 Early Warning System Bot 2012-04-26 12:59:10 PDT
Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12528958
Comment 15 Simon Fraser (smfr) 2012-04-26 13:02:20 PDT
Comment on attachment 139042 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        of property handlers. This patch remove the property handlers for a
> +        separated class making AnimationBase simpler.

I'd use the term "move" rather than "remove".

> Source/WebCore/page/animation/AnimationPropertyHandler.h:41
> +class AnimationPropertyHandler {

I think PropertyAnimation would be a better name for this class, or maybe even CSSPropertyAnimation.
Comment 16 Igor Trindade Oliveira 2012-04-26 17:48:51 PDT
Created attachment 139108 [details]
Patch

Updated patch.
Comment 17 Igor Trindade Oliveira 2012-04-26 17:57:48 PDT
Created attachment 139112 [details]
Patch

Updated patch. Fix changelog.
Comment 18 WebKit Review Bot 2012-04-27 12:55:56 PDT
Comment on attachment 139112 [details]
Patch

Attachment 139112 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12551293

New failing tests:
compositing/overflow-trumps-transform-style.html
transforms/3d/general/3dtransform-values.html
Comment 19 WebKit Review Bot 2012-04-27 12:56:02 PDT
Created attachment 139251 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 20 Igor Trindade Oliveira 2012-04-27 14:10:04 PDT
Created attachment 139272 [details]
Patch

Fix crash in the previous patch.
Comment 21 Igor Trindade Oliveira 2012-04-27 14:21:36 PDT
Created attachment 139277 [details]
Patch

Updated patch.
Comment 22 Dean Jackson 2012-04-27 15:32:25 PDT
Comment on attachment 139277 [details]
Patch

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

As long is this is just moving code to CSSPropertyAnimation, then I'm ok with it.

> Source/WebCore/ChangeLog:10
> +        property handlers. This patch moves the property handlers for a separated
> +        class making AnimationBase simpler.

handlers *to a separate* class
Comment 23 Igor Trindade Oliveira 2012-04-28 11:03:17 PDT
Created attachment 139366 [details]
Patch

final patch.
Comment 24 WebKit Review Bot 2012-04-28 13:58:27 PDT
Comment on attachment 139366 [details]
Patch

Clearing flags on attachment: 139366

Committed r115581: <http://trac.webkit.org/changeset/115581>