[webkit-reviews] review granted: [Bug 130946] [UI-side compositing] Proxy animations to the UI process : [Attachment 228130] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Mar 30 19:06:29 PDT 2014
Tim Horton <thorton at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 130946: [UI-side compositing] Proxy animations to the UI process
https://bugs.webkit.org/show_bug.cgi?id=130946
Attachment 228130: Patch
https://bugs.webkit.org/attachment.cgi?id=228130&action=review
------- Additional Comments from Tim Horton <thorton at apple.com>
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?
More information about the webkit-reviews
mailing list