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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 659913003: Background image should clamp to minimum size(1, 1) after scale. (Closed)

Created:
6 years, 2 months ago by pals
Modified:
6 years, 1 month ago
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Background image should clamp to minimum size(1, 1) after scale. While one of the values of background-size is auto we have to use the appropriate scale to maintain our aspect ratio. In this step we might end up getting new image width or height as zero. In this case we should clamp to minimum value as 1. BUG=424048 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185238

Patch Set 1 #

Total comments: 3

Patch Set 2 : Ref test #

Patch Set 3 : Fixed the regressions #

Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -12 lines) Patch
M LayoutTests/TestExpectations View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/fast/backgrounds/background-img-repeat.html View 1 chunk +14 lines, -0 lines 0 comments Download
A LayoutTests/fast/backgrounds/background-img-repeat-expected.html View 1 1 chunk +13 lines, -0 lines 0 comments Download
A LayoutTests/fast/backgrounds/resources/bgimg1x50.png View 1 2 Binary file 0 comments Download
M LayoutTests/fast/backgrounds/size/zero.html View 1 2 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/platform/linux/fast/backgrounds/size/zero-expected.png View 1 2 Binary file 0 comments Download
M LayoutTests/platform/linux/fast/backgrounds/size/zero-expected.txt View 1 2 1 chunk +8 lines, -7 lines 0 comments Download
M Source/core/paint/BoxPainter.cpp View 1 2 1 chunk +12 lines, -4 lines 0 comments Download

Messages

Total messages: 27 (11 generated)
pals
Please review.
6 years, 2 months ago (2014-10-16 06:07:40 UTC) #2
chrishtr
Can you point me to the code that is scaling down one of the dimensions ...
6 years, 2 months ago (2014-10-16 20:56:18 UTC) #3
pals
On 2014/10/16 20:56:18, chrishtr wrote: > Can you point me to the code that is ...
6 years, 2 months ago (2014-10-17 05:02:42 UTC) #4
chrishtr
https://codereview.chromium.org/659913003/diff/1/Source/core/paint/BoxPainter.cpp File Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/659913003/diff/1/Source/core/paint/BoxPainter.cpp#newcode827 Source/core/paint/BoxPainter.cpp:827: applySubPixelHeuristicForTileSize(tileSize, positioningAreaSize); How about moving applySubPixelHeuristicForTileSize down to line ...
6 years, 2 months ago (2014-10-17 19:02:17 UTC) #5
chrishtr
I'm about to go on vacation for a week. Levi can help you finish this ...
6 years, 2 months ago (2014-10-17 21:22:46 UTC) #7
leviw_travelin_and_unemployed
Sanjoy, did you explore moving the call that Chris mentioned?
6 years, 2 months ago (2014-10-24 23:36:16 UTC) #8
pals
On 2014/10/24 23:36:16, leviw wrote: > Sanjoy, did you explore moving the call that Chris ...
6 years, 1 month ago (2014-11-04 06:52:37 UTC) #9
leviw_travelin_and_unemployed
One last issue with the test, but I do think this approach is reasonable. https://codereview.chromium.org/659913003/diff/1/LayoutTests/fast/backgrounds/background-img-repeat.html ...
6 years, 1 month ago (2014-11-05 19:33:42 UTC) #10
pals
Yes, it should be a ref test. Done. Please take a look. https://codereview.chromium.org/659913003/diff/1/LayoutTests/fast/backgrounds/background-img-repeat.html File LayoutTests/fast/backgrounds/background-img-repeat.html ...
6 years, 1 month ago (2014-11-06 06:08:08 UTC) #11
chrishtr
lgtm
6 years, 1 month ago (2014-11-06 18:02:30 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/659913003/20001
6 years, 1 month ago (2014-11-06 18:03:05 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/32481)
6 years, 1 month ago (2014-11-06 19:09:21 UTC) #16
pals
The regressions was caused as background-size:(0, 0) was clamped to (1, 1) by clamp clampToMinimumSize(IntSize(1, ...
6 years, 1 month ago (2014-11-12 05:33:11 UTC) #23
chrishtr
lgtm
6 years, 1 month ago (2014-11-12 17:23:51 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/659913003/160001
6 years, 1 month ago (2014-11-12 18:40:23 UTC) #26
commit-bot: I haz the power
6 years, 1 month ago (2014-11-12 19:43:45 UTC) #27
Message was sent while issue was closed. 
Committed patchset #3 (id:160001) as 185238

Powered by Google App Engine
This is Rietveld 408576698