Bug 47156 – CSS 2.1 failure: background-intrinsic-*

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

Created attachment 69751 [details]
background-intrinsic-001.htm

The following tests fail:

html4/background-intrinsic-001	fail
html4/background-intrinsic-003	fail
html4/background-intrinsic-004	fail
html4/background-intrinsic-005	fail
html4/background-intrinsic-007	fail
html4/background-intrinsic-008	fail

[Simon Fraser (smfr)]

Probably we don't handle images that just have an intrinsic ratio.

[Simon Fraser (smfr)]

The problem here is that SVGImage fails to report the correct size() for SVG with a viewBox. If both height and width of the SVG are relative, the viewBox should be used to compute the correct size for a given containerSize, but I can't find the right incantation of viewBox/preserveAspectRatio etc to do this.

[Nikolas Zimmermann]

(In reply to comment #2)
> The problem here is that SVGImage fails to report the correct size() for SVG with a viewBox. If both height and width of the SVG are relative, the viewBox should be used to compute the correct size for a given containerSize, but I can't find the right incantation of viewBox/preserveAspectRatio etc to do this.

SVGFitToViewBox contains a helper metho for that task:
static AffineTransform viewBoxToViewTransform(const FloatRect& viewBoxRect, const SVGPreserveAspectRatio&, float viewWidth, float viewHeight);

For a given viewBox rect and a SVGPreseveAspectRatio, as well as an available width/height it gives you the AffineTransform that you need to concat to the current graphics context CTM, to render the SVG properly according to the viewBox/preserveAspectRatio settings.

Does that help?
(Note, I only had a quick glance at what you wrote, and didn't check the actual testcase, i'm in hurry atm...)

[Nikolas Zimmermann]

I have a fix for this! I got all the CSS 2.1 background-intrinsic-* tests working, without breaking any of the CSS3 background-size tests (contain/cover still work fine).

I'm down to zero regressions and will prepare a fix for this soon.

[Nikolas Zimmermann]

Created attachment 101807 [details]
Patch

Uploading a first version of the patch w/o a proper ChangeLog in WebCore. Want to get some early results - especially the cr-linux-ews pixel test results!
This took more than 2 weeks, was much harder than anticipated.

It fixes all CSS 2.1 background-intrinsic-* sizing tests, as well allows tiled SVG background images (though zooming is broken in that case, just like for non-SVG background images - I'll fix that in a follow-up patch).

[WebKit Review Bot]

Attachment 101807 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1

Source/WebCore/svg/graphics/SVGImage.cpp:148:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/svg/graphics/SVGImage.cpp:170:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/rendering/RenderImage.cpp:181:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/rendering/RenderImage.cpp:182:  Missing space before {  [whitespace/braces] [5]
Total errors found: 4 in 99 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 101807 [details]
Patch

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

[Nikolas Zimmermann]

Created attachment 101812 [details]
Patch v2

Trying to get cr-linux-ews to build to receive pixel test results.
Not sure why win won't apply the patch.

[WebKit Review Bot]

Attachment 101812 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1

Source/WebCore/ChangeLog:82:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 100 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 101812 [details]
Patch v2

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

[Nikolas Zimmermann]

Created attachment 101813 [details]
Patch v3

I'll write a ChangeLog during the next hours, this should be enough now to get cr-lin-ews results, hopefully...

[WebKit Review Bot]

Comment on attachment 101813 [details]
Patch v3

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

[Nikolas Zimmermann]

Created attachment 101815 [details]
Patch v4

Chromium WebCore builds now, attempt to fix WebKit chromium (DragImageTest).

[Nikolas Zimmermann]

CCing Adam Roben, who might know whats going on with the win slave, why it doesn't apply the patch.

[WebKit Review Bot]

Comment on attachment 101815 [details]
Patch v4

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

New failing tests:
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html
css2.1/20110323/background-intrinsic-007.htm
fast/backgrounds/size/contain-and-cover.html
css2.1/20110323/background-intrinsic-006.htm
http/tests/inspector/network/network-cachedresources-with-same-urls.html
css2.1/20110323/background-intrinsic-008.htm
css2.1/20110323/background-intrinsic-005.htm
fast/forms/input-spinbutton-capturing.html
fast/block/float/015.html
css2.1/20110323/background-intrinsic-009.htm
http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
fast/canvas/webgl/css-webkit-canvas-repaint.html
fast/canvas/webgl/get-active-test.html
fast/forms/input-number-large-padding.html
css2.1/20110323/background-intrinsic-004.htm
fast/forms/input-number-events.html
http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html
css2.1/20110323/background-intrinsic-003.htm

[Early Warning System Bot]

Comment on attachment 101815 [details]
Patch v4

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

[Adam Roben (:aroben)]

(In reply to comment #14)
> CCing Adam Roben, who might know whats going on with the win slave, why it doesn't apply the patch.

I don't know why. It's hard to tell with so little output from the bot. Maybe Eric or Adam can pull more log info for us?

[Adam Barth]

> I don't know why. It's hard to tell with so little output from the bot. Maybe Eric or Adam can pull more log info for us?

Not sure.  Maybe its an svn/git issue?  I think the win bot uses svn.

[Nikolas Zimmermann]

Created attachment 102011 [details]
Patch v5

Working on fixing regressions... Leaving out r? flag, until these are resolved.

[WebKit Review Bot]

Attachment 102011 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1

Source/WebCore/loader/cache/CachedImage.cpp:207:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 106 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 102011 [details]
Patch v5

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

[WebKit Review Bot]

Comment on attachment 102011 [details]
Patch v5

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

New failing tests:
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html
css2.1/20110323/background-intrinsic-007.htm
css1/text_properties/vertical_align.html
css2.1/20110323/background-intrinsic-006.htm
http/tests/inspector/network/network-cachedresources-with-same-urls.html
css2.1/20110323/background-intrinsic-008.htm
fast/canvas/webgl/get-active-test.html
fast/block/float/015.html
css2.1/20110323/background-intrinsic-009.htm
fast/block/positioning/replaced-inside-fixed-top-bottom.html
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
fast/canvas/webgl/css-webkit-canvas-repaint.html
http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html
css2.1/20110323/background-intrinsic-005.htm
fast/backgrounds/size/contain-and-cover.html
http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-003.htm

[Nikolas Zimmermann]

Created attachment 102107 [details]
Patch v6

Finished ChangeLog, patch should be ready for review now.

[WebKit Review Bot]

Attachment 102107 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1

Source/WebCore/loader/cache/CachedImage.cpp:207:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 102 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 102131 [details]
Patch v7

Previous patch was missing three rebaselines on mac (all progressions!).
This patch passes locally and has no more pixel test regressions.

[WebKit Review Bot]

Comment on attachment 102131 [details]
Patch v7

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

New failing tests:
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html
css2.1/20110323/background-intrinsic-007.htm
css1/text_properties/vertical_align.html
css2.1/20110323/background-intrinsic-006.htm
http/tests/inspector/network/network-cachedresources-with-same-urls.html
css2.1/20110323/background-intrinsic-008.htm
fast/canvas/webgl/get-active-test.html
fast/block/float/015.html
css2.1/20110323/background-intrinsic-009.htm
fast/block/positioning/replaced-inside-fixed-top-bottom.html
http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
fast/canvas/webgl/css-webkit-canvas-repaint.html
http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html
css2.1/20110323/background-intrinsic-005.htm
fast/backgrounds/size/contain-and-cover.html
http/tests/inspector/resource-tree/resource-tree-reload.html
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-003.htm

[Nikolas Zimmermann]

Created attachment 102624 [details]
Patch v8

Updated to ToT. Anyone wants to review?

[WebKit Review Bot]

Comment on attachment 102624 [details]
Patch v8

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

New failing tests:
http/tests/security/webgl-remote-read-remote-image-blocked-no-crossorigin.html
css2.1/20110323/background-intrinsic-007.htm
css1/text_properties/vertical_align.html
css2.1/20110323/background-intrinsic-006.htm
http/tests/inspector/network/network-cachedresources-with-same-urls.html
fast/gradients/generated-gradients.html
css2.1/20110323/background-intrinsic-005.htm
fast/block/float/015.html
css2.1/20110323/background-intrinsic-009.htm
fast/block/positioning/replaced-inside-fixed-top-bottom.html
css2.1/20110323/background-intrinsic-008.htm
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
fast/canvas/webgl/css-webkit-canvas-repaint.html
http/tests/security/canvas-remote-read-remote-image-blocked-no-crossorigin.html
fast/backgrounds/size/contain-and-cover.html
http/tests/inspector/resource-tree/resource-tree-reload.html
http/tests/security/canvas-remote-read-remote-image-blocked-then-allowed.html
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-003.htm

[Nikolas Zimmermann]

Marking this bug as depending on 64974. I want to resolve all issues related to the size negotiation with object/embed/iframe/... first before landing the final part for CSS image values & html:img/svg:image support.

Still the patch can be reviewed as-is :-)

[Nikolas Zimmermann]

Quick update:
In bug 67087, a first split-up from this patch was supposed to land, though that piece of code is flawed and needs to be redesigned. I'm going to take care of this next week, stay tuned! This bug has top priority for me.

[Nikolas Zimmermann]

Comment on attachment 102624 [details]
Patch v8

Clearing flags. A new patch will be uploaded once 69416 is in.

[Nikolas Zimmermann]

Created attachment 110122 [details]
Draft patch

To aid Anttis review on bug 69416, uploading an updated master patch, w/o test changes.

[WebKit Review Bot]

Attachment 110122 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/loader/cache/CachedImage.cp..." exit_code: 1

Source/WebCore/rendering/style/StyleImage.h:108:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/rendering/style/StyleImage.h:109:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 110122 [details]
Draft patch

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

New failing tests:
http/tests/security/xssAuditor/cookie-injection.html
fast/selectors/unqualified-hover-strict.html
css1/text_properties/vertical_align.html
fast/gradients/generated-gradients.html
fast/ruby/generated-before-counter-doesnt-crash.html
fast/forms/input-spinbutton-capturing.html
fast/block/float/015.html
fast/images/percent-height-image.html
fast/repaint/block-layout-inline-children-replaced.html
fast/ruby/before-table-doesnt-crash.html
fast/canvas/webgl/css-webkit-canvas-repaint.html
fast/block/positioning/replaced-inside-fixed-top-bottom.html
fast/forms/input-number-large-padding.html
fast/backgrounds/size/contain-and-cover.html
fast/forms/input-number-events.html
fast/forms/implicit-submission.html
fast/ruby/before-block-doesnt-crash.html

[Nikolas Zimmermann]

Created attachment 111303 [details]
Updated patch

Not marking for review yet, as there's still one known regression, but I want to gather some early EWS build results.

[WebKit Review Bot]

Attachment 111303 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Created attachment 111427 [details]
Final patch - v1

Finally a complete patch again, now that all previously splitted-off chunks landed.
Code changes are about 53k now, 15k ChangeLogs and the rest are new tests and pixel results.

[Antti Koivisto]

Comment on attachment 111427 [details]
Final patch - v1

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

Looks pretty good though I can't really review the substance of the size calculations.

> Source/WebCore/loader/cache/CachedImage.cpp:138
> +    if (!m_image || !m_image->isSVGImage())
> +        return m_image.get();

It would be nicer to separate these conditions (return 0 on first)

> Source/WebCore/loader/cache/CachedImage.cpp:147
> +    if (!m_image || !m_image->isSVGImage())
> +        return m_image.get();

It would be nicer to separate these conditions (return 0 on first)

> Source/WebCore/loader/cache/CachedImage.cpp:167
> +    if (!m_image || !m_image->isSVGImage())
> +        return m_image;

It would be nicer to separate these conditions (return 0 on first)

> Source/WebCore/loader/cache/CachedImage.cpp:172
> +    IntSize size;
> +    if (Image* result = m_svgImageCache.imageForRenderer(renderer, &size))
> +        return result;
> +    if (size.isEmpty())
> +        return m_image;

Why isn't a zero sized image valid here? If it is not valid, why was it cached? Why return m_image instead? Could this ASSERT(!size.isEmpty()) instead?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:786
> +static inline LayoutSize calculateImageIntrinsicDimensions(const RenderBoxModelObject* box, StyleImage* image, const LayoutSize& positioningAreaSize)
> +{

Hyatt should check this as well. 

The function is too long. It should be split into logically named subfunctions.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:824
> +    // If the image has one of either an intrinsic width or an intrinsic height:
> +    // * and an intrinsic aspect ratio, then the missing dimension is calculated from the given dimension and the ratio.
> +    // * and no intrinsic aspect ratio, then the missing dimension is assumed to be the size of the rectangle that
> +    //   establishes the coordinate system for the 'background-position' property.
> +    if ((resolvedWidth && !resolvedHeight) || (!resolvedWidth && resolvedHeight)) {
> +        if (intrinsicRatio.isEmpty()) {
> +            if (resolvedWidth)
> +                resolvedHeight = positioningAreaSize.height();
> +            else
> +                resolvedWidth = positioningAreaSize.width();
> +        } else {
> +            // FIXME: Remove unnecessary rounding when layout is off ints: webkit.org/b/63656
> +            if (resolvedWidth)
> +                resolvedHeight = static_cast<LayoutUnit>(ceilf(resolvedWidth * intrinsicRatio.height() / intrinsicRatio.width()));
> +            else
> +                resolvedWidth = static_cast<LayoutUnit>(ceilf(resolvedHeight * intrinsicRatio.width() / intrinsicRatio.height()));
> +        }
> +
> +        return LayoutSize(resolvedWidth, resolvedHeight);
> +    }

This path should be a function.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:853
> +    }

This path should be a function.

[WebKit Review Bot]

Comment on attachment 111427 [details]
Final patch - v1

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

New failing tests:
fast/repaint/block-layout-inline-children-replaced.html
css2.1/20110323/background-intrinsic-007.htm
css1/text_properties/vertical_align.html
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-008.htm
css2.1/20110323/background-intrinsic-005.htm
fast/block/float/015.html
css2.1/20110323/background-intrinsic-009.htm
fast/block/positioning/replaced-inside-fixed-top-bottom.html
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
fast/backgrounds/size/contain-and-cover.html
fast/forms/implicit-submission.html
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-003.htm

[Nikolas Zimmermann]

(In reply to comment #38) 
> > Source/WebCore/loader/cache/CachedImage.cpp:167
> > +    if (!m_image || !m_image->isSVGImage())
> > +        return m_image;
> 
> It would be nicer to separate these conditions (return 0 on first)
Fixed, all occurrences.

> 
> > Source/WebCore/loader/cache/CachedImage.cpp:172
> > +    IntSize size;
> > +    if (Image* result = m_svgImageCache.imageForRenderer(renderer, &size))
> > +        return result;
> > +    if (size.isEmpty())
> > +        return m_image;
> 
> Why isn't a zero sized image valid here? If it is not valid, why was it cached? Why return m_image instead? Could this ASSERT(!size.isEmpty()) instead?
I guess you misread the code.
The m_svgImageCache.imageForRenderer() call looks up the overridden container size for the passed in renderer. If the renderer is known, size will be non-zero. That doesn't necessarily mean that a cached image is also available! imageForRenderer() will return 0, if it knows the size, but the Image of that desired size hasn't been produced yet. If the renderer is completely unknown imageForRenderer() will also return 0, but also size is empty.

To rephrase:
lookupOrCreateImageRenderer() will return m_image, if there is no associated container size for a given renderer.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:786> > +{
> 
> Hyatt should check this as well. 
> 
> The function is too long. It should be split into logically named subfunctions.
Okay, will do.

[Nikolas Zimmermann]

Created attachment 111450 [details]
Final patch - v2

Fixed Anttis comments.

[Nikolas Zimmermann]

Created attachment 112504 [details]
Final patch - v3

CSS Gradients + zooming was broken, it turns out there is no pixel test for that, so I added one.
Made few adjustments and cleaned up the zooming related code to make it work for generated images, cached images and cached SVG images.
SVG Images are zoomed by adjusting their page zoom factor. Cached non-SVG images are scaled up, and generated images render at any desired size.
Previously zooming was only handled by adjusting the size of the root <svg> renderer, but that does not work at all with relative lengths.

The new behaviors match Opera / Firefox / IE9, finally.
We're still the only one to support the CSS 2.1 background-intrinsic* tests perfectly - checked using latest nightlies of these browsers, except IE.

[Antti Koivisto]

Comment on attachment 112504 [details]
Final patch - v3

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

r=me, looks good to me!

> Source/WebCore/rendering/RenderBoxModelObject.cpp:842
> +    // A generated image without a fixed size, will always return the container size as intrinsic size.

It would be good to have a spec link here

[Nikolas Zimmermann]

(In reply to comment #43)
> (From update of attachment 112504 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112504&action=review
> 
> r=me, looks good to me!
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:842
> > +    // A generated image without a fixed size, will always return the container size as intrinsic size.
> 
> It would be good to have a spec link here

Fixed. Landed in r98483.
I will close all fixed bugs, if no regressions appear anywhere.

[Julien Chaffraix]

Reverted r98483 for reason:

Change is causing some crashes and ASSERT.

Committed r98508: <http://trac.webkit.org/changeset/98508>

[Nikolas Zimmermann]

Created attachment 112991 [details]
Next try - v1

This obsoletes the "Final patch - v3" which was landed and reverted. It turns out there was a race condition, when more than one client accessed the resource.
Solving it requires aligning CachedImages usage of ImageBySizeCache with the CSSImageGeneratorValue approach, which is the only safe one.

Awaiting EWS results, to see whether flakiness is gone as well.

[WebKit Review Bot]

Attachment 112991 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css2..." exit_code: 1

Source/WebCore/rendering/ImageBySizeCache.h:61:  The parameter name "size" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/svg/graphics/SVGImage.cpp:126:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 116 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 112991 [details]
Next try - v1

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

New failing tests:
fast/repaint/block-layout-inline-children-replaced.html
css2.1/20110323/background-intrinsic-007.htm
css1/text_properties/vertical_align.html
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-008.htm
css2.1/20110323/background-intrinsic-005.htm
fast/block/float/015.html
css2.1/20110323/background-intrinsic-009.htm
fast/block/positioning/replaced-inside-fixed-top-bottom.html
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
css2.1/20110323/background-intrinsic-004.htm
fast/backgrounds/size/contain-and-cover-zoomed.html
css2.1/20110323/background-intrinsic-003.htm

[Nikolas Zimmermann]

Created attachment 112993 [details]
Next try - v2

Fixed style issues, updated to ToT again. No more known regressions, running all svg/as-image, as-background-image, zoom/page tests in one WebProcess works fine, including wild zooming and reloading :-)

Unfortunately I never saw the backtrace that Julien captured on my own. When the patch landed a DoS attack hosed build.webkit.org, so I couldn't receive any logs. Hopefully it's gone now - at least we're not deleting SVGImages anymore from within setContainerSizeForRenderer, which was the root of the problem according to the backtrace.

Let's see...

[WebKit Review Bot]

Comment on attachment 112993 [details]
Next try - v2

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

New failing tests:
css2.1/20110323/background-intrinsic-007.htm
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-008.htm
css2.1/20110323/background-intrinsic-005.htm
css2.1/20110323/background-intrinsic-009.htm
fast/block/positioning/replaced-inside-fixed-top-bottom.html
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
fast/backgrounds/size/contain-and-cover-zoomed.html
css2.1/20110323/background-intrinsic-003.htm

[Nikolas Zimmermann]

Created attachment 113027 [details]
Next try - v3

Stupid me, only included LayoutTests in the last diff, forgot Source/.

[Antti Koivisto]

Comment on attachment 113027 [details]
Next try - v3

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

r=me, but some comments to ponder in the future.

> Source/WebCore/loader/cache/CachedImage.cpp:168
> +    // Request desired size/zoom for this renderer from the cache.
> +    IntSize size;
> +    float zoom = 1;
> +    m_svgImageCache.getDesiredSizeAndZoom(renderer, size, zoom);
> +    if (size.isEmpty())
> +        return m_image.get();
> +
> +    if (Image* image = m_svgImageCache.getImage(renderer, size, zoom))
> +        return image;
> +
> +    // Create and cache new image at desired size.
> +    RefPtr<Image> newImage = SVGImage::createWithDataAndSize(this, m_data.get(), size, zoom);
> +    Image* newImagePtr = newImage.get();
> +    m_svgImageCache.addClient(renderer, size, zoom);
> +    m_svgImageCache.putImage(size, newImage.release());
> +    return newImagePtr;

Notice how you are pulling data our of m_svgImageCache and then using it to call right back to the m_svgImageCache? This is a clear sign that all this code belongs to the image cache class.

Generally the design (name included) of the ImageBySizeCache is not good as its purpose and functionality is vague. The changes in this patch make the situation worse.

> Source/WebCore/rendering/ImageBySizeCache.h:37
> +        , desiredSize(newSize)

I think "requested" would be better than "desired" .

> Source/WebCore/rendering/ImageBySizeCache.h:47
> +    IntSize actualSize;
> +    IntSize desiredSize;
> +    float actualZoom;
> +    float desiredZoom;

What guarantees that the desired and actual sizes get reconciled at some point if they differ? There is no code in ImageBySizeCache to do that or assert that.

[WebKit Review Bot]

Comment on attachment 113027 [details]
Next try - v3

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

New failing tests:
fast/repaint/block-layout-inline-children-replaced.html
css2.1/20110323/background-intrinsic-007.htm
css1/text_properties/vertical_align.html
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-008.htm
css2.1/20110323/background-intrinsic-005.htm
fast/block/float/015.html
css2.1/20110323/background-intrinsic-009.htm
fast/block/positioning/replaced-inside-fixed-top-bottom.html
css2.1/20110323/background-intrinsic-002.htm
css2.1/20110323/background-intrinsic-001.htm
css2.1/20110323/background-intrinsic-004.htm
fast/backgrounds/size/contain-and-cover-zoomed.html
css2.1/20110323/background-intrinsic-003.htm

[Nikolas Zimmermann]

(In reply to comment #52)
> (From update of attachment 113027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113027&action=review
> 
> r=me, but some comments to ponder in the future.
> Notice how you are pulling data our of m_svgImageCache and then using it to call right back to the m_svgImageCache? This is a clear sign that all this code belongs to the image cache class.
As discussed with Antti, this can be done in a follow-up cleanup.

> > Source/WebCore/rendering/ImageBySizeCache.h:37
> > +        , desiredSize(newSize)
> 
> I think "requested" would be better than "desired" .
Fixed.
 
> What guarantees that the desired and actual sizes get reconciled at some point if they differ? There is no code in ImageBySizeCache to do that or assert that.
Will include that in a further patch.

[Nikolas Zimmermann]

Comment on attachment 113027 [details]
Next try - v3

Landed in r98852. Clearing flags.

[Nikolas Zimmermann]

Closing master bug. Still waiting for all bots to complete testing this.

[Csaba Osztrogonác]

(In reply to comment #55)
> (From update of attachment 113027 [details])
> Landed in r98852. Clearing flags.

It broke Qt Linux Release minimal build:
../../../Source/WebCore/rendering/RenderImage.cpp: In member function 'virtual WebCore::RenderBox* WebCore::RenderImage::embeddedContentBox() const':
../../../Source/WebCore/rendering/RenderImage.cpp:549: error: 'SVGImage' was not declared in this scope
../../../Source/WebCore/rendering/RenderImage.cpp:549: error: no matching function for call to 'static_pointer_cast(WTF::RefPtr<WebCore::Image>&)'

Could you fix minimal build please?

[Ryosuke Niwa]

Mac rebaseline done in http://trac.webkit.org/changeset/98901.

[Csaba Osztrogonác]

(In reply to comment #57)
> (In reply to comment #55)
> > (From update of attachment 113027 [details] [details])
> > Landed in r98852. Clearing flags.
> 
> It broke Qt Linux Release minimal build:
> ../../../Source/WebCore/rendering/RenderImage.cpp: In member function 'virtual WebCore::RenderBox* WebCore::RenderImage::embeddedContentBox() const':
> ../../../Source/WebCore/rendering/RenderImage.cpp:549: error: 'SVGImage' was not declared in this scope
> ../../../Source/WebCore/rendering/RenderImage.cpp:549: error: no matching function for call to 'static_pointer_cast(WTF::RefPtr<WebCore::Image>&)'
> 
> Could you fix minimal build please?

Buildfix landed in http://trac.webkit.org/changeset/98895