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

Issue 1008893002: [CSS Shapes] Normalize margin box size (Closed)

Created:
5 years, 9 months ago by rwlbuis
Modified:
5 years, 9 months ago
CC:
blink-reviews, blink-reviews-rendering, blink-reviews-style_chromium.org, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

[CSS Shapes] Normalize margin box size Since the reference box may end up with negative widths/heights due to negative margins, clamp to zero for the reference box calculation in the MarginBox case. Since fixing this (466942) ends up hitting the ASSERT from bug 450625, fix that by also clamping the floatMarginBoxWidth to zero since a float with negative width has no float area. BUG=450625, 466942 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192180

Patch Set 1 #

Patch Set 2 : Remove printfs #

Patch Set 3 : Clamp the reference box size to zero #

Total comments: 2

Patch Set 4 : Address review comments #

Patch Set 5 : Use std::max instead of clamp #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -0 lines) Patch
A LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-ellipse-negative-margins.html View 1 chunk +20 lines, -0 lines 0 comments Download
A + LayoutTests/fast/shapes/shape-outside-floats/shape-outside-floats-ellipse-negative-margins-expected.txt View 0 chunks +-1 lines, --1 lines 0 comments Download
M Source/core/layout/shapes/ShapeOutsideInfo.cpp View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download

Messages

Total messages: 17 (6 generated)
rwlbuis
PTAL
5 years, 9 months ago (2015-03-13 22:52:15 UTC) #2
rwlbuis
5 years, 9 months ago (2015-03-14 02:34:31 UTC) #4
bjonesbe
For the reference box size, you should clamp to 0, not abs. A negative height ...
5 years, 9 months ago (2015-03-16 16:21:43 UTC) #5
bjonesbe
https://codereview.chromium.org/1008893002/diff/60001/Source/core/layout/shapes/ShapeOutsideInfo.cpp File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right): https://codereview.chromium.org/1008893002/diff/60001/Source/core/layout/shapes/ShapeOutsideInfo.cpp#newcode59 Source/core/layout/shapes/ShapeOutsideInfo.cpp:59: if (newReferenceBoxLogicalSize.isEmpty()) { Sorry I missed this earlier, but ...
5 years, 9 months ago (2015-03-16 22:26:35 UTC) #8
rwlbuis
On 2015/03/16 22:26:35, bjonesbe wrote: > https://codereview.chromium.org/1008893002/diff/60001/Source/core/layout/shapes/ShapeOutsideInfo.cpp > File Source/core/layout/shapes/ShapeOutsideInfo.cpp (right): > > https://codereview.chromium.org/1008893002/diff/60001/Source/core/layout/shapes/ShapeOutsideInfo.cpp#newcode59 > ...
5 years, 9 months ago (2015-03-17 20:33:10 UTC) #9
Bem Jones-Bey (adobe)
lgtm
5 years, 9 months ago (2015-03-17 20:35:55 UTC) #10
rwlbuis
@leviw PTAL
5 years, 9 months ago (2015-03-17 20:47:07 UTC) #12
leviw_travelin_and_unemployed
LGTM.
5 years, 9 months ago (2015-03-19 15:31:41 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1008893002/100001
5 years, 9 months ago (2015-03-19 15:32:03 UTC) #15
rwlbuis
On 2015/03/19 15:31:41, leviw wrote: > LGTM. Thanks! Two bugs squashed for the price of ...
5 years, 9 months ago (2015-03-19 15:52:59 UTC) #16
commit-bot: I haz the power
5 years, 9 months ago (2015-03-19 16:59:41 UTC) #17
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192180

Powered by Google App Engine
This is Rietveld 408576698