Bug 80123 - [BlackBerry] Upstream LayerAnimation.{cpp, h}
Summary: [BlackBerry] Upstream LayerAnimation.{cpp, h}
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73119
  Show dependency treegraph
 
Reported: 2012-03-02 01:08 PST by Robin Cao
Modified: 2012-03-27 19:57 PDT (History)
7 users (show)

See Also:


Attachments
patch (21.62 KB, patch)
2012-03-26 21:35 PDT, Robin Cao
no flags Details | Formatted Diff | Diff
updated-patch (21.42 KB, patch)
2012-03-27 19:37 PDT, Robin Cao
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Cao 2012-03-02 01:08:24 PST
LayerAnimation is a helper class used in accelerated animations.
Comment 1 Robin Cao 2012-03-26 21:35:33 PDT
Created attachment 133971 [details]
patch
Comment 2 Rob Buis 2012-03-27 04:48:58 PDT
Comment on attachment 133971 [details]
patch

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

Looks good, but can still be improved a bit.

> Source/WebCore/platform/graphics/blackberry/LayerAnimation.cpp:112
> +                                        ctf->y2(),

These params can be placed in the same line.

> Source/WebCore/platform/graphics/blackberry/LayerAnimation.cpp:221
> +        // A trick to avoid touching the refcount of shared TransformOperations on the wrong thread

Lacks period at end of sentence.

> Source/WebCore/platform/graphics/blackberry/LayerAnimation.h:24
> +#include "GraphicsLayer.h"

Is this one needed?

> Source/WebCore/platform/graphics/blackberry/LayerAnimation.h:50
> +

Remove empty line.

> Source/WebCore/platform/graphics/blackberry/LayerAnimation.h:58
> +        // This returns a copy of the animation.

This comment does not add much info.
Comment 3 Robin Cao 2012-03-27 19:13:35 PDT
(In reply to comment #2)
> (From update of attachment 133971 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133971&action=review
> 
> Looks good, but can still be improved a bit.
> 
> > Source/WebCore/platform/graphics/blackberry/LayerAnimation.cpp:112
> > +                                        ctf->y2(),
> 
> These params can be placed in the same line.
> 
Agree, looks better in the same line.

> > Source/WebCore/platform/graphics/blackberry/LayerAnimation.cpp:221
> > +        // A trick to avoid touching the refcount of shared TransformOperations on the wrong thread
> 
> Lacks period at end of sentence.
> 
Added.

> > Source/WebCore/platform/graphics/blackberry/LayerAnimation.h:24
> > +#include "GraphicsLayer.h"
> 
> Is this one needed?
> 
This is needed because LayerAnimation has a member variable: KeyframeValueList m_values;
And KeyframeValueList is defined in "GraphicsLayer.h".

> > Source/WebCore/platform/graphics/blackberry/LayerAnimation.h:50
> > +
> 
> Remove empty line.
> 
Fixed.

> > Source/WebCore/platform/graphics/blackberry/LayerAnimation.h:58
> > +        // This returns a copy of the animation.
> 
> This comment does not add much info.
Removed.

Thanks for the review.
Comment 4 Robin Cao 2012-03-27 19:37:46 PDT
Created attachment 134203 [details]
updated-patch
Comment 5 Rob Buis 2012-03-27 19:39:29 PDT
Comment on attachment 134203 [details]
updated-patch

LGTM.
Comment 6 WebKit Review Bot 2012-03-27 19:56:59 PDT
Comment on attachment 134203 [details]
updated-patch

Clearing flags on attachment: 134203

Committed r112358: <http://trac.webkit.org/changeset/112358>
Comment 7 WebKit Review Bot 2012-03-27 19:57:04 PDT
All reviewed patches have been landed.  Closing bug.