Bug 130946 - [UI-side compositing] Proxy animations to the UI process
Summary: [UI-side compositing] Proxy animations to the UI process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-30 13:29 PDT by Simon Fraser (smfr)
Modified: 2014-06-04 00:52 PDT (History)
8 users (show)

See Also:


Attachments
Patch (157.38 KB, patch)
2014-03-30 13:41 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
For Win EWS (158.22 KB, patch)
2014-03-30 22:57 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
For Win EWS (158.23 KB, patch)
2014-03-31 08:57 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
For Win EWS (159.75 KB, patch)
2014-03-31 13:01 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (552.47 KB, application/zip)
2014-03-31 13:56 PDT, Build Bot
no flags Details
For Win EWS (160.89 KB, patch)
2014-03-31 14:03 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2014-03-30 13:29:16 PDT
[UI-side compositing] Proxy animations to the UI process
Comment 1 Simon Fraser (smfr) 2014-03-30 13:41:30 PDT
Created attachment 228130 [details]
Patch
Comment 2 WebKit Commit Bot 2014-03-30 13:43:32 PDT
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.
Comment 3 Tim Horton 2014-03-30 19:06:50 PDT
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?
Comment 4 Simon Fraser (smfr) 2014-03-30 22:57:44 PDT
Created attachment 228151 [details]
For Win EWS
Comment 5 WebKit Commit Bot 2014-03-30 22:58:53 PDT
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.
Comment 6 Simon Fraser (smfr) 2014-03-31 08:57:48 PDT
Created attachment 228182 [details]
For Win EWS
Comment 7 WebKit Commit Bot 2014-03-31 08:59:51 PDT
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.
Comment 8 Brent Fulgham 2014-03-31 11:35:46 PDT
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()
Comment 9 Brent Fulgham 2014-03-31 11:38:03 PDT
This line continues to fail on Windows (32-bit) build:

COMPILE_ASSERT(sizeof(RenderStyle) == sizeof(SameSizeAsRenderStyle), RenderStyle_should_stay_small);
Comment 10 Simon Fraser (smfr) 2014-03-31 13:01:01 PDT
Created attachment 228195 [details]
For Win EWS
Comment 11 WebKit Commit Bot 2014-03-31 13:03:57 PDT
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.
Comment 12 Build Bot 2014-03-31 13:56:25 PDT
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
Comment 13 Build Bot 2014-03-31 13:56:29 PDT
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
Comment 14 Simon Fraser (smfr) 2014-03-31 14:03:07 PDT
Created attachment 228202 [details]
For Win EWS
Comment 15 WebKit Commit Bot 2014-03-31 14:05:52 PDT
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.
Comment 16 Simon Fraser (smfr) 2014-03-31 16:17:08 PDT
https://trac.webkit.org/r166542
Comment 17 Csaba Osztrogonác 2014-06-04 00:52:37 PDT
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).