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

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

Created:
6 years, 10 months ago by Yoav Weiss
Modified:
6 years, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months ago (2013-10-09 21:03:15 UTC) #11
Yoav Weiss
Got everything working. > > Open issues: > * Canvas + SVG +2X shows some ...
6 years, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months ago (2013-10-13 00:12:13 UTC) #20
pdr.
LGTM!
6 years, 10 months ago (2013-10-13 00:12:44 UTC) #21
pdr.
Please update the change description to reflect the new approach before landing.
6 years, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months 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, 10 months ago (2013-10-13 18:37:20 UTC) #33
commit-bot: I haz the power
Change committed as 159544
6 years, 10 months 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, 10 months ago (2013-10-14 22:48:32 UTC) #35
commit-bot: I haz the power
6 years, 10 months 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