Summary: | Animate CSS Image filter() function | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, dino, dstockwell, eflews.bot, esprehn+autocc, glenn, gtk-ews, gyuyoung.kim, koivisto, kondapallykalyan, macpherson, menard, rego+ews, rniwa, simon.fraser, webkit-ews, xan.lopez | ||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||
Bug Blocks: | 120603 | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Dirk Schulze
2013-08-16 23:38:46 PDT
Created attachment 208984 [details]
Patch
Attachment 208984 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/animations/resources/animation-test-helpers.js', u'LayoutTests/fast/filter-image/filter-image-animation-expected.txt', u'LayoutTests/fast/filter-image/filter-image-animation.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.h', u'Source/WebCore/css/CSSFilterImageValue.h', u'Source/WebCore/page/animation/CSSPropertyAnimation.cpp']" exit_code: 1
Source/WebCore/page/animation/CSSPropertyAnimation.cpp:221: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 208985 [details]
Patch
Created attachment 209007 [details]
Patch
Prepare CSSPropertyAnimation for future generated image blend operations.
Comment on attachment 209007 [details] Patch Attachment 209007 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1497206 Comment on attachment 209007 [details] Patch Attachment 209007 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1387361 Comment on attachment 209007 [details] Patch Attachment 209007 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1433645 Comment on attachment 209007 [details] Patch Attachment 209007 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1433646 Comment on attachment 209007 [details] Patch Attachment 209007 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1387364 Created attachment 209013 [details]
Patch
patch with build fix
Created attachment 209079 [details]
Patch
Add toCSSFilterImageValue functions as suggested by anttik.
Comment on attachment 209079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209079&action=review > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:217 > + CSSFilterImageValue* fromValue = static_cast<CSSFilterImageValue*>(from->data()); > + CSSFilterImageValue* toValue = static_cast<CSSFilterImageValue*>(to->data()); You should use you casting functions here too. > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:321 > + CSSImageGeneratorValue* fromGenerated = static_cast<CSSImageGeneratorValue*>(from->data()); > + CSSImageGeneratorValue* toGenerated = static_cast<CSSImageGeneratorValue*>(to->data()); The could have casting functions. Not my best typing day. Comment on attachment 209079 [details] Patch 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. Created attachment 209116 [details]
Patch
Addresses anttik's and smfr's feedback.
Comment on attachment 209116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209116&action=review > LayoutTests/fast/filter-image/filter-image-animation.html:58 > + @-webkit-keyframes no-anim { > + from { background-image: -webkit-filter(url(resources/image.svg), sepia(1)); } > + to { background-image: -webkit-filter(url(resources/blue.svg), sepia(0)); } > + } You should test a blend between a filter and simple image, and a filter and gradient, and filter and crossfade (to check for code that behaves badly). You should also test for blends between filter lists of the same and different lengths, and maybe with missing filter lists. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:903 > +static inline PassRefPtr<CSSPrimitiveValue> valueForPixel(double length, const RenderStyle* style, ZoomAdjustedPixel adjust) This has a very confusing name; it sounds like you're lookup up the RGBA for a single pixel. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:908 > +static inline PassRefPtr<CSSPrimitiveValue> valueForPixel(const Length& length, const RenderStyle* style, ZoomAdjustedPixel adjust) Ditto. > Source/WebCore/css/CSSComputedStyleDeclaration.h:51 > +enum ZoomAdjustedPixel { AdjustPixel, DoNotAdjustPixel }; Enum types should be descriptive, like AdjustPixelVaulesForZoom { AdjustPixelValuesForZoom, DoNotAdjustPixelValues } > Source/WebCore/css/CSSFilterImageValue.cpp:168 > +bool CSSFilterImageValue::equalInputImages(const CSSFilterImageValue& other) const Much better than subimage! > Source/WebCore/css/CSSFilterImageValue.h:49 > + Blank line. Created attachment 209953 [details]
Patch
Comment on attachment 209953 [details] Patch Attachment 209953 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1640119 New failing tests: animations/cross-fade-webkit-mask-box-image.html animations/cross-fade-webkit-mask-image.html animations/cross-fade-background-image.html animations/cross-fade-border-image-source.html animations/cross-fade-list-style-image.html Created attachment 209962 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Comment on attachment 209953 [details] Patch Attachment 209953 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1627569 New failing tests: animations/cross-fade-webkit-mask-box-image.html animations/cross-fade-webkit-mask-image.html animations/cross-fade-background-image.html animations/cross-fade-border-image-source.html animations/cross-fade-list-style-image.html Created attachment 209967 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
Created attachment 209973 [details]
Patch
Comment on attachment 209973 [details] Patch Attachment 209973 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/1640183 New failing tests: animations/cross-fade-border-image-source.html Created attachment 209978 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.4
Comment on attachment 209973 [details] Patch Attachment 209973 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/1605646 New failing tests: animations/cross-fade-border-image-source.html Created attachment 209994 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.4
Comment on attachment 209973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209973&action=review You should add a test for animations between -webkit-filter(url(foo), ...) and url(foo), which should animate the filter values to the no-op filter (just like the filter property). > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:903 > +static inline PassRefPtr<CSSPrimitiveValue> adjustLengthForZoom(double length, const RenderStyle* style, AdjustPixelVaulesForZoom adjust) AdjustPixelVaulesForZoom is mis-spelled. It's also weird to have the same name for the enum as one of its values. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:919 > + for (const ShadowData* s = shadow; s; s = s->next()) { We don't use short variable names like 's'. Maybe currShadowData? > Source/WebCore/css/CSSComputedStyleDeclaration.h:51 > +enum AdjustPixelVaulesForZoom { AdjustPixelValuesForZoom, DoNotAdjustPixelValues }; Spelling. > Source/WebCore/page/animation/CSSPropertyAnimation.cpp:221 > + blendFilterOperations(anim, filterOperationsResult, > + fromValue->filterOperations(), toValue->filterOperations(), progress); I don't think you need to wrap these lines. Created attachment 210016 [details]
Patch
Created attachment 210034 [details]
Patch
Created attachment 210035 [details]
Patch
Comment on attachment 210035 [details] Patch Clearing flags on attachment: 210035 Committed r154906: <http://trac.webkit.org/changeset/154906> All reviewed patches have been landed. Closing bug. |