Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1499443002:
SkImageShaderFactoryToName SkAlphaThresholdFilterFactoryToName (Closed)
|
Created:
5 years ago by hal.canary Modified:
5 years ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkImageShaderFactoryToName SkAlphaThresholdFilterFactoryToName
https://gold.skia.org/diff?test=image-shader&left=8807a80c69a5d565821432fe6a7b74ec&top=80222191bf0768b0fc62c8e05b58fb5f
https://gold.skia.org/diff?test=imagealphathreshold&left=fc3fbbfbd1b1e7ec1c33c00c6c22b9a8&top=493096aac6f44b91cd6522c6049d5a56
BUG=skia:4613
Committed: https://skia.googlesource.com/skia/+/ba923d38a5ea092aaa37967c5367ae19e2e4f017
Patch Set 1 #
Total comments: 6
Patch Set 2 : add SkAlphaThresholdFilterFactoryToName #Patch Set 3 : 2015-12-02 (Wednesday) 16:18:52 EST #
Total comments: 1
Patch Set 4 : 2015-12-07 (Monday) 13:45:37 EST #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== SkImageShaderFactoryToName ========== to ========== SkImageShaderFactoryToName BUG=skia:4613 ==========
halcanary@google.com changed reviewers: + reed@google.com
PTAL
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1499443002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1499443002/1
mtklein@google.com changed reviewers: + mtklein@google.com
Might want to update src/ports/SkGlobalInitialization_chromium.cpp too if you intend to serialize these from Chrome. If you do please heed its warnings about image filters and pictures, or we'll be flooded with fuzzer bugs and security vulnerabilities. https://codereview.chromium.org/1499443002/diff/1/src/ports/SkGlobalInitializ... File src/ports/SkGlobalInitialization_default.cpp (right): https://codereview.chromium.org/1499443002/diff/1/src/ports/SkGlobalInitializ... src/ports/SkGlobalInitialization_default.cpp:85: SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkImageShader) What's up with SkAlphaThresholdFilter? https://codereview.chromium.org/1499443002/diff/1/tests/FlattenableFactoryToN... File tests/FlattenableFactoryToName.cpp (right): https://codereview.chromium.org/1499443002/diff/1/tests/FlattenableFactoryToN... tests/FlattenableFactoryToName.cpp:1: /* I'm not sure I see the long term value of this test. https://codereview.chromium.org/1499443002/diff/1/tests/FlattenableFactoryToN... tests/FlattenableFactoryToName.cpp:36: test_flattenable(r, filter, "SkAlphaThresholdFilter()"); We don't really guarantee these particular strings.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1499443002/diff/1/src/ports/SkGlobalInitializ... File src/ports/SkGlobalInitialization_default.cpp (right): https://codereview.chromium.org/1499443002/diff/1/src/ports/SkGlobalInitializ... src/ports/SkGlobalInitialization_default.cpp:85: SK_DEFINE_FLATTENABLE_REGISTRAR_ENTRY(SkImageShader) On 2015/12/02 at 20:27:59, mtklein wrote: > What's up with SkAlphaThresholdFilter? It required more work. See patch 2. https://codereview.chromium.org/1499443002/diff/1/tests/FlattenableFactoryToN... File tests/FlattenableFactoryToName.cpp (right): https://codereview.chromium.org/1499443002/diff/1/tests/FlattenableFactoryToN... tests/FlattenableFactoryToName.cpp:1: /* On 2015/12/02 at 20:27:59, mtklein wrote: > I'm not sure I see the long term value of this test. I'm not sure how gold failed to catch these. https://codereview.chromium.org/1499443002/diff/1/tests/FlattenableFactoryToN... tests/FlattenableFactoryToName.cpp:36: test_flattenable(r, filter, "SkAlphaThresholdFilter()"); On 2015/12/02 at 20:28:00, mtklein wrote: > We don't really guarantee these particular strings. the string was for my ERRORF macro.
Description was changed from ========== SkImageShaderFactoryToName BUG=skia:4613 ========== to ========== SkImageShaderFactoryToName SkAlphaThresholdFilterFactoryToName https://gold.skia.org/diff?test=image-shader&left=8807a80c69a5d565821432fe6a7... BUG=skia:4613 ==========
Description was changed from ========== SkImageShaderFactoryToName SkAlphaThresholdFilterFactoryToName https://gold.skia.org/diff?test=image-shader&left=8807a80c69a5d565821432fe6a7... BUG=skia:4613 ========== to ========== SkImageShaderFactoryToName SkAlphaThresholdFilterFactoryToName https://gold.skia.org/diff?test=image-shader&left=8807a80c69a5d565821432fe6a7... https://gold.skia.org/diff?test=imagealphathreshold&left=fc3fbbfbd1b1e7ec1c33... BUG=skia:4613 ==========
halcanary@google.com changed reviewers: + chrome-security@google.com
I added src/ports/SkGlobalInitialization_chromium.cpp to this CL. I noted the broken GMs that this should fix in the CL description. I removed the unit test, since gold is already testing it. I added chrome-security@google.com, as suggested by SkGlobalInitialization_chromium.cpp. I believe that these CreateProc()s are as secure as all of the other ones we already expose to Chromium.
https://codereview.chromium.org/1499443002/diff/40001/gyp/effects.gypi File gyp/effects.gypi (right): https://codereview.chromium.org/1499443002/diff/40001/gyp/effects.gypi#newcode20 gyp/effects.gypi:20: '<(skia_src_path)/effects/SkAlphaThresholdFilterImpl.h', Let's do this part first before adding the types to SkGlobalInitialization, and keep this CL focused on that (in case we need to revert it).
On 2015/12/07 at 15:58:41, mtklein wrote: > https://codereview.chromium.org/1499443002/diff/40001/gyp/effects.gypi > File gyp/effects.gypi (right): > > https://codereview.chromium.org/1499443002/diff/40001/gyp/effects.gypi#newcode20 > gyp/effects.gypi:20: '<(skia_src_path)/effects/SkAlphaThresholdFilterImpl.h', > Let's do this part first before adding the types to SkGlobalInitialization, and keep this CL focused on that (in case we need to revert it). Done, rebased.
lgtm
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1499443002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1499443002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1499443002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1499443002/60001
Message was sent while issue was closed.
Description was changed from ========== SkImageShaderFactoryToName SkAlphaThresholdFilterFactoryToName https://gold.skia.org/diff?test=image-shader&left=8807a80c69a5d565821432fe6a7... https://gold.skia.org/diff?test=imagealphathreshold&left=fc3fbbfbd1b1e7ec1c33... BUG=skia:4613 ========== to ========== SkImageShaderFactoryToName SkAlphaThresholdFilterFactoryToName https://gold.skia.org/diff?test=image-shader&left=8807a80c69a5d565821432fe6a7... https://gold.skia.org/diff?test=imagealphathreshold&left=fc3fbbfbd1b1e7ec1c33... BUG=skia:4613 Committed: https://skia.googlesource.com/skia/+/ba923d38a5ea092aaa37967c5367ae19e2e4f017 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/ba923d38a5ea092aaa37967c5367ae19e2e4f017 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
