[Webkit-unassigned] [Bug 119938] Animate CSS Image filter() function
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 19 11:18:04 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=119938
Simon Fraser (smfr) <simon.fraser at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #209079|review? |review-
Flag| |
--- Comment #14 from Simon Fraser (smfr) <simon.fraser at apple.com> 2013-08-19 11:17:32 PST ---
(From update of attachment 209079)
View in context: https://bugs.webkit.org/attachment.cgi?id=209079&action=review
Looks OK but I'd like to see a patch with the enum and other comments addressed.
> Source/WebCore/css/CSSComputedStyleDeclaration.h:65
> + static PassRefPtr<CSSValue> valueForFilter(const RenderObject*, const RenderStyle*, const FilterOperations&, bool adjust = true);
It's not clear what "adjust" means in this context. You should use an enum.
> Source/WebCore/css/CSSComputedStyleDeclaration.h:80
> - PassRefPtr<CSSValue> valueForShadow(const ShadowData*, CSSPropertyID, const RenderStyle*) const;
> + static PassRefPtr<CSSValue> valueForShadow(const ShadowData*, CSSPropertyID, const RenderStyle*, bool adjust = true);
It's not clear what "adjust" means in this context. You should use an enum.
> Source/WebCore/css/CSSFilterImageValue.h:73
> + bool equalSubimages(const CSSFilterImageValue&) const;
subimage is confusing; we should rename this ("input image"?)
> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:225
> + blendFilterOperations(
> + anim,
> + filterOperationsResult,
> + fromValue->filterOperations(),
> + toValue->filterOperations(),
> + progress);
This doesn't need to be so aggressively wrapped.
> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:236
> + RefPtr<CSSValue> filterValue = ComputedStyleExtractor::valueForFilter(
> + anim->renderer(),
> + anim->renderer()->style(),
> + filterOperationsResult,
> + false);
Ditto.
Congratulations, you just fell into the boolean trap.
> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:323
> + ASSERT(fromGenerated);
> + ASSERT(toGenerated);
Not sure these assertions are useful, since (if filters are enabled) you'll crash on the next line anyway.
> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:327
> + // Animation of generated images just possible if subimages are equal.
s/just/is only
I don't really know what a subimage is in this context. It seems different from our use of the term in other places.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list