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

Issue 1087283002: Rework the checks for too-few values in feColorMatrix filter. (Closed)

Created:
5 years, 8 months ago by Stephen Chennney
Modified:
5 years, 8 months ago
CC:
blink-reviews, Rik, danakj, Dominik Röttsches, dshwang, krit, ed+blinkwatch_opera.com, f(malita), fs, gyuyoung2, jbroman, Justin Novosad, kouhei+svg_chromium.org, pdr+graphicswatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Rework the checks for too-few values in feColorMatrix filter. Revert the prevous fix because it was flat-out wrong for some filters. Here we add checks at all points the values are used, because it is impossible to enforce an always-valid m_values vector inside the FEColorMatrix object. For example, we need to support any ordering of setAttribute('type', ...) and setAttribute('values', ...), but the valid number of values depends on the type. We couldn't set the type from "hueRotate" to "matrix", for example, without first adding more values than are necessary for the "hueRotate". That's a bad user experience. R=fs@opera.com,ed@opera.com,pdr@chromium.org BUG=468519 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193911

Patch Set 1 : Correct patch #

Total comments: 12

Patch Set 2 : Review comments and tests! #

Total comments: 4

Patch Set 3 : Better tests #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+103 lines, -18 lines) Patch
A LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html View 1 2 1 chunk +39 lines, -0 lines 2 comments Download
A + LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1-expected.txt View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A LayoutTests/svg/filters/feColorMatrix-setAttribute-crash2.html View 1 2 1 chunk +39 lines, -0 lines 0 comments Download
A + LayoutTests/svg/filters/feColorMatrix-setAttribute-crash2-expected.txt View 1 0 chunks +-1 lines, --1 lines 0 comments Download
M Source/core/svg/SVGFEColorMatrixElement.cpp View 1 chunk +2 lines, -6 lines 0 comments Download
M Source/platform/graphics/filters/FEColorMatrix.h View 1 chunk +3 lines, -0 lines 0 comments Download
M Source/platform/graphics/filters/FEColorMatrix.cpp View 1 5 chunks +22 lines, -14 lines 0 comments Download

Messages

Total messages: 21 (6 generated)
Stephen Chennney
This is not ready to land because the test does not crash in run-webkit-tests. But ...
5 years, 8 months ago (2015-04-15 17:37:41 UTC) #1
Stephen Chennney
Oops, not the right patch.
5 years, 8 months ago (2015-04-15 17:38:23 UTC) #2
pdr.
https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right): https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html#newcode16 LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:16: setTimeout(changeValues, 100); What's this timer waiting on? https://codereview.chromium.org/1087283002/diff/20001/Source/platform/graphics/filters/FEColorMatrix.cpp File ...
5 years, 8 months ago (2015-04-15 18:34:22 UTC) #4
fs
On 2015/04/15 17:37:41, Stephen Chenney wrote: > This is not ready to land because the ...
5 years, 8 months ago (2015-04-15 22:07:27 UTC) #5
Stephen Chennney
New patch imminent. https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right): https://codereview.chromium.org/1087283002/diff/20001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html#newcode16 LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:16: setTimeout(changeValues, 100); On 2015/04/15 22:07:27, fs ...
5 years, 8 months ago (2015-04-16 18:12:19 UTC) #6
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1087283002/60001
5 years, 8 months ago (2015-04-16 18:20:00 UTC) #9
pdr.
https://codereview.chromium.org/1087283002/diff/60001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right): https://codereview.chromium.org/1087283002/diff/60001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html#newcode24 LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:24: setTimeout(changeValues, 200); setTimeout 200ms? Can this be requestAnimationFrame(changeValues);? https://codereview.chromium.org/1087283002/diff/60001/Source/platform/graphics/filters/FEColorMatrix.cpp ...
5 years, 8 months ago (2015-04-16 18:54:54 UTC) #10
Stephen Chennney
Test updated to use requestAnimationFrame. It's a little more opaque, I suppose, but more portable. ...
5 years, 8 months ago (2015-04-16 19:14:14 UTC) #11
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1087283002/80001
5 years, 8 months ago (2015-04-16 19:14:51 UTC) #13
pdr.
LGTM with or without my suggestion https://codereview.chromium.org/1087283002/diff/80001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right): https://codereview.chromium.org/1087283002/diff/80001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html#newcode25 LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:25: requestAnimationFrame(forcePaint); Does this ...
5 years, 8 months ago (2015-04-16 19:19:17 UTC) #14
Stephen Chennney
I think I've described the behavior. https://codereview.chromium.org/1087283002/diff/80001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html File LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html (right): https://codereview.chromium.org/1087283002/diff/80001/LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html#newcode25 LayoutTests/svg/filters/feColorMatrix-setAttribute-crash1.html:25: requestAnimationFrame(forcePaint); On 2015/04/16 ...
5 years, 8 months ago (2015-04-16 19:30:32 UTC) #15
fs
lgtm Thanks for fixing!
5 years, 8 months ago (2015-04-16 21:49:26 UTC) #16
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 8 months ago (2015-04-16 22:32:43 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1087283002/80001
5 years, 8 months ago (2015-04-16 23:39:50 UTC) #20
commit-bot: I haz the power
5 years, 8 months ago (2015-04-16 23:40:46 UTC) #21
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193911

Powered by Google App Engine
This is Rietveld 408576698