Bug 52162 - Implement CSS3 Images cross-fade() image function
Summary: Implement CSS3 Images cross-fade() image function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on: 72639
Blocks: 46590
  Show dependency treegraph
 
Reported: 2011-01-10 11:49 PST by Simon Fraser (smfr)
Modified: 2012-01-13 16:32 PST (History)
17 users (show)

See Also:


Attachments
parse -webkit-cross-fade (29.64 KB, patch)
2011-10-28 01:33 PDT, Tim Horton
simon.fraser: review-
Details | Formatted Diff | Diff
address changes (30.32 KB, patch)
2011-10-28 15:56 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff
[preliminary] render cross-fade (34.69 KB, patch)
2011-11-03 10:59 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
[preliminary] render cross-fade - should apply now (34.30 KB, patch)
2011-11-04 10:40 PDT, Tim Horton
gustavo: commit-queue-
Details | Formatted Diff | Diff
[preliminary] render cross-fade (57.53 KB, patch)
2011-11-10 18:05 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Render Cross-fade (87.35 KB, patch)
2011-11-14 12:23 PST, Tim Horton
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Render Cross-Fade (plus build system changes for other ports) (91.99 KB, patch)
2011-11-14 12:37 PST, Tim Horton
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Render Cross-Fade (plus build fix for non-Mac) (92.00 KB, patch)
2011-11-14 13:43 PST, Tim Horton
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Render Cross-Fade (plus build fix for non-debug) (92.03 KB, patch)
2011-11-14 13:56 PST, Tim Horton
simon.fraser: review-
Details | Formatted Diff | Diff
Render Cross-Fade (92.20 KB, patch)
2011-11-15 16:32 PST, Tim Horton
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Render Cross-Fade (and allow fractional cross-fade amounts, and clamp them to 0-1) (103.42 KB, patch)
2011-11-15 20:15 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Render Cross-Fade (Windows build fix, hopefully) (104.17 KB, patch)
2011-11-15 20:22 PST, Tim Horton
simon.fraser: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fix the getComputedStyle test to use dummy:// (22.75 KB, patch)
2011-11-17 12:28 PST, Tim Horton
no flags Details | Formatted Diff | Diff
Fix layering violation (17.61 KB, patch)
2011-11-17 14:15 PST, Tim Horton
thorton: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
fix layering violation, take 2 (18.74 KB, patch)
2011-11-30 12:39 PST, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-01-10 11:49:08 PST
We should implement http://dev.w3.org/csswg/css3-images/#cross-fade-function so that we can enable transitions for background-image.
Comment 1 Simon Fraser (smfr) 2011-08-29 16:39:35 PDT
<rdar://problem/10042838>
Comment 2 Tim Horton 2011-10-28 01:33:11 PDT
Created attachment 112832 [details]
parse -webkit-cross-fade
Comment 3 mitz 2011-10-28 07:40:48 PDT
Comment on attachment 112832 [details]
parse -webkit-cross-fade

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

> Source/WebCore/css/CSSCrossfadeValue.cpp:29
> +#include "RenderObject.h"

It looks like a forward declaration of RenderObject would suffice here.

> Source/WebCore/css/CSSCrossfadeValue.cpp:35
> +

No need for this empty line.

> Source/WebCore/css/CSSCrossfadeValue.cpp:41
> +    // TODO: what do we show if these haven't been set yet!?

WebKit style is to use “FIXME” in comments, not “TODO”.

> Source/WebCore/css/CSSCrossfadeValue.cpp:61
> +    UNUSED_PARAM(size);
> +    
> +    if (size.isEmpty())

size is not unused after all…

> Source/WebCore/css/CSSCrossfadeValue.cpp:64
> +    return 0;

But maybe it should be?

> Source/WebCore/css/CSSCrossfadeValue.h:46
> +    virtual String cssText() const;
> +
> +    virtual PassRefPtr<Image> image(RenderObject*, const IntSize&);
> +    virtual bool isFixedSize() const { return false; }
> +    virtual IntSize fixedSize(const RenderObject*);

These should can all be marked OVERRIDE.

> Source/WebCore/css/CSSParser.h:94
> +    bool parseFillImage(CSSParserValueList *, RefPtr<CSSValue>&);

Extra space before the star.
Comment 4 Simon Fraser (smfr) 2011-10-28 10:58:43 PDT
Comment on attachment 112832 [details]
parse -webkit-cross-fade

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

Also, patch doesn't apply.

> LayoutTests/fast/css/getComputedStyle/computed-style-cross-fade-expected.txt:16
> +PASS testCrossfade("background-image: -webkit-cross-fade(20%, url(http://example.com/a.png), url(http://example.com/b.png))", "background-image") is "none"
> +PASS successfullyParsed is true

You should test
-webkit-cross-fade(url(..),)
-webkit-cross-fade(,)
-webkit-cross-fade(,url(...)) as well.

> Source/WebCore/css/CSSCrossfadeValue.h:50
> +    void setFromImage(const PassRefPtr<CSSImageValue> fromImage) { m_fromImage = fromImage; }
> +    void setToImage(const PassRefPtr<CSSImageValue> toImage) { m_toImage = toImage; }
> +    void setPercentage(const PassRefPtr<CSSPrimitiveValue> percentage) { m_percentage = percentage; }

No need for the consts.

Perhaps you should just pass the two CSSImageValues into the create() method and ctor?

> Source/WebCore/css/CSSParser.cpp:2791
> +bool CSSParser::parseFillImage(CSSParserValueList * valueList, RefPtr<CSSValue>& value)

No space before the * for any of these.

> Source/WebCore/css/CSSParser.cpp:6195
> +    RefPtr<CSSCrossfadeValue> result = CSSCrossfadeValue::create();

Might as well delay this until you actually need it.

> Source/WebCore/css/CSSParser.cpp:6229
> +    if (!a || a->unit != CSSPrimitiveValue::CSS_PERCENTAGE)
> +        return false;

Should we allow fractions here ("0.4")?
Comment 5 Tim Horton 2011-10-28 15:56:28 PDT
Created attachment 112928 [details]
address changes

The only thing (as far as I can see) that this patch doesn't address is Simon's last question about fractions. To which I say: I don't know! Should we? The spec says "<percentage>" but I don't know what that actually means.
Comment 6 Tim Horton 2011-10-28 16:12:35 PDT
Parsing part landed in r98775.
Comment 7 Ryosuke Niwa 2011-10-28 23:43:15 PDT
Fixed the expected result in http://trac.webkit.org/changeset/98797.
Comment 8 Tim Horton 2011-11-03 10:59:39 PDT
Created attachment 113523 [details]
[preliminary] render cross-fade

Concerns:

1. loader/cache/CachedResourceClient.h -- multiple inheritance of two things which inherit from CachedResourceClient seems not to work. I'll need some assistance there from someone who knows the finer points of C++.
Comment 9 Tim Horton 2011-11-04 10:40:37 PDT
Created attachment 113676 [details]
[preliminary] render cross-fade - should apply now
Comment 10 WebKit Review Bot 2011-11-04 10:43:45 PDT
Attachment 113676 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.xcodeproj/project.p..." exit_code: 1

Source/WebCore/css/CSSCrossfadeValue.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/CSSCrossfadeValue.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:63:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:67:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/GeneratorGeneratedImage.h:44:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/css/CSSCrossfadeValue.h:57:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/css/CSSCrossfadeValue.h:58:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/css/CSSCrossfadeValue.h:59:  Missing space inside { }.  [whitespace/braces] [5]
Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:53:  The parameter name "observer" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:53:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 14 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Gustavo Noronha (kov) 2011-11-04 10:53:57 PDT
Comment on attachment 113676 [details]
[preliminary] render cross-fade - should apply now

Attachment 113676 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10336017
Comment 12 WebKit Review Bot 2011-11-04 10:54:31 PDT
Comment on attachment 113676 [details]
[preliminary] render cross-fade - should apply now

Attachment 113676 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10333020
Comment 13 Early Warning System Bot 2011-11-04 10:56:48 PDT
Comment on attachment 113676 [details]
[preliminary] render cross-fade - should apply now

Attachment 113676 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10333022
Comment 14 Tim Horton 2011-11-04 10:57:25 PDT
Oh, the other build systems again. Someday I'll remember that without being reminded.
Comment 15 Gyuyoung Kim 2011-11-04 11:05:09 PDT
Comment on attachment 113676 [details]
[preliminary] render cross-fade - should apply now

Attachment 113676 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10334021
Comment 16 Tim Horton 2011-11-10 18:05:30 PST
Created attachment 114611 [details]
[preliminary] render cross-fade

I was hoping that this would be in a commit-able state today, but then I got to writing tests. For some reason, the images don't render in DRT, even though I followed the code through the debugger and can see that the images are valid and the rendering code is being called. This will require further investigation...
Comment 17 Simon Fraser (smfr) 2011-11-11 10:18:19 PST
Do you need to wait for the image load events before testing?
Comment 18 Tim Horton 2011-11-11 12:36:18 PST
(In reply to comment #17)
> Do you need to wait for the image load events before testing?

Maybe? I tried data-urls too, which other people seem to use without waiting for any sort of load events, and they don't help any. (I'd also somewhat expect DRT to wait for images to finish loading before spitting out an image, but maybe not).

The ones that use -webkit-canvas inputs do work, though, so it's definitely related to image loading.

Making gratuitous use of layoutTestController.display() doesn't help like I expected it to, either (not that I was going to keep it that way, just for trying to see *something*).
Comment 19 Tim Horton 2011-11-14 12:23:58 PST
Created attachment 115002 [details]
Render Cross-fade

Have at it! I have a feeling this is going to need a good bit of review.
Comment 20 Early Warning System Bot 2011-11-14 12:35:24 PST
Comment on attachment 115002 [details]
Render Cross-fade

Attachment 115002 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10484022
Comment 21 Tim Horton 2011-11-14 12:37:00 PST
Created attachment 115011 [details]
Render Cross-Fade (plus build system changes for other ports)
Comment 22 Early Warning System Bot 2011-11-14 12:47:51 PST
Comment on attachment 115011 [details]
Render Cross-Fade (plus build system changes for other ports)

Attachment 115011 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10487026
Comment 23 Gyuyoung Kim 2011-11-14 13:39:00 PST
Comment on attachment 115011 [details]
Render Cross-Fade (plus build system changes for other ports)

Attachment 115011 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10485041
Comment 24 Gustavo Noronha (kov) 2011-11-14 13:41:19 PST
Comment on attachment 115011 [details]
Render Cross-Fade (plus build system changes for other ports)

Attachment 115011 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10487042
Comment 25 Tim Horton 2011-11-14 13:43:36 PST
Created attachment 115025 [details]
Render Cross-Fade (plus build fix for non-Mac)
Comment 26 Early Warning System Bot 2011-11-14 13:51:54 PST
Comment on attachment 115025 [details]
Render Cross-Fade (plus build fix for non-Mac)

Attachment 115025 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10372237
Comment 27 Tim Horton 2011-11-14 13:56:06 PST
Created attachment 115027 [details]
Render Cross-Fade (plus build fix for non-debug)

Perhaps I should get out my VMs.
Comment 28 Simon Fraser (smfr) 2011-11-14 14:25:24 PST
Comment on attachment 115027 [details]
Render Cross-Fade (plus build fix for non-debug)

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

This is a good start! I'd like to see a test case with a tiled cross-fade image.

> LayoutTests/css3/images/cross-fade-invalidation.html:16
> +    if(window.layoutTestController)
> +	layoutTestController.notifyDone();

Need to indent there

> Source/WebCore/css/CSSCrossfadeValue.cpp:71
> +    if (value->isImageValue())
> +        return static_cast<CSSImageValue*>(value.get())->cachedOrPendingImage()->isPendingImage();
> +
> +    if (value->isImageGeneratorValue())
> +        return static_cast<CSSImageGeneratorValue*>(value.get())->isPending();

These two bare functions look like candidates for a virtual method.

> Source/WebCore/css/CSSCrossfadeValue.cpp:117
> +    CachedImage* fromImage, * toImage;

We don't declare multiple variables on one line like this.

> Source/WebCore/css/CSSCrossfadeValue.cpp:120
> +    fromImage = cachedImageForCSSValue(m_fromImage, renderer, size);
> +    toImage = cachedImageForCSSValue(m_toImage, renderer, size);

Initialize at declaration!

> Source/WebCore/css/CSSCrossfadeValue.h:42
> +    static PassRefPtr<CSSCrossfadeValue> create(PassRefPtr<CSSValue> fromImage, PassRefPtr<CSSValue> toImage) { return adoptRef(new CSSCrossfadeValue(fromImage, toImage)); }

Break this onto 3 lines.

> Source/WebCore/css/CSSCrossfadeValue.h:43
> +    ~CSSCrossfadeValue() { }

You can remove this.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:117
> +    if (isPending())
> +        m_image = StylePendingImage::create(this).get();

It seems odd that calling this method has the side-effect of changing m_image.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:198
> +    case CrossfadeClass:
> +        return static_cast<const CSSCrossfadeValue*>(this)->isPending();
> +    case CanvasClass:
> +        return static_cast<const CSSCanvasValue*>(this)->isPending();
> +    case LinearGradientClass:
> +        return static_cast<const CSSLinearGradientValue*>(this)->isPending();
> +    case RadialGradientClass:
> +        return static_cast<const CSSRadialGradientValue*>(this)->isPending();
> +    default:

Erm, why not use virtual methods?

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:45
> +    m_size = size;

Does the superclass not have a ctor that sets the size?

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:64
> +    float inversePercentage = 1.0f - m_percentage;

Use 1, not 1.0f

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:69
> +    fromImage = m_fromImage->image();
> +    fromImageSize = fromImage->size();
> +    toImage = m_toImage->image();
> +    toImageSize = toImage->size();

Declare at first use. This is C++, not C :)

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:76
> +    crossfadeImageSize = IntSize(fromImageSize.width() * inversePercentage + toImageSize.width() * m_percentage,
> +                                 fromImageSize.height() * inversePercentage + toImageSize.height() * m_percentage);

Does the spec say anything about interpolating sizes? I'm not sure it's correct to animate the size too.

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:73
> +    : m_ownerValue(ownerValue)
> +    , m_ready(false) { }

These should be indented.

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:76
> +    virtual void imageChanged(CachedImage* image, const IntRect* rect = 0) OVERRIDE {

{ on new line.
Comment 29 Tim Horton 2011-11-14 15:12:02 PST
(In reply to comment #28)
> (From update of attachment 115027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115027&action=review
> > Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:76
> > +    crossfadeImageSize = IntSize(fromImageSize.width() * inversePercentage + toImageSize.width() * m_percentage,
> > +                                 fromImageSize.height() * inversePercentage + toImageSize.height() * m_percentage);
> 
> Does the spec say anything about interpolating sizes? I'm not sure it's correct to animate the size too.

Yep, that's what it says to do with the size.
Comment 30 Tim Horton 2011-11-14 15:12:37 PST
(In reply to comment #29)
> Yep, that's what it says to do with the size.

More precisely, given ''cross-fade(A,B,p)'', where A and B are images and p is a percentage between 0% and 100%, the function represents an image with width equal to widthA × (1-p) + widthB × p and height equal to heightA × (1-p) + heightB × p. The contents of the image must be constructed by first scaling A and B to the size of the generated image, then applying dissolve(A,1-p) plus dissolve(B,p).
Comment 31 Dean Jackson 2011-11-14 15:33:32 PST
As discussed on IRC, we can't use SrcOver compositing to do this effect. We need additive blending.
Comment 32 Tim Horton 2011-11-14 23:42:27 PST
(In reply to comment #31)
> As discussed on IRC, we can't use SrcOver compositing to do this effect. We need additive blending.

I wonder (based on our conversation before)... is it not equivalent to paint opaque white behind the images and then continue on exactly as-is?
Comment 33 Tim Horton 2011-11-14 23:50:26 PST
(In reply to comment #32)
> (In reply to comment #31)
> > As discussed on IRC, we can't use SrcOver compositing to do this effect. We need additive blending.
> 
> I wonder (based on our conversation before)... is it not equivalent to paint opaque white behind the images and then continue on exactly as-is?

Of course not, in the case of images with alpha. Nevermind.
Comment 34 Tim Horton 2011-11-15 16:32:48 PST
Created attachment 115275 [details]
Render Cross-Fade
Comment 35 Tim Horton 2011-11-15 16:43:44 PST
(In reply to comment #28)
> (From update of attachment 115027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115027&action=review
> 
> This is a good start! I'd like to see a test case with a tiled cross-fade image.

I've made one in -simple and one in -tiled do actual tiling. This was an excellent call, because it was actually broken. Fixed.

> > Source/WebCore/css/CSSCrossfadeValue.cpp:71
> > +    if (value->isImageValue())
> > +        return static_cast<CSSImageValue*>(value.get())->cachedOrPendingImage()->isPendingImage();
> > +
> > +    if (value->isImageGeneratorValue())
> > +        return static_cast<CSSImageGeneratorValue*>(value.get())->isPending();
> 
> These two bare functions look like candidates for a virtual method.

The only common ancestor that CSSImageValue and CSSImageGeneratorValue share is CSSValue, not an ideal place for this. We could add another class here in between and make these virtual, but it's unclear that there's a point, with the devirtualization efforts that seem to be going on.

> > Source/WebCore/css/CSSImageGeneratorValue.cpp:117
> > +    if (isPending())
> > +        m_image = StylePendingImage::create(this).get();
> 
> It seems odd that calling this method has the side-effect of changing m_image.

That's how generatedImage() worked before, I believe. It is a little strange though; I'm open to suggestions!

> > Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:45
> > +    m_size = size;
> 
> Does the superclass not have a ctor that sets the size?

The superclass doesn't have any constructors. Should it? Can you not initialize members of the superclass in initializer lists? (it seems that you can't)

> > Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:69
> > +    fromImage = m_fromImage->image();
> > +    fromImageSize = fromImage->size();
> > +    toImage = m_toImage->image();
> > +    toImageSize = toImage->size();
> 
> Declare at first use. This is C++, not C :)

My background shining through!

(In reply to comment #31)
> As discussed on IRC, we can't use SrcOver compositing to do this effect. We need additive blending.

Fixed, we now use a transparency layer and CompositePlusLighter.
Comment 36 Tim Horton 2011-11-15 16:55:57 PST
>> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:76
>> +    virtual void imageChanged(CachedImage* image, const IntRect* rect = 0) OVERRIDE {
>
> { on new line.

Style bot is red because I made this change, so ignore it, I guess. Unless it's right?
Comment 37 Adam Barth 2011-11-15 17:01:00 PST
+levin: Maybe a style-checker bug?
Comment 38 WebKit Review Bot 2011-11-15 17:45:38 PST
Comment on attachment 115275 [details]
Render Cross-Fade

Attachment 115275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10487536

New failing tests:
css3/images/cross-fade-simple.html
css3/images/cross-fade-invalidation.html
fast/css/getComputedStyle/computed-style-cross-fade.html
css3/images/cross-fade-tiled.html
css3/images/cross-fade-blending.html
css3/images/cross-fade-sizing.html
Comment 39 David Levin 2011-11-15 17:52:32 PST
(In reply to comment #37)
> +levin: Maybe a style-checker bug?


A style checker bug, but it shouldn't be hit often and shouldn't be hit in this case (because it is only hit if someone is inlining a virtual function).

(In reply to comment #36)
> >> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:76
> >> +    virtual void imageChanged(CachedImage* image, const IntRect* rect = 0) OVERRIDE {
> >
> > { on new line.
> 
> Style bot is red because I made this change, so ignore it, I guess. Unless it's right?

The code is defining an inline function but since it is virtual and never called directly, the function will never be inlined, so it doesn't make sense to write it this way. In the past, I've been told (by ap) not to write functions inline in WebKit if they aren't meant to be inlined.

If this function isn't inlined, the style checker will no longer complain.
Comment 40 Tim Horton 2011-11-15 20:15:49 PST
Created attachment 115306 [details]
Render Cross-Fade (and allow fractional cross-fade amounts, and clamp them to 0-1)
Comment 41 Tim Horton 2011-11-15 20:22:32 PST
Created attachment 115308 [details]
Render Cross-Fade (Windows build fix, hopefully)
Comment 42 WebKit Review Bot 2011-11-15 21:05:37 PST
Comment on attachment 115308 [details]
Render Cross-Fade (Windows build fix, hopefully)

Attachment 115308 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10372836

New failing tests:
css3/images/cross-fade-tiled.html
css3/images/cross-fade-invalidation.html
css3/images/cross-fade-blending.html
css3/images/cross-fade-simple.html
css3/images/cross-fade-sizing.html
Comment 43 Simon Fraser (smfr) 2011-11-16 09:33:53 PST
Comment on attachment 115308 [details]
Render Cross-Fade (Windows build fix, hopefully)

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

> LayoutTests/css3/images/cross-fade-tiled.html:9
> +	-webkit-transform:translateZ(0);

Why the translateZ()?

> Source/WebCore/css/CSSCrossfadeValue.cpp:42
> +CachedImage* cachedImageForCSSValue(PassRefPtr<CSSValue>, const RenderObject*, const IntSize&);
> +void loadSubimage(PassRefPtr<CSSValue>, CachedResourceLoader*);
> +bool subimageIsPending(PassRefPtr<CSSValue>);

You could just move the functions to the stop of the file and make them static.

> Source/WebCore/css/CSSCrossfadeValue.cpp:62
> +    IntSize size;
> +
> +    CachedImage* fromImage = cachedImageForCSSValue(m_fromImage, renderer, size);
> +    CachedImage* toImage = cachedImageForCSSValue(m_toImage, renderer, size);

Seems weird to send in an empty size here.

> Source/WebCore/css/CSSCrossfadeValue.cpp:85
> +bool subimageIsPending(PassRefPtr<CSSValue> value)

You should only use PassRefPtr<CSSValue> if you're passing ownership, which doesn't seem to be the case here. Can you pass a const CSSValue*?

> Source/WebCore/css/CSSCrossfadeValue.cpp:98
> +void loadSubimage(PassRefPtr<CSSValue> value, CachedResourceLoader* cachedResourceLoader)

Ditto.

> Source/WebCore/css/CSSCrossfadeValue.cpp:113
> +CachedImage* cachedImageForCSSValue(PassRefPtr<CSSValue> value, const RenderObject* renderer, const IntSize& size)

Ditto.

> Source/WebCore/css/CSSCrossfadeValue.cpp:115
>      UNUSED_PARAM(size);

Are you going to use size?

> Source/WebCore/css/CSSCrossfadeValue.h:65
> +    // NOTE: We put the ImageObserver in a member instead of inheriting from it

No need for "NOTE:".

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:76
> +    context->beginTransparencyLayer(1.0);

1.0 -> 1

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:62
> +    CachedImage* m_fromImage;
> +    CachedImage* m_toImage;

I'd like to see a comment about who owns these CachedImages.
Comment 44 David Levin 2011-11-16 10:00:31 PST
(In reply to comment #39)
> (In reply to comment #37)
> > +levin: Maybe a style-checker bug?
> A style checker bug, but it shouldn't be hit often.

fwiw, I did fix the style checker bug as well (even though I don't think it should appear hardly ever... but if someone inlines a virtual function, I don't want to give them an obtuse error. If we want to flag it, the error should be more specific.)
https://bugs.webkit.org/show_bug.cgi?id=72515
Comment 45 Tim Horton 2011-11-16 17:28:53 PST
Landed in r100535, with Simon's changes.
Comment 46 Csaba Osztrogonác 2011-11-16 23:57:44 PST
It broke fast/css/getComputedStyle/computed-style-cross-fade.html on the Qt bot:

(In reply to comment #45)
> Landed in r100535, with Simon's changes.

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/css/getComputedStyle/computed-style-cross-fade-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/css/getComputedStyle/computed-style-cross-fade-actual.txt 
@@ -1,22 +1,7 @@
-Blocked access to external URL http://example.com/example.png
 Blocked access to external URL http://example.com/example.png
 Blocked access to external URL http://example.com/a.png
 Blocked access to external URL http://example.com/b.png
-Blocked access to external URL http://example.com/example.png
 Blocked access to external URL http://example.com/c.png
-Blocked access to external URL http://example.com/a.png
-Blocked access to external URL http://example.com/b.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
 -webkit-cross-fade
Comment 47 Csaba Osztrogonác 2011-11-17 08:51:06 PST
It fails on GTK with similar diff:

--- /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/fast/css/getComputedStyle/computed-style-cross-fade-expected.txt 
+++ /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/fast/css/getComputedStyle/computed-style-cross-fade-actual.txt 
@@ -1,22 +1,11 @@
-Blocked access to external URL http://example.com/example.png
 Blocked access to external URL http://example.com/example.png
 Blocked access to external URL http://example.com/a.png
 Blocked access to external URL http://example.com/b.png
-Blocked access to external URL http://example.com/example.png
 Blocked access to external URL http://example.com/c.png
-Blocked access to external URL http://example.com/a.png
-Blocked access to external URL http://example.com/b.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
-Blocked access to external URL http://example.com/example.png
+Blocked access to external URL http://www.iana.org/domains/example/
+Blocked access to external URL http://www.iana.org/domains/example/
+Blocked access to external URL http://www.iana.org/domains/example/
+Blocked access to external URL http://www.iana.org/domains/example/
 -webkit-cross-fade
Comment 48 Csaba Osztrogonác 2011-11-17 08:55:04 PST
I skipped fast/css/getComputedStyle/computed-style-cross-fade.html on Qt temporarily to make buildbot happier: http://trac.webkit.org/changeset/100633 .

Please unskip it when a proper fix landing.
Comment 49 Tim Horton 2011-11-17 12:11:40 PST
(In reply to comment #47)
> It fails on GTK with similar diff:
> 
> --- /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/fast/css/getComputedStyle/computed-style-cross-fade-expected.txt 
> +++ /var/lib/buildbot/build/gtk-linux-32-release/build/layout-test-results/fast/css/getComputedStyle/computed-style-cross-fade-actual.txt 
> @@ -1,22 +1,11 @@
> -Blocked access to external URL http://example.com/example.png

Oh boy; I was hoping those wouldn't be a problem (would be consistent) but I was wrong. What do we use for images? I can't use anything on the local filesystem, as it resolves it to an absolute path which will certainly be broken on many systems. It looks like some of the other tests use "dummy://test.png" -- is that what I should be using? I'll try it out.
Comment 50 Tim Horton 2011-11-17 12:28:47 PST
Created attachment 115659 [details]
Fix the getComputedStyle test to use dummy://
Comment 51 Tim Horton 2011-11-17 14:15:08 PST
Created attachment 115678 [details]
Fix layering violation
Comment 52 WebKit Review Bot 2011-11-17 15:20:08 PST
Comment on attachment 115659 [details]
Fix the getComputedStyle test to use dummy://

Clearing flags on attachment: 115659

Committed r100687: <http://trac.webkit.org/changeset/100687>
Comment 53 Simon Fraser (smfr) 2011-11-29 14:38:41 PST
Comment on attachment 115678 [details]
Fix layering violation

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

> Source/WebCore/css/CSSCrossfadeValue.cpp:58
> +        
> +        if (!styleCachedImage)

I'd omit the blank line here.

> Source/WebCore/css/CSSCrossfadeValue.cpp:158
> +    UNUSED_PARAM(image);

You could just omit the parameter name instead.
Comment 54 Tim Horton 2011-11-29 18:27:49 PST
I just noticed something weird about refreshing when the page is in the cache (looks like freeing something we shouldn't be, but it's not a crash, the image shows up as the nullImage tiled over the whole space), so I'm going to look at that tomorrow and not land this as-is.
Comment 55 Tim Horton 2011-11-30 12:39:52 PST
Created attachment 117250 [details]
fix layering violation, take 2

More thorough testing this time, and it seems to work perfectly.
Comment 56 Tim Horton 2011-11-30 13:23:23 PST
Landed in r101546.
Comment 57 Tim Horton 2012-01-13 16:32:57 PST
Not sure why this is still open.