Bug 80123

Summary: [BlackBerry] Upstream LayerAnimation.{cpp, h}
Product: WebKit Reporter: Robin Cao <robin.webkit>
Component: WebKit BlackBerryAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: arvid2.nilsson, charles.wei, leo.yang, levin+threading, rwlbuis, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Bug Depends on:    
Bug Blocks: 73119    
Attachments:
Description Flags
patch
none
updated-patch none

Robin Cao
Reported 2012-03-02 01:08:24 PST
LayerAnimation is a helper class used in accelerated animations.
Attachments
patch (21.62 KB, patch)
2012-03-26 21:35 PDT, Robin Cao 		no flags
DetailsFormatted DiffDiff
updated-patch (21.42 KB, patch)
2012-03-27 19:37 PDT, Robin Cao 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Robin Cao
Comment 1 2012-03-26 21:35:33 PDT
Created attachment 133971 [details] patch
Rob Buis
Comment 2 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.
Robin Cao
Comment 3 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.
Robin Cao
Comment 4 2012-03-27 19:37:46 PDT
Created attachment 134203 [details] updated-patch
Rob Buis
Comment 5 2012-03-27 19:39:29 PDT
Comment on attachment 134203 [details] updated-patch LGTM.
WebKit Review Bot
Comment 6 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>
WebKit Review Bot
Comment 7 2012-03-27 19:57:04 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.