Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(38)

Issue 25105004: Use srcset's resource pixel density to determine intrinsic size (Closed)

Created:
6 years, 1 month ago by Yoav Weiss
Modified:
6 years, 1 month ago
Reviewers:
pdr.
CC:
blink-reviews, dglazkov+blink, Nate Chapin, adamk+blink_chromium.org, gavinp+loader_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@cleantests
Visibility:
Public.

Description

Use srcset's resource pixel density to determine intrinsic size According to the spec "When an img element has a current pixel density that is not 1.0, the element's image data must be treated as if its resolution, in device pixels per CSS pixels, was the current pixel density." I've added that support using the following changes: * HTMLSrcsetParser now returns the image candidate to HTMLImageElement. * HTMLImageElement passes the devicePixelRatio data to RenderImage, which stores it. * Bitmap images are scaled using the devicePixelRatio at RenderImageResource's intrinsicSize() and imageSize(). * SVG images are scaled using the devicePixelRatio at RenderReplaced::computeAspectRatioInformationForRenderBox. * Canvas support added at CanvasRenderingContext2D::sizeFor. Other changes: * Removed useless dumpAsText from tests * Modified one of the tests to use testRunner.dumpResourceRequestCallbacks() instead of internals.isPreloaded(), since isPreloaded wasn't reporting what the test was intended to test. * Added some tests to TestExpectations because of bugs discovered during this CL work. BUG=299556 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159544 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159622

Patch Set 1 #

Patch Set 2 : Bitmap images working #

Patch Set 3 : The "RenderImage option". Bitmap images and canvas working. #

Patch Set 4 : Same as previous patch, that was corrupted #

Patch Set 5 : PNG, SVG an canvas working #

Patch Set 6 : Everything working #

Patch Set 7 : Fixed expected test results #

Total comments: 22

Patch Set 8 : Addressed review comments #

Patch Set 9 : Moved SVG scaling. Renamed vars. #

Patch Set 10 : Added rebaselines to TestExpectations #

Patch Set 11 : Rebased #

Patch Set 12 : Fixed TestExpectations. #

Patch Set 13 : More rebaselines in TestExpectations #

Patch Set 14 : Fixed crash+rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1640 lines, -88 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +51 lines, -8 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-2x.html View 1 2 3 4 2 chunks +4 lines, -6 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-2x-expected.txt View 1 2 3 4 5 6 7 1 chunk +11 lines, -1 line 0 comments Download
A + LayoutTests/fast/hidpi/image-srcset-change-resource-dpr.html View 1 2 3 4 5 6 7 2 chunks +13 lines, -17 lines 0 comments Download
A + LayoutTests/fast/hidpi/image-srcset-change-resource-dpr-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-data-src.html View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-data-srcset.html View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-dpr-zoom.html View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-fraction.html View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-ignore-double-descriptor.html View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-ignore-double-descriptor-expected.txt View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-intrinsic-size.html View 1 2 1 chunk +30 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-intrinsic-size-expected.txt View 1 2 1 chunk +9 lines, -0 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-invalid-descriptor.html View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-invalid-descriptor-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-invalid-inputs-correct-src.html View 1 2 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-invalid-inputs-correct-src-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-only-src-attribute.html View 1 2 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-only-src-attribute-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-png.html View 1 2 3 4 1 chunk +64 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-png-canvas.html View 1 2 3 4 5 1 chunk +48 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-relative-svg.html View 1 2 3 4 1 chunk +37 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-relative-svg-canvas.html View 1 2 3 4 5 1 chunk +35 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-relative-svg-canvas-2x.html View 1 2 3 4 5 1 chunk +35 lines, -0 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-simple-2x.html View 1 2 1 chunk +2 lines, -6 lines 0 comments Download
M LayoutTests/fast/hidpi/image-srcset-simple-2x-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-svg.html View 1 2 3 4 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-svg-canvas.html View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-svg-canvas-2x.html View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-svg-expected.png View 1 2 3 4 Binary file 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-svg-expected.txt View 1 2 3 4 1 chunk +458 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/image-srcset-svg2.html View 1 2 3 4 1 chunk +36 lines, -0 lines 0 comments Download
M LayoutTests/fast/hidpi/resources/green-200-px-square.png View 1 2 Binary file 0 comments Download
A LayoutTests/fast/hidpi/resources/relativesrcset.svg View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/resources/srcset.png View 1 2 Binary file 0 comments Download
A LayoutTests/fast/hidpi/resources/srcset.svg View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/fast/hidpi/resources/srcset-helper.js View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
A LayoutTests/fast/hidpi/resources/svg_canvas_helper.js View 1 2 3 4 5 1 chunk +20 lines, -0 lines 0 comments Download
A LayoutTests/fast/hidpi/resources/svg_tests.css View 1 2 3 4 1 chunk +20 lines, -0 lines 0 comments Download
M LayoutTests/platform/linux/fast/hidpi/image-set-as-background-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/hidpi/image-set-background-dynamic-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/hidpi/image-set-border-image-dynamic-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/hidpi/image-set-border-image-simple-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/hidpi/image-set-in-content-dynamic-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/hidpi/image-set-out-of-order-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/hidpi/image-set-simple-expected.png View 1 2 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-png-canvas-expected.png View 1 2 3 4 5 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-png-canvas-expected.txt View 1 2 3 4 5 1 chunk +62 lines, -0 lines 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-png-expected.png View 1 2 3 4 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-png-expected.txt View 1 2 3 4 1 chunk +140 lines, -0 lines 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-relative-svg-canvas-2x-expected.png View 1 2 3 4 5 6 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-relative-svg-canvas-2x-expected.txt View 1 2 3 4 5 6 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-relative-svg-canvas-expected.png View 1 2 3 4 5 6 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-relative-svg-canvas-expected.txt View 1 2 3 4 5 6 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-relative-svg-expected.png View 1 2 3 4 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-relative-svg-expected.txt View 1 2 3 4 1 chunk +75 lines, -0 lines 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-svg-canvas-2x-expected.png View 1 2 3 4 5 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-svg-canvas-2x-expected.txt View 1 2 3 4 5 1 chunk +36 lines, -0 lines 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-svg-canvas-expected.png View 1 2 3 4 5 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-svg-canvas-expected.txt View 1 2 3 4 5 1 chunk +37 lines, -0 lines 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-svg-expected.png View 1 2 3 4 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-svg-expected.txt View 1 2 3 4 1 chunk +77 lines, -0 lines 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-svg2-expected.png View 1 2 3 4 Binary file 0 comments Download
A LayoutTests/platform/linux/fast/hidpi/image-srcset-svg2-expected.txt View 1 2 3 4 1 chunk +75 lines, -0 lines 0 comments Download
M Source/core/html/HTMLImageElement.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLImageElement.cpp View 1 2 3 4 5 6 7 8 9 10 3 chunks +11 lines, -2 lines 0 comments Download
M Source/core/html/canvas/CanvasRenderingContext2D.cpp View 1 2 3 4 5 6 7 8 9 10 5 chunks +21 lines, -9 lines 0 comments Download
M Source/core/html/parser/HTMLSrcsetParser.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/parser/HTMLSrcsetParser.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +7 lines, -4 lines 0 comments Download
M Source/core/rendering/RenderImage.h View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderImage.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/RenderImageResource.h View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/rendering/RenderImageResource.cpp View 1 2 3 4 5 6 7 8 2 chunks +11 lines, -0 lines 0 comments Download
M Source/core/rendering/RenderReplaced.cpp View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -1 line 0 comments Download

Messages

Total messages: 36 (0 generated)
Yoav Weiss
6 years, 1 month ago (2013-09-27 21:34:06 UTC) #1
Yoav Weiss
Following pdr's advice, I added some SVG tests, to make sure intrinsic size works well ...
6 years, 1 month ago (2013-09-30 16:21:40 UTC) #2
pdr.
Not LGTM, but I have ideas on how to fix this :) The core issue ...
6 years, 1 month ago (2013-10-02 02:53:13 UTC) #3
Yoav Weiss
On 2013/10/02 02:53:13, pdr wrote: > Not LGTM, but I have ideas on how to ...
6 years, 1 month ago (2013-10-06 22:22:57 UTC) #4
pdr.
On 2013/10/06 22:22:57, Yoav Weiss wrote: > I've started implementing that with https://codereview.chromium.org/25627006/. > I ...
6 years, 1 month ago (2013-10-06 22:41:59 UTC) #5
Yoav Weiss
On 2013/10/06 22:41:59, pdr wrote: > On 2013/10/06 22:22:57, Yoav Weiss wrote: > > I've ...
6 years, 1 month ago (2013-10-07 21:20:21 UTC) #6
pdr.
On 2013/10/07 21:20:21, Yoav Weiss wrote: > That's actually not the example I'm using. I'm ...
6 years, 1 month ago (2013-10-08 06:36:04 UTC) #7
Yoav Weiss
> I created a few testcases > at http://pr.gg/srcset.html. These reflect my understanding of srcset ...
6 years, 1 month ago (2013-10-08 07:42:53 UTC) #8
Yoav Weiss
> > > > If you can post an updated patch with the testcase that's ...
6 years, 1 month ago (2013-10-08 16:42:20 UTC) #9
Yoav Weiss
Moved to store the intrinsicSizeFactor in RenderImage, following eseidel's suggestion. I've also added pdr's SVG ...
6 years, 1 month ago (2013-10-09 04:52:56 UTC) #10
Yoav Weiss
With latest patch, PNG and SVG images are working fine. Canvas works as well, with ...
6 years, 1 month ago (2013-10-09 21:03:15 UTC) #11
Yoav Weiss
Got everything working. > > Open issues: > * Canvas + SVG +2X shows some ...
6 years, 1 month ago (2013-10-10 21:08:43 UTC) #12
pdr.
This patch is fantastic Yoav, thank you for seeing it through. I think this is ...
6 years, 1 month ago (2013-10-11 05:17:33 UTC) #13
Yoav Weiss
Thanks for the review! My comments below. One more question, is it OK if I ...
6 years, 1 month ago (2013-10-11 07:22:31 UTC) #14
Yoav Weiss
> One more question, is it OK if I rebaseline the few SVG tests that ...
6 years, 1 month ago (2013-10-11 08:45:04 UTC) #15
Yoav Weiss
Addressed the review comments and uploaded the modified patch. > https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/RenderImage.cpp#newcode618 > Source/core/rendering/RenderImage.cpp:618: if (cachedImage ...
6 years, 1 month ago (2013-10-11 17:05:46 UTC) #16
pdr.
On 2013/10/11 07:22:31, Yoav Weiss wrote: > https://codereview.chromium.org/25105004/diff/91001/Source/core/html/canvas/CanvasRenderingContext2D.cpp#newcode1215 > Source/core/html/canvas/CanvasRenderingContext2D.cpp:1215: static LayoutSize > sizeFor(HTMLImageElement* image, ...
6 years, 1 month ago (2013-10-11 18:26:38 UTC) #17
pdr.
On 2013/10/11 17:05:46, Yoav Weiss wrote: > Addressed the review comments and uploaded the modified ...
6 years, 1 month ago (2013-10-11 18:29:38 UTC) #18
Yoav Weiss
Renamed intrinsicSizeFactor to imageDevicePixelRatio & ScaledSize to imageSizeAfterDevicePixelRatio > > I now have a better ...
6 years, 1 month ago (2013-10-11 22:18:07 UTC) #19
pdr.
On 2013/10/11 22:18:07, Yoav Weiss wrote: > Removed SVG scaling from intrinsicSize() into > RenderReplaced::computeAspectRatioInformationForRenderBox, ...
6 years, 1 month ago (2013-10-13 00:12:13 UTC) #20
pdr.
LGTM!
6 years, 1 month ago (2013-10-13 00:12:44 UTC) #21
pdr.
Please update the change description to reflect the new approach before landing.
6 years, 1 month ago (2013-10-13 00:14:06 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/134001
6 years, 1 month ago (2013-10-13 08:16:51 UTC) #23
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=10809
6 years, 1 month ago (2013-10-13 09:57:17 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/147001
6 years, 1 month ago (2013-10-13 14:51:43 UTC) #25
commit-bot: I haz the power
Failed to apply patch for Source/core/html/canvas/CanvasRenderingContext2D.cpp: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
6 years, 1 month ago (2013-10-13 14:52:09 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/160001
6 years, 1 month ago (2013-10-13 16:09:59 UTC) #27
commit-bot: I haz the power
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_presubmit&number=8757
6 years, 1 month ago (2013-10-13 16:21:55 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/196001
6 years, 1 month ago (2013-10-13 16:37:03 UTC) #29
pdr.
On 2013/10/13 16:37:03, I haz the power (commit-bot) wrote: > CQ is trying da patch. ...
6 years, 1 month ago (2013-10-13 17:38:59 UTC) #30
Yoav Weiss
Yeah. Fixed the presubmit failures. Now hoping the rebaslines worked. :) On Sun, Oct 13, ...
6 years, 1 month ago (2013-10-13 17:41:41 UTC) #31
commit-bot: I haz the power
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_rel&number=10847
6 years, 1 month ago (2013-10-13 18:18:20 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/213001
6 years, 1 month ago (2013-10-13 18:37:20 UTC) #33
commit-bot: I haz the power
Change committed as 159544
6 years, 1 month ago (2013-10-13 19:40:05 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/230001
6 years, 1 month ago (2013-10-14 22:48:32 UTC) #35
commit-bot: I haz the power
6 years, 1 month ago (2013-10-15 01:38:46 UTC) #36
Message was sent while issue was closed.
Change committed as 159622

Powered by Google App Engine
This is Rietveld 408576698