WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
47156
CSS 2.1 failure: background-intrinsic-*
https://bugs.webkit.org/show_bug.cgi?id=47156
Summary
CSS 2.1 failure: background-intrinsic-*
Simon Fraser (smfr)
Reported
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
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
Show Obsolete
(16)
View All
Add attachment
proposed patch, testcase, etc.
Simon Fraser (smfr)
Comment 1
2011-01-08 13:50:20 PST
Probably we don't handle images that just have an intrinsic ratio.
Simon Fraser (smfr)
Comment 2
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.
Nikolas Zimmermann
Comment 3
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...)
Nikolas Zimmermann
Comment 4
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.
Nikolas Zimmermann
Comment 5
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).
WebKit Review Bot
Comment 6
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.
WebKit Review Bot
Comment 7
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
Nikolas Zimmermann
Comment 8
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.
WebKit Review Bot
Comment 9
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.
WebKit Review Bot
Comment 10
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
Nikolas Zimmermann
Comment 11
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...
WebKit Review Bot
Comment 12
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
Nikolas Zimmermann
Comment 13
2011-07-23 10:33:28 PDT
Created
attachment 101815
[details]
Patch v4 Chromium WebCore builds now, attempt to fix WebKit chromium (DragImageTest).
Nikolas Zimmermann
Comment 14
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.
WebKit Review Bot
Comment 15
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
Early Warning System Bot
Comment 16
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
Adam Roben (:aroben)
Comment 17
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?
Adam Barth
Comment 18
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.
Nikolas Zimmermann
Comment 19
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.
WebKit Review Bot
Comment 20
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.
Early Warning System Bot
Comment 21
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
WebKit Review Bot
Comment 22
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
Nikolas Zimmermann
Comment 23
2011-07-27 01:56:09 PDT
Created
attachment 102107
[details]
Patch v6 Finished ChangeLog, patch should be ready for review now.
WebKit Review Bot
Comment 24
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.
Nikolas Zimmermann
Comment 25
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.
WebKit Review Bot
Comment 26
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
Nikolas Zimmermann
Comment 27
2011-08-02 01:28:14 PDT
Created
attachment 102624
[details]
Patch v8 Updated to ToT. Anyone wants to review?
WebKit Review Bot
Comment 28
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
Nikolas Zimmermann
Comment 29
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 :-)
Nikolas Zimmermann
Comment 30
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.
Nikolas Zimmermann
Comment 31
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.
Nikolas Zimmermann
Comment 32
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.
WebKit Review Bot
Comment 33
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.
WebKit Review Bot
Comment 34
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
Nikolas Zimmermann
Comment 35
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.
WebKit Review Bot
Comment 36
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.
Nikolas Zimmermann
Comment 37
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.
Antti Koivisto
Comment 38
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.
WebKit Review Bot
Comment 39
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
Nikolas Zimmermann
Comment 40
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.
Nikolas Zimmermann
Comment 41
2011-10-18 09:17:32 PDT
Created
attachment 111450
[details]
Final patch - v2 Fixed Anttis comments.
Nikolas Zimmermann
Comment 42
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.
Antti Koivisto
Comment 43
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
Nikolas Zimmermann
Comment 44
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.
Julien Chaffraix
Comment 45
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
>
Nikolas Zimmermann
Comment 46
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.
WebKit Review Bot
Comment 47
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.
WebKit Review Bot
Comment 48
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
Nikolas Zimmermann
Comment 49
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...
WebKit Review Bot
Comment 50
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
Nikolas Zimmermann
Comment 51
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/.
Antti Koivisto
Comment 52
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.
WebKit Review Bot
Comment 53
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
Nikolas Zimmermann
Comment 54
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.
Nikolas Zimmermann
Comment 55
2011-10-31 07:13:57 PDT
Comment on
attachment 113027
[details]
Next try - v3 Landed in
r98852
. Clearing flags.
Nikolas Zimmermann
Comment 56
2011-10-31 08:34:25 PDT
Closing master bug. Still waiting for all bots to complete testing this.
Csaba Osztrogonác
Comment 57
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?
Ryosuke Niwa
Comment 58
2011-10-31 16:12:26 PDT
Mac rebaseline done in
http://trac.webkit.org/changeset/98901
.
Csaba Osztrogonác
Comment 59
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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug