Bug 47156 - CSS 2.1 failure: background-intrinsic-*
Summary: CSS 2.1 failure: background-intrinsic-*
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 64974 67087 69416 70314 70945 70949 71226 71253
Blocks: 47141 16281 34521 42944 45439 52045
  Show dependency treegraph
 
Reported: 2010-10-04 21:49 PDT by Simon Fraser (smfr)
Modified: 2011-11-02 08:14 PDT (History)
15 users (show)

See Also:


Attachments
background-intrinsic-001.htm (1.71 KB, text/html)
2010-10-04 21:49 PDT, Simon Fraser (smfr)
no flags Details
Patch (682.92 KB, patch)
2011-07-23 05:00 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (683.42 KB, patch)
2011-07-23 08:13 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v3 (683.81 KB, patch)
2011-07-23 09:38 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v4 (686.17 KB, patch)
2011-07-23 10:33 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v5 (870.25 KB, patch)
2011-07-26 08:39 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v6 (869.72 KB, patch)
2011-07-27 01:56 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v7 (1.07 MB, patch)
2011-07-27 05:19 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v8 (1.07 MB, patch)
2011-08-02 01:28 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Draft patch (73.57 KB, patch)
2011-10-07 04:15 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Updated patch (1.94 MB, patch)
2011-10-17 12:38 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch - v1 (1.07 MB, patch)
2011-10-18 05:25 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Final patch - v2 (1.07 MB, patch)
2011-10-18 09:17 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch - v3 (deleted)
2011-10-26 07:33 PDT, Nikolas Zimmermann
koivisto: review+
Details | Formatted Diff | Diff
Next try - v1 (deleted)
2011-10-30 02:56 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Next try - v2 (deleted)
2011-10-30 07:16 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Next try - v3 (deleted)
2011-10-31 02:58 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-10-04 21:49:14 PDT
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
Comment 1 Simon Fraser (smfr) 2011-01-08 13:50:20 PST
Probably we don't handle images that just have an intrinsic ratio.
Comment 2 Simon Fraser (smfr) 2011-01-08 17:05:52 PST
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.
Comment 3 Nikolas Zimmermann 2011-01-09 04:19:30 PST
(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...)
Comment 4 Nikolas Zimmermann 2011-07-04 08:59:25 PDT
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.
Comment 5 Nikolas Zimmermann 2011-07-23 05:00:08 PDT
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).
Comment 6 WebKit Review Bot 2011-07-23 05:04:12 PDT
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.
Comment 7 WebKit Review Bot 2011-07-23 05:15:25 PDT
Comment on attachment 101807 [details]
Patch

Attachment 101807 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9231378
Comment 8 Nikolas Zimmermann 2011-07-23 08:13:04 PDT
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.
Comment 9 WebKit Review Bot 2011-07-23 08:17:55 PDT
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.
Comment 10 WebKit Review Bot 2011-07-23 08:29:15 PDT
Comment on attachment 101812 [details]
Patch v2

Attachment 101812 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9231414
Comment 11 Nikolas Zimmermann 2011-07-23 09:38:50 PDT
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...
Comment 12 WebKit Review Bot 2011-07-23 10:02:27 PDT
Comment on attachment 101813 [details]
Patch v3

Attachment 101813 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9230458
Comment 13 Nikolas Zimmermann 2011-07-23 10:33:28 PDT
Created attachment 101815 [details]
Patch v4

Chromium WebCore builds now, attempt to fix WebKit chromium (DragImageTest).
Comment 14 Nikolas Zimmermann 2011-07-23 10:34:52 PDT
CCing Adam Roben, who might know whats going on with the win slave, why it doesn't apply the patch.
Comment 15 WebKit Review Bot 2011-07-23 17:11:52 PDT
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
Comment 16 Early Warning System Bot 2011-07-24 20:58:52 PDT
Comment on attachment 101815 [details]
Patch v4

Attachment 101815 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9231867
Comment 17 Adam Roben (:aroben) 2011-07-25 05:24:59 PDT
(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?
Comment 18 Adam Barth 2011-07-25 11:07:09 PDT
> 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.
Comment 19 Nikolas Zimmermann 2011-07-26 08:39:36 PDT
Created attachment 102011 [details]
Patch v5

Working on fixing regressions... Leaving out r? flag, until these are resolved.
Comment 20 WebKit Review Bot 2011-07-26 08:44:57 PDT
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.
Comment 21 Early Warning System Bot 2011-07-26 09:20:43 PDT
Comment on attachment 102011 [details]
Patch v5

Attachment 102011 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9251365
Comment 22 WebKit Review Bot 2011-07-26 13:59:43 PDT
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
Comment 23 Nikolas Zimmermann 2011-07-27 01:56:09 PDT
Created attachment 102107 [details]
Patch v6

Finished ChangeLog, patch should be ready for review now.
Comment 24 WebKit Review Bot 2011-07-27 02:01:51 PDT
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.
Comment 25 Nikolas Zimmermann 2011-07-27 05:19:40 PDT
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.
Comment 26 WebKit Review Bot 2011-07-27 07:21:19 PDT
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
Comment 27 Nikolas Zimmermann 2011-08-02 01:28:14 PDT
Created attachment 102624 [details]
Patch v8

Updated to ToT. Anyone wants to review?
Comment 28 WebKit Review Bot 2011-08-02 21:27:28 PDT
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
Comment 29 Nikolas Zimmermann 2011-08-04 05:30:55 PDT
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 :-)
Comment 30 Nikolas Zimmermann 2011-09-28 11:18:09 PDT
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.
Comment 31 Nikolas Zimmermann 2011-10-05 06:39:31 PDT
Comment on attachment 102624 [details]
Patch v8

Clearing flags. A new patch will be uploaded once 69416 is in.
Comment 32 Nikolas Zimmermann 2011-10-07 04:15:39 PDT
Created attachment 110122 [details]
Draft patch

To aid Anttis review on bug 69416, uploading an updated master patch, w/o test changes.
Comment 33 WebKit Review Bot 2011-10-07 04:18:05 PDT
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.
Comment 34 WebKit Review Bot 2011-10-07 10:25:09 PDT
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
Comment 35 Nikolas Zimmermann 2011-10-17 12:38:03 PDT
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.
Comment 36 WebKit Review Bot 2011-10-17 12:41:21 PDT
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.
Comment 37 Nikolas Zimmermann 2011-10-18 05:25:13 PDT
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.
Comment 38 Antti Koivisto 2011-10-18 06:19:49 PDT
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.
Comment 39 WebKit Review Bot 2011-10-18 07:13:41 PDT
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
Comment 40 Nikolas Zimmermann 2011-10-18 07:54:30 PDT
(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.
Comment 41 Nikolas Zimmermann 2011-10-18 09:17:32 PDT
Created attachment 111450 [details]
Final patch - v2

Fixed Anttis comments.
Comment 42 Nikolas Zimmermann 2011-10-26 07:33:09 PDT
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.
Comment 43 Antti Koivisto 2011-10-26 08:13:21 PDT
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
Comment 44 Nikolas Zimmermann 2011-10-26 08:31:43 PDT
(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.
Comment 45 Julien Chaffraix 2011-10-26 12:00:14 PDT
Reverted r98483 for reason:

Change is causing some crashes and ASSERT.

Committed r98508: <http://trac.webkit.org/changeset/98508>
Comment 46 Nikolas Zimmermann 2011-10-30 02:56:32 PDT
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.
Comment 47 WebKit Review Bot 2011-10-30 03:02:49 PDT
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.
Comment 48 WebKit Review Bot 2011-10-30 04:37:18 PDT
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
Comment 49 Nikolas Zimmermann 2011-10-30 07:16:30 PDT
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...
Comment 50 WebKit Review Bot 2011-10-30 11:56:49 PDT
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
Comment 51 Nikolas Zimmermann 2011-10-31 02:58:39 PDT
Created attachment 113027 [details]
Next try - v3

Stupid me, only included LayoutTests in the last diff, forgot Source/.
Comment 52 Antti Koivisto 2011-10-31 03:41:24 PDT
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.
Comment 53 WebKit Review Bot 2011-10-31 06:50:02 PDT
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
Comment 54 Nikolas Zimmermann 2011-10-31 07:13:17 PDT
(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.
Comment 55 Nikolas Zimmermann 2011-10-31 07:13:57 PDT
Comment on attachment 113027 [details]
Next try - v3

Landed in r98852. Clearing flags.
Comment 56 Nikolas Zimmermann 2011-10-31 08:34:25 PDT
Closing master bug. Still waiting for all bots to complete testing this.
Comment 57 Csaba Osztrogonác 2011-10-31 15:50:06 PDT
(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?
Comment 58 Ryosuke Niwa 2011-10-31 16:12:26 PDT
Mac rebaseline done in http://trac.webkit.org/changeset/98901.
Comment 59 Csaba Osztrogonác 2011-10-31 17:03:10 PDT
(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