Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(400)

Issue 1756763004: Merge image sizing algorithms (Closed)

Created:
4 years, 9 months ago by davve
Modified:
4 years, 9 months ago
Reviewers:
pdr., Yoav Weiss, fs
CC:
fs, blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, dshwang, krit, eae+blinkwatch, f(malita), gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+svgwatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, slimming-paint-reviews_chromium.org, szager+layoutwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Merge image sizing algorithms Let users of StyleImage use StyleImage::imageSize() to get the image size instead of fetching intrinsic information and calculating the size outside StyleImage. This let's us remove the sizing algorithm in LayoutBoxModelObject::calculateImageIntrinsicDimensions. By passing along the default object size to StyleImage::imageSize, we can remove the sizing algorithm in LayoutBoxModelObject and reuse the one in SVGImage instead for the one image type that needs the complicated sizing algorithm. Simpler algorithms can remain simple, e.g. for generated images with no fixed size, the default object size is returned unmodified. SVGImage::concreteObjectSize almost had the necessary bits to support full sizing of SVG images within a style context, i.e. through StyleImage. The only missing bit was the the contain constraint on the default object size added by this patch. Some zoom juggling needed since the provided default object size is sometimes zoomed and SVGImage has no notion of zoom. Thus the zoom is removed before calling SVGImage::concreteObjectSize() and reapplied on the result afterwards. Background images and other decorative images should never respect the exif rotation[1], so StyleImage::imageSize now requests the image size from ImageResource without exif rotation applied. Presumably StyleImage::imageSize() was broken but unused in this regard before. In contrast to LBMO::calculateImageIntrinsicDimensions(), StyleImage::imageSize returns the size for layout, i.e. the size compensated for the image scale factor. This fixes two hidpi bugs, one for list item marker images and one for shape-outside. BUG=581357, 591935, 591939, 592888, 592886 Committed: https://crrev.com/3c7b1342b32dd6174efa9a47a4b476c940462b9c Cr-Commit-Position: refs/heads/master@{#379801}

Patch Set 1 #

Patch Set 2 : Bring cross-faded images along #

Patch Set 3 : Image orientation and scale factor fudging. #

Patch Set 4 : Fix c&p error #

Patch Set 5 : Extend test to cover svg and non-svg case #

Total comments: 11

Patch Set 6 : Break out common code to StyleImage and fold constant into call #

Total comments: 3

Patch Set 7 : Share zoom code between generated and fetched style images #

Patch Set 8 : Drop calculate prefix from concreteObjectSize member function #

Total comments: 7

Patch Set 9 : Image scaling tests and small fixes #

Patch Set 10 : Fix unused variable 'styleImage' in release #

Unified diffs Side-by-side diffs Delta from patch set Stats (+228 lines, -170 lines) Patch
A third_party/WebKit/LayoutTests/css3/images/cross-fade-svg-size.html View 1 2 3 4 1 chunk +38 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/css3/images/cross-fade-svg-size-expected.html View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/fast/dom/HTMLImageElement/image-natural-width-height-svg-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
A third_party/WebKit/LayoutTests/fast/hidpi/image-set-list-style-image.html View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/hidpi/image-set-list-style-image-expected.html View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/hidpi/image-set-shape-outside.html View 1 2 3 4 5 6 7 8 1 chunk +11 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/hidpi/image-set-shape-outside-expected.html View 1 2 3 4 5 6 7 8 1 chunk +9 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/svg/as-list-image/svg-list-image-intrinsic-size-zoom.html View 1 chunk +11 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/core.gypi View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSCrossfadeValue.h View 1 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/css/CSSCrossfadeValue.cpp View 1 2 3 4 5 6 7 3 chunks +8 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/css/CSSImageGeneratorValue.h View 1 2 chunks +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/css/CSSImageGeneratorValue.cpp View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBoxModelObject.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -87 lines 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h View 3 4 5 6 7 8 1 chunk +5 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/LayoutListMarker.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/shapes/ShapeOutsideInfo.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/BackgroundImageGeometry.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/paint/BoxPainter.cpp View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/NinePieceImageGrid.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/paint/NinePieceImagePainter.cpp View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleFetchedImage.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/style/StyleFetchedImage.cpp View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleFetchedImageSet.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp View 1 2 3 4 5 6 7 8 1 chunk +10 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleGeneratedImage.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp View 1 2 3 4 5 6 1 chunk +6 lines, -18 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleImage.h View 1 2 3 4 5 6 3 chunks +5 lines, -1 line 0 comments Download
A third_party/WebKit/Source/core/style/StyleImage.cpp View 1 2 3 4 5 6 7 1 chunk +37 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/style/StyleInvalidImage.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/style/StylePendingImage.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/svg/graphics/SVGImage.h View 1 2 3 4 5 6 7 4 chunks +10 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp View 1 2 3 4 5 6 7 4 chunks +22 lines, -8 lines 0 comments Download

Messages

Total messages: 38 (16 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1756763004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1756763004/80001
4 years, 9 months ago (2016-03-03 12:16:51 UTC) #3
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 9 months ago (2016-03-03 13:44:23 UTC) #6
davve
I think this is starting to get ready for review now. +yoav: I'm very uncertain ...
4 years, 9 months ago (2016-03-03 13:59:15 UTC) #8
Yoav Weiss
On 2016/03/03 13:59:15, David Vest wrote: > I think this is starting to get ready ...
4 years, 9 months ago (2016-03-03 14:50:48 UTC) #9
fs
https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h File third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h (right): https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h#newcode58 third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h:58: scaledImageSize.scale(1 / m_styleImage->imageScaleFactor()); Won't this double-scale if m_styleImage is ...
4 years, 9 months ago (2016-03-03 15:22:54 UTC) #10
fs
https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h File third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h (right): https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h#newcode58 third_party/WebKit/Source/core/layout/LayoutImageResourceStyleImage.h:58: scaledImageSize.scale(1 / m_styleImage->imageScaleFactor()); On 2016/03/03 at 15:22:53, fs wrote: ...
4 years, 9 months ago (2016-03-03 15:24:43 UTC) #11
davve
https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Source/core/paint/BoxPainter.cpp File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Source/core/paint/BoxPainter.cpp#newcode143 third_party/WebKit/Source/core/paint/BoxPainter.cpp:143: && !layer.image()->imageSize(&imageClient, imageClient.style()->effectiveZoom(), LayoutSize()).isEmpty() On 2016/03/03 15:22:53, fs wrote: ...
4 years, 9 months ago (2016-03-04 06:49:43 UTC) #12
fs
On 2016/03/04 at 06:49:43, davve wrote: ... > https://codereview.chromium.org/1756763004/diff/80001/third_party/WebKit/Source/core/style/StyleFetchedImage.cpp > File third_party/WebKit/Source/core/style/StyleFetchedImage.cpp (right): > > ...
4 years, 9 months ago (2016-03-04 09:00:56 UTC) #14
davve
https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp File third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp (right): https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp#newcode52 third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp:52: FloatSize unzoomedDefaultObjectSize(defaultObjectSize); On 2016/03/04 09:00:56, fs wrote: > (Strange ...
4 years, 9 months ago (2016-03-04 13:02:56 UTC) #15
fs
https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp File third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp (right): https://codereview.chromium.org/1756763004/diff/100001/third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp#newcode52 third_party/WebKit/Source/core/style/StyleGeneratedImage.cpp:52: FloatSize unzoomedDefaultObjectSize(defaultObjectSize); On 2016/03/04 at 13:02:56, David Vest wrote: ...
4 years, 9 months ago (2016-03-04 13:06:57 UTC) #16
Yoav Weiss
Hey David! Comments below. I don't mind the orientation change, but it'd be good to ...
4 years, 9 months ago (2016-03-07 11:16:07 UTC) #17
davve
Replying with what I know about what's in LayoutTests already. Will look more into testing ...
4 years, 9 months ago (2016-03-07 13:07:38 UTC) #18
davve
https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp File third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp (right): https://codereview.chromium.org/1756763004/diff/140001/third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp#newcode103 third_party/WebKit/Source/core/style/StyleFetchedImageSet.cpp:103: // border-image, etc.) > The tests: > > fast/hidpi/image-set-background-repeat-without-size.html ...
4 years, 9 months ago (2016-03-07 14:26:10 UTC) #19
davve
I moved back the scaling compensation to StyleFetchedImageSet::imageSize() so that imageSize always returns what layout ...
4 years, 9 months ago (2016-03-07 16:53:40 UTC) #20
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1756763004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1756763004/160001
4 years, 9 months ago (2016-03-08 07:32:04 UTC) #24
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/104011) chromeos_daisy_chromium_compile_only_ng on ...
4 years, 9 months ago (2016-03-08 07:42:54 UTC) #26
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1756763004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1756763004/180001
4 years, 9 months ago (2016-03-08 07:50:19 UTC) #28
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 9 months ago (2016-03-08 09:11:31 UTC) #30
Yoav Weiss
LGTM
4 years, 9 months ago (2016-03-08 09:11:49 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1756763004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1756763004/180001
4 years, 9 months ago (2016-03-08 10:19:11 UTC) #34
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 9 months ago (2016-03-08 10:30:21 UTC) #36
commit-bot: I haz the power
4 years, 9 months ago (2016-03-08 10:32:12 UTC) #38
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/3c7b1342b32dd6174efa9a47a4b476c940462b9c
Cr-Commit-Position: refs/heads/master@{#379801}

Powered by Google App Engine
This is Rietveld 408576698