[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