Bug 84978 – Move PropertyWrapper out of the AnimationBase

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Igor Trindade Oliveira]

Removing PropertyWrapper makes the AnimationBase simpler.

[Igor Trindade Oliveira]

Created attachment 139034 [details]
Patch

Proposed patch.

[Igor Trindade Oliveira]

With this change will be easier to make gPropertyWrappers ref counted. It is the next step.

[Simon Fraser (smfr)]

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

[Igor Trindade Oliveira]

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?

[Simon Fraser (smfr)]

Why does it have to be deletable?

[Igor Trindade Oliveira]

Created attachment 139039 [details]
Patch

Updated patch

[Igor Trindade Oliveira]

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?

[Igor Trindade Oliveira]

Created attachment 139042 [details]
Patch

Proposed patch.

[Early Warning System Bot]

Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12547004

[Build Bot]

Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12523955

[Gustavo Noronha (kov)]

Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12528956

[Igor Trindade Oliveira]

Comment on attachment 139042 [details]
Patch

Clear flags. A new patch is being cooked.

[Build Bot]

Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12490898

[Early Warning System Bot]

Comment on attachment 139042 [details]
Patch

Attachment 139042 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12528958

[Simon Fraser (smfr)]

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.

[Igor Trindade Oliveira]

Created attachment 139108 [details]
Patch

Updated patch.

[Igor Trindade Oliveira]

Created attachment 139112 [details]
Patch

Updated patch. Fix changelog.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Igor Trindade Oliveira]

Created attachment 139272 [details]
Patch

Fix crash in the previous patch.

[Igor Trindade Oliveira]

Created attachment 139277 [details]
Patch

Updated patch.

[Dean Jackson]

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

[Igor Trindade Oliveira]

Created attachment 139366 [details]
Patch

final patch.

[WebKit Review Bot]

Comment on attachment 139366 [details]
Patch

Clearing flags on attachment: 139366

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