Implement CSS3 Images cross-fade() image function

[Simon Fraser (smfr)]

We should implement http://dev.w3.org/csswg/css3-images/#cross-fade-function so that we can enable transitions for background-image.

[Simon Fraser (smfr)]

<rdar://problem/10042838>

[Tim Horton]

Created attachment 112832 [details]
parse -webkit-cross-fade

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.

[Simon Fraser (smfr)]

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")?

[Tim Horton]

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.

[Tim Horton]

Parsing part landed in r98775.

[Ryosuke Niwa]

Fixed the expected result in http://trac.webkit.org/changeset/98797.

[Tim Horton]

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++.

[Tim Horton]

Created attachment 113676 [details]
[preliminary] render cross-fade - should apply now

[WebKit Review Bot]

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.

[Gustavo Noronha (kov)]

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

[WebKit Review Bot]

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

[Early Warning System Bot]

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

[Tim Horton]

Oh, the other build systems again. Someday I'll remember that without being reminded.

[Gyuyoung Kim]

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

[Tim Horton]

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...

[Simon Fraser (smfr)]

Do you need to wait for the image load events before testing?

[Tim Horton]

(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*).

[Tim Horton]

Created attachment 115002 [details]
Render Cross-fade

Have at it! I have a feeling this is going to need a good bit of review.

[Early Warning System Bot]

Comment on attachment 115002 [details]
Render Cross-fade

Attachment 115002 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10484022

[Tim Horton]

Created attachment 115011 [details]
Render Cross-Fade (plus build system changes for other ports)

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Gustavo Noronha (kov)]

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

[Tim Horton]

Created attachment 115025 [details]
Render Cross-Fade (plus build fix for non-Mac)

[Early Warning System Bot]

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

[Tim Horton]

Created attachment 115027 [details]
Render Cross-Fade (plus build fix for non-debug)

Perhaps I should get out my VMs.

[Simon Fraser (smfr)]

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.

[Tim Horton]

(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.

[Tim Horton]

(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).

[Dean Jackson]

As discussed on IRC, we can't use SrcOver compositing to do this effect. We need additive blending.

[Tim Horton]

(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?

[Tim Horton]

(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.

[Tim Horton]

Created attachment 115275 [details]
Render Cross-Fade

[Tim Horton]

(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.

[Tim Horton]

>> 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?

[Adam Barth]

+levin: Maybe a style-checker bug?

[WebKit Review Bot]

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

[David Levin]

(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.

[Tim Horton]

Created attachment 115306 [details]
Render Cross-Fade (and allow fractional cross-fade amounts, and clamp them to 0-1)

[Tim Horton]

Created attachment 115308 [details]
Render Cross-Fade (Windows build fix, hopefully)

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

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.

[David Levin]

(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

[Tim Horton]

Landed in r100535, with Simon's changes.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

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.

[Tim Horton]

(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.

[Tim Horton]

Created attachment 115659 [details]
Fix the getComputedStyle test to use dummy://

[Tim Horton]

Created attachment 115678 [details]
Fix layering violation

[WebKit Review Bot]

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>

[Simon Fraser (smfr)]

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.

[Tim Horton]

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.

[Tim Horton]

Created attachment 117250 [details]
fix layering violation, take 2

More thorough testing this time, and it seems to work perfectly.

[Tim Horton]

Landed in r101546.

[Tim Horton]

Not sure why this is still open.