Bug 130946 – [UI-side compositing] Proxy animations to the UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

[UI-side compositing] Proxy animations to the UI process

[Simon Fraser (smfr)]

Created attachment 228130 [details]
Patch

[WebKit Commit Bot]

Attachment 228130 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/animation/TimingFunction.h:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:176:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:224:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tim Horton]

Comment on attachment 228130 [details]
Patch

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

> Source/WebCore/ChangeLog:48
> +        * platform/graphics/ca/win/PlatformCAAnimationWin.cpp:
> +        * platform/graphics/ca/win/PlatformCAAnimationWin.h: Added.

Not added to the project file?

> Source/WebKit2/ChangeLog:16
> +        Add support the "animationDidStart" callback and sending this back to the

add support the?

> Source/WebKit2/ChangeLog:24
> +        and that the RemoteLayerTreeHost have a reference to the drawing area.

This is slightly unfortunate.

> Source/WebCore/platform/animation/TimingFunction.h:193
> +    static PassRefPtr<StepsTimingFunction> create()

newline above.

> Source/WebCore/platform/animation/TimingFunction.h:213
> +    void setStepAtStart(bool stepsAtStart) { m_stepAtStart = stepsAtStart; }

step or steps?

> Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.h:30
> +#include <wtf/RetainPtr.h>

newline above

> Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.h:34
> +OBJC_CLASS CAPropertyAnimation;
> +OBJC_CLASS NSString;
> +OBJC_CLASS CAMediaTimingFunction;

sort

> Source/WebCore/platform/graphics/ca/mac/PlatformCAAnimationMac.mm:2
> - * Copyright (C) 2010 Apple Inc. All rights reserved.
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

...

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.h:32
> +#include <wtf/RetainPtr.h>

newline above

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.h:42
> +//    static PassRefPtr<PlatformCAAnimation> create(PlatformAnimationRef);

??? (commented out)

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.h:48
> +    virtual PassRefPtr<PlatformCAAnimation> copy() const override;

in some of the other classes (the timing functions), this was called clone. here, copy. should we pick one?

> Source/WebKit2/Platform/IPC/ArgumentCoders.h:203
> +        for (typename HashSetType::const_iterator it = hashSet.begin(), end = hashSet.end(); it != end; ++it)

isn't this a good candidate for range-based for?

> Source/WebKit2/WebProcess/WebPage/mac/GraphicsLayerCARemote.cpp:61
> +// FIXME: reject filter animations

if we don't support them yet, shouldn't we reject them now? :D

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:62
> +    if ((self = [super init])) {

I think we tend to use an early return here but I admit it doesn't gain us much.

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:159
> +

no need for this newline

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:162
> +            encoder << *static_cast<LinearTimingFunction*>(timingFunction.get());

I think encoder << static_cast<LinearTimingFunction>(*timingFunction); might have the same effect with less wackiness?

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:243
> +                if (!decoder.decode(*static_cast<LinearTimingFunction*>(timingFunction.get())))

ditto

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:267
> +#pragma mark -

?

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:306
> +        m_properties.hasNonZeroBeginTime = value;

Is this a "had", then, according to the comment?

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:615
> +            [NSNumber numberWithDouble:color.red()],

@[ @(color.red()), @(color.green()), @(color.blue()), @(color.alpha()) ];?

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCAAnimationRemote.mm:625
> +            [NSNumber numberWithDouble:point.x()],

ditto

> Source/WebKit2/WebProcess/WebPage/mac/PlatformCALayerRemote.cpp:281
> +    // FIXME: remove from m_properties.addedAnimations ?

probably?

[Simon Fraser (smfr)]

Created attachment 228151 [details]
For Win EWS

[WebKit Commit Bot]

Attachment 228151 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/animation/TimingFunction.h:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:176:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:224:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Created attachment 228182 [details]
For Win EWS

[WebKit Commit Bot]

Attachment 228182 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/animation/TimingFunction.h:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:176:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:224:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Comment on attachment 228182 [details]
For Win EWS

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

I also needed to:
1. Add a `#include "GraphicsContext.h"' to PlatformCALayerWinInternal.cpp. I guess the header includes are a little different now.
2. Continue to have a friend relationship between PlatformCAAnimation and PlatformCALayerWin, so that PlatformCALayerWin could call "setActualStartTimeIfNeeded" from animationStarted.

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:147
>      return adoptRef(new PlatformCAAnimation(type, keyPath));

return adoptRef(new PlatformCAAnimationWin(type, keyPath));
                                       ^^^

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:152
>      return adoptRef(new PlatformCAAnimation(animation));

return adoptRef(new PlatformCAAnimationWin(type, keyPath));
                                       ^^^

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:156
>      : m_type(type)

: PlatformCAAnimation(type)

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:169
>          m_type = Basic;

This needs to be setType(Basic); (and later, setType(Keyframe);)

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:302
>      CACFTimingFunctionRef timingFunc = CACFAnimationGetTimingFunction(value->m_animation.get());

Need to use "toPlatformCAAnimationWin(value)->m_animation.get()"

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:389
>      CACFAnimationSetFromValue(m_animation.get(), CACFAnimationGetFromValue(value->platformAnimation()));

toPlatformCAAnimationWin(value)->platformAnimation()

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:440
>          return;

Need to use toPlatformCAAnimationWin(value)->platformAnimation() for next line.

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:515
>          return;

toPlatformCAAnimationWin on next line.

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:537
>          return;

toPlatformCAAnimationWin on next line.

> Source/WebCore/platform/graphics/ca/win/PlatformCAAnimationWin.cpp:559
>      CACFAnimationSetTimingFunctions(m_animation.get(), CACFAnimationGetTimingFunctions(value->platformAnimation()));

Need to use toPlatformCAAnimationWin(value)->platformAnimation()

[Brent Fulgham]

This line continues to fail on Windows (32-bit) build:

COMPILE_ASSERT(sizeof(RenderStyle) == sizeof(SameSizeAsRenderStyle), RenderStyle_should_stay_small);

[Simon Fraser (smfr)]

Created attachment 228195 [details]
For Win EWS

[WebKit Commit Bot]

Attachment 228195 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/animation/TimingFunction.h:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:176:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:224:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 228195 [details]
For Win EWS

Attachment 228195 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5233147704770560

New failing tests:
media/W3C/audio/canPlayType/canPlayType_application_octet_stream_with_codecs_1.html

[Build Bot]

Created attachment 228200 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

Created attachment 228202 [details]
For Win EWS

[WebKit Commit Bot]

Attachment 228202 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/animation/TimingFunction.h:81:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:176:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/animation/TimingFunction.h:224:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

https://trac.webkit.org/r166542

[Csaba Osztrogonác]

Comment on attachment 228202 [details]
For Win EWS

Cleared review? from attachment 228202 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).