Bug 52162 - Implement CSS3 Images cross-fade() image function
: Implement CSS3 Images cross-fade() image function
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
: InRadar
: 72639
: 46590
  Show dependency treegraph
 
Reported: 2011-01-10 11:49 PST by
Modified: 2012-01-13 16:32 PST (History)


Attachments
parse -webkit-cross-fade (29.64 KB, patch)
2011-10-28 01:33 PST, Tim Horton
simon.fraser: review-
Review Patch | Details | Formatted Diff | Diff
address changes (30.32 KB, patch)
2011-10-28 15:56 PST, Tim Horton
simon.fraser: review+
Review Patch | Details | Formatted Diff | Diff
[preliminary] render cross-fade (34.69 KB, patch)
2011-11-03 10:59 PST, Tim Horton
no flags Review Patch | Details | Formatted Diff | Diff
[preliminary] render cross-fade - should apply now (34.30 KB, patch)
2011-11-04 10:40 PST, Tim Horton
gns: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
[preliminary] render cross-fade (57.53 KB, patch)
2011-11-10 18:05 PST, Tim Horton
no flags Review Patch | Details | Formatted Diff | Diff
Render Cross-fade (87.35 KB, patch)
2011-11-14 12:23 PST, Tim Horton
webkit-ews: commit‑queue-
Review Patch | 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-
Review Patch | 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-
Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff
Render Cross-Fade (92.20 KB, patch)
2011-11-15 16:32 PST, Tim Horton
webkit.review.bot: commit‑queue-
Review Patch | 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 Review Patch | 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-
Review Patch | 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 Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff
fix layering violation, take 2 (18.74 KB, patch)
2011-11-30 12:39 PST, Tim Horton
simon.fraser: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-08-29 16:39:35 PST -------
<rdar://problem/10042838>
------- Comment #2 From 2011-10-28 01:33:11 PST -------
Created an attachment (id=112832) [details]
parse -webkit-cross-fade
------- Comment #3 From 2011-10-28 07:40:48 PST -------
(From update of attachment 112832 [details])
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 From 2011-10-28 10:58:43 PST -------
(From update of attachment 112832 [details])
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 From 2011-10-28 15:56:28 PST -------
Created an attachment (id=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 From 2011-10-28 16:12:35 PST -------
Parsing part landed in r98775.
------- Comment #7 From 2011-10-28 23:43:15 PST -------
Fixed the expected result in http://trac.webkit.org/changeset/98797.
------- Comment #8 From 2011-11-03 10:59:39 PST -------
Created an attachment (id=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 From 2011-11-04 10:40:37 PST -------
Created an attachment (id=113676) [details]
[preliminary] render cross-fade - should apply now
------- Comment #10 From 2011-11-04 10:43:45 PST -------
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 From 2011-11-04 10:53:57 PST -------
(From update of attachment 113676 [details])
Attachment 113676 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10336017
------- Comment #12 From 2011-11-04 10:54:31 PST -------
(From update of attachment 113676 [details])
Attachment 113676 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10333020
------- Comment #13 From 2011-11-04 10:56:48 PST -------
(From update of attachment 113676 [details])
Attachment 113676 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10333022
------- Comment #14 From 2011-11-04 10:57:25 PST -------
Oh, the other build systems again. Someday I'll remember that without being reminded.
------- Comment #15 From 2011-11-04 11:05:09 PST -------
(From update of attachment 113676 [details])
Attachment 113676 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10334021
------- Comment #16 From 2011-11-10 18:05:30 PST -------
Created an attachment (id=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 From 2011-11-11 10:18:19 PST -------
Do you need to wait for the image load events before testing?
------- Comment #18 From 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 From 2011-11-14 12:23:58 PST -------
Created an attachment (id=115002) [details]
Render Cross-fade

Have at it! I have a feeling this is going to need a good bit of review.
------- Comment #20 From 2011-11-14 12:35:24 PST -------
(From update of attachment 115002 [details])
Attachment 115002 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10484022
------- Comment #21 From 2011-11-14 12:37:00 PST -------
Created an attachment (id=115011) [details]
Render Cross-Fade (plus build system changes for other ports)
------- Comment #22 From 2011-11-14 12:47:51 PST -------
(From update of attachment 115011 [details])
Attachment 115011 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10487026
------- Comment #23 From 2011-11-14 13:39:00 PST -------
(From update of attachment 115011 [details])
Attachment 115011 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10485041
------- Comment #24 From 2011-11-14 13:41:19 PST -------
(From update of attachment 115011 [details])
Attachment 115011 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10487042
------- Comment #25 From 2011-11-14 13:43:36 PST -------
Created an attachment (id=115025) [details]
Render Cross-Fade (plus build fix for non-Mac)
------- Comment #26 From 2011-11-14 13:51:54 PST -------
(From update of attachment 115025 [details])
Attachment 115025 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10372237
------- Comment #27 From 2011-11-14 13:56:06 PST -------
Created an attachment (id=115027) [details]
Render Cross-Fade (plus build fix for non-debug)

Perhaps I should get out my VMs.
------- Comment #28 From 2011-11-14 14:25:24 PST -------
(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.

> 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 From 2011-11-14 15:12:02 PST -------
(In reply to comment #28)
> (From update of attachment 115027 [details] [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 From 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 From 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 From 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 From 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 From 2011-11-15 16:32:48 PST -------
Created an attachment (id=115275) [details]
Render Cross-Fade
------- Comment #35 From 2011-11-15 16:43:44 PST -------
(In reply to comment #28)
> (From update of attachment 115027 [details] [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 From 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 From 2011-11-15 17:01:00 PST -------
+levin: Maybe a style-checker bug?
------- Comment #38 From 2011-11-15 17:45:38 PST -------
(From update of attachment 115275 [details])
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 From 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 From 2011-11-15 20:15:49 PST -------
Created an attachment (id=115306) [details]
Render Cross-Fade (and allow fractional cross-fade amounts, and clamp them to 0-1)
------- Comment #41 From 2011-11-15 20:22:32 PST -------
Created an attachment (id=115308) [details]
Render Cross-Fade (Windows build fix, hopefully)
------- Comment #42 From 2011-11-15 21:05:37 PST -------
(From update of attachment 115308 [details])
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 From 2011-11-16 09:33:53 PST -------
(From update of attachment 115308 [details])
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 From 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 From 2011-11-16 17:28:53 PST -------
Landed in r100535, with Simon's changes.
------- Comment #46 From 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 From 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 From 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 From 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 From 2011-11-17 12:28:47 PST -------
Created an attachment (id=115659) [details]
Fix the getComputedStyle test to use dummy://
------- Comment #51 From 2011-11-17 14:15:08 PST -------
Created an attachment (id=115678) [details]
Fix layering violation
------- Comment #52 From 2011-11-17 15:20:08 PST -------
(From update of attachment 115659 [details])
Clearing flags on attachment: 115659

Committed r100687: <http://trac.webkit.org/changeset/100687>
------- Comment #53 From 2011-11-29 14:38:41 PST -------
(From update of attachment 115678 [details])
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 From 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 From 2011-11-30 12:39:52 PST -------
Created an attachment (id=117250) [details]
fix layering violation, take 2

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