|
|
Created:
4 years ago by Manuel Rego Modified:
4 years ago CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-grid] Fix crash clamping grid lines
Avoid issues with very big values for the grid lines
clamping them during parsing time.
BUG=670241
TEST=CSSPropertyParserTest.GridPositionLimit*
Committed: https://crrev.com/784fc71c3d69b622afc93ff2005640f8592b0ff5
Cr-Commit-Position: refs/heads/master@{#439094}
Patch Set 1 #
Total comments: 2
Patch Set 2 : New version adding CSSPrimitiveValue::getIntValueClamped() #
Total comments: 2
Patch Set 3 : Use clampTo() #
Messages
Total messages: 21 (9 generated)
rego@igalia.com changed reviewers: + cbiesinger@chromium.org, jfernandez@igalia.com, svillar@igalia.com
jfernandez@igalia.com changed reviewers: + timloh@chromium.org
Added @timloh https://codereview.chromium.org/2546993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2546993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3024: if (numericValue) { Wouldn't be better to define new CSSPrimitiveValue APIs ? Something like - getIntValue(min, max) - clampIntValue(min, max)
We should consider this for merging into stable
Thanks for the review, uploaded new version. PTAL. https://codereview.chromium.org/2546993002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2546993002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3024: if (numericValue) { On 2016/12/02 12:28:03, jfernandez wrote: > Wouldn't be better to define new CSSPrimitiveValue APIs ? Something like > - getIntValue(min, max) Uploaded new version following this approach.
lgtm
The CQ bit was checked by rego@igalia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2546993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2546993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3026: numericValue->getIntValueClamped(-kGridMaxTracks, kGridMaxTracks), Instead of using this you can get the int value and then use clampTo()
The CQ bit was unchecked by rego@igalia.com
Thanks for the reviews, just uploaded a new version. https://codereview.chromium.org/2546993002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/2546993002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3026: numericValue->getIntValueClamped(-kGridMaxTracks, kGridMaxTracks), On 2016/12/16 10:58:33, svillar wrote: > Instead of using this you can get the int value and then use clampTo() Yeah sure. Good idea.
Much better indeed. lgtm
The CQ bit was checked by rego@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from jfernandez@igalia.com Link to the patchset: https://codereview.chromium.org/2546993002/#ps40001 (title: "Use clampTo()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481890138729520, "parent_rev": "f100a4c9933c4f945be4d4ed2270675a83ada87d", "commit_rev": "a06b20326fe4207f7bef2683d1358ac6668c071e"}
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix crash clamping grid lines Avoid issues with very big values for the grid lines clamping them during parsing time. BUG=670241 TEST=CSSPropertyParserTest.GridPositionLimit* ========== to ========== [css-grid] Fix crash clamping grid lines Avoid issues with very big values for the grid lines clamping them during parsing time. BUG=670241 TEST=CSSPropertyParserTest.GridPositionLimit* Review-Url: https://codereview.chromium.org/2546993002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [css-grid] Fix crash clamping grid lines Avoid issues with very big values for the grid lines clamping them during parsing time. BUG=670241 TEST=CSSPropertyParserTest.GridPositionLimit* Review-Url: https://codereview.chromium.org/2546993002 ========== to ========== [css-grid] Fix crash clamping grid lines Avoid issues with very big values for the grid lines clamping them during parsing time. BUG=670241 TEST=CSSPropertyParserTest.GridPositionLimit* Committed: https://crrev.com/784fc71c3d69b622afc93ff2005640f8592b0ff5 Cr-Commit-Position: refs/heads/master@{#439094} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/784fc71c3d69b622afc93ff2005640f8592b0ff5 Cr-Commit-Position: refs/heads/master@{#439094} |