Issue 1829093002: Use sk_sp-based APIs for SkColorFilter

Issue 1829093002: Use sk_sp-based APIs for SkColorFilter (Closed)

Created:
4 years, 9 months ago by tomhudson

Modified:
4 years, 8 months ago

Reviewers:
danakj, f(malita), sky

CC:
chromium-reviews, blink-reviews-platform-graphics_chromium.org, dshwang, fs, sievers+watch_chromium.org, kinuko+watch, kouhei+svg_chromium.org, Stephen Chennney, rwlbuis, krit, drott+blinkwatch_chromium.org, jam, Justin Novosad, jbauman+watch_chromium.org, Rik, darin-cc_chromium.org, blink-reviews-paint_chromium.org, blink-reviews, gyuyoung2, pdr+svgwatchlist_chromium.org, jbroman, pdr+graphicswatchlist_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, Ian Vollick, tfarina, slimming-paint-reviews_chromium.org, f(malita), cc-bugs_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Use sk_sp-based APIs for SkColorFilter Favor Skia's smart pointer based interface over raw pointers. Switch Chrome (but not Blink) to sk_sp<SkColorFilter> fields/params where feasible. BUG=skia:5077, 597622 R=fmalita@chromium.org,danakj@chromium.org TBR=sky@chromium.org CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/1f8d413031d8680ac02c4954c02e55fe3f6a7174 Cr-Commit-Position: refs/heads/master@{#383982}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : Much SkUserConfig churn, many rebase #

Patch Set 4 : Bad rebase #

Total comments: 19

Patch Set 5 : Respond to reviews #

Total comments: 1

Created: 4 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+71 lines, -84 lines)			Patch
M	cc/blink/web_display_item_list_impl.cc	View		1 chunk	+2 lines, -3 lines	0 comments	Download
M	cc/layers/heads_up_display_layer_impl.cc	View	1	1 chunk	+9 lines, -10 lines	0 comments	Download
M	cc/output/gl_renderer_unittest.cc	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	cc/output/render_surface_filters.cc	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	cc/output/renderer_pixeltest.cc	View	1 2 3 4	2 chunks	+8 lines, -8 lines	0 comments	Download
M	cc/playback/compositing_display_item.h	View		3 chunks	+4 lines, -4 lines	0 comments	Download
M	cc/playback/compositing_display_item.cc	View	1 2 3 4	4 chunks	+5 lines, -5 lines	0 comments	Download
M	cc/playback/display_item_list_unittest.cc	View		1 chunk	+3 lines, -3 lines	0 comments	Download
M	cc/trees/layer_tree_host_pixeltest_filters.cc	View	1 2 3 4	2 chunks	+5 lines, -6 lines	0 comments	Download
M	chrome/browser/ui/views/tabs/tab_strip.cc	View		1 chunk	+2 lines, -3 lines	0 comments	Download
M	content/browser/compositor/surface_utils.cc	View		1 chunk	+1 line, -3 lines	0 comments	Download
M	skia/config/SkUserConfig.h	View	1 2 3	1 chunk	+0 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/core/paint/SVGPaintContext.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp	View		1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/ColorSpace.cpp	View		2 chunks	+2 lines, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/DrawLooperBuilder.cpp	View		1 chunk	+1 line, -2 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/GraphicsContextState.h	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/GraphicsContextState.cpp	View	1 2 3 4	1 chunk	+3 lines, -3 lines	1 comment	Download
M	third_party/WebKit/Source/platform/graphics/filters/FEColorMatrix.cpp	View	1 2 3 4	3 chunks	+4 lines, -4 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/filters/FEComponentTransfer.cpp	View	1 2 3 4	1 chunk	+2 lines, -3 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/filters/FEFlood.cpp	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/filters/FilterEffect.cpp	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	third_party/WebKit/Source/platform/graphics/filters/SourceAlpha.cpp	View	1 2 3 4	1 chunk	+1 line, -1 line	0 comments	Download
M	ui/compositor/compositing_recorder.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	ui/gfx/skbitmap_operations.cc	View		1 chunk	+2 lines, -3 lines	0 comments	Download
M	ui/gfx/skia_util.cc	View		1 chunk	+3 lines, -4 lines	0 comments	Download

Messages

Total messages: 28 (16 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829093002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829093002/20001

4 years, 9 months ago (2016-03-25 20:14:32 UTC) #9

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/149835) ios_rel_device_gn on ...

4 years, 9 months ago (2016-03-25 20:17:05 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/1829093002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829093002/60001

4 years, 9 months ago (2016-03-25 21:19:18 UTC) #13

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/136154)

4 years, 9 months ago (2016-03-25 23:29:14 UTC) #15

danakj

LGTM but we can avoid some raw-pointers still: https://codereview.chromium.org/1829093002/diff/60001/cc/blink/web_display_item_list_impl.cc File cc/blink/web_display_item_list_impl.cc (right): https://codereview.chromium.org/1829093002/diff/60001/cc/blink/web_display_item_list_impl.cc#newcode166 cc/blink/web_display_item_list_impl.cc:166: SkColorFilter* ...

4 years, 9 months ago (2016-03-26 00:49:52 UTC) #16

f(malita)

https://codereview.chromium.org/1829093002/diff/60001/cc/output/gl_renderer_unittest.cc File cc/output/gl_renderer_unittest.cc (right): https://codereview.chromium.org/1829093002/diff/60001/cc/output/gl_renderer_unittest.cc#newcode1422 cc/output/gl_renderer_unittest.cc:1422: SkColorFilter::MakeMatrixFilterRowMajor255(matrix).release(), NULL)); It's unfortunate that SkColorFilterImageFilter doesn't have a ...

4 years, 9 months ago (2016-03-26 14:16:50 UTC) #17

tomhudson

sky@, please review chrome/browser/ui/views/. https://codereview.chromium.org/1829093002/diff/60001/cc/blink/web_display_item_list_impl.cc File cc/blink/web_display_item_list_impl.cc (right): https://codereview.chromium.org/1829093002/diff/60001/cc/blink/web_display_item_list_impl.cc#newcode166 cc/blink/web_display_item_list_impl.cc:166: SkColorFilter* color_filter) { On 2016/03/26 ...

4 years, 8 months ago (2016-03-28 13:11:17 UTC) #19

sky@, please review chrome/browser/ui/views/.

https://codereview.chromium.org/1829093002/diff/60001/cc/blink/web_display_it...
File cc/blink/web_display_item_list_impl.cc (right):

https://codereview.chromium.org/1829093002/diff/60001/cc/blink/web_display_it...
cc/blink/web_display_item_list_impl.cc:166: SkColorFilter* color_filter) {
On 2016/03/26 00:49:51, danakj wrote:
> can you make this take sk_sp instead?

This inherits from blink::WebDisplayItemList, and and this point we aren't
exposing sk_sp<> in any Blink APIs.

https://codereview.chromium.org/1829093002/diff/60001/cc/output/gl_renderer_u...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1829093002/diff/60001/cc/output/gl_renderer_u...
cc/output/gl_renderer_unittest.cc:1422:
SkColorFilter::MakeMatrixFilterRowMajor255(matrix).release(), NULL));
On 2016/03/26 14:16:48, f(malita) wrote:
> It's unfortunate that SkColorFilterImageFilter doesn't have a sk_sp factory
yet
> :(
> 
> This pattern leaks (see the ASAN bot): old style/rawptr Create() doesn't
> transfer the ref but grabs a new one.  So we can't release the color filter,
> because no one is adopting it.  Let's leave it in a sk_sp instead.
> 
> Safe pattern:
> 
>   sk_sp<SkColorFilter> color_filter =
> SkColorFilter::MakeMatrixFilterRowMajor255();
>   SkColorFilterImageFilter::Create(color_filter.get()...);
> 
> Leaky pattern:
> 
>  
>
SkColorFilterImageFilter::Create(SkColorFilter::MakeMatrixFilterRowMajor255().release()...);

Done.

Thanks for the thorough review and explanation. I'd just changed all those
things over, not understanding the reference flow well enough, but easy enough
to change back.

https://codereview.chromium.org/1829093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/GraphicsContextState.cpp
(right):

https://codereview.chromium.org/1829093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/GraphicsContextState.cpp:162:
m_fillPaint.setColorFilter(toSkSp(m_colorFilter));
On 2016/03/26 14:16:49, f(malita) wrote:
> Can we switch m_colorFilter & m_colorFilter to sk_sp?  I don't think we pass
> them out of GCS except in raw form.

There's only one SkColorFilter? Typo?

https://codereview.chromium.org/1829093002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/filters/FEColorMatrix.cpp
(right):

https://codereview.chromium.org/1829093002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/filters/FEColorMatrix.cpp:166: auto
filter = createColorFilter(m_type, m_values);
On 2016/03/26 14:16:49, f(malita) wrote:
> Dana pointed out on a different CL that Chromium style doesn't approve of auto
> for smart pointers.
> 
> I'm not familiar with the original motivation, but this pattern did seem kind
of
> fragile to me when seen in Skia.
> 
> We should probably stick to Chromium style in Blink (and maybe consider for
Skia
> also).

Done.

Yay! I'm not fond of auto but having seen it introduced so heavily in Skia
recently thought it was part of the house style.

f(malita)

lgtm https://codereview.chromium.org/1829093002/diff/60001/third_party/WebKit/Source/platform/graphics/GraphicsContextState.cpp File third_party/WebKit/Source/platform/graphics/GraphicsContextState.cpp (right): https://codereview.chromium.org/1829093002/diff/60001/third_party/WebKit/Source/platform/graphics/GraphicsContextState.cpp#newcode162 third_party/WebKit/Source/platform/graphics/GraphicsContextState.cpp:162: m_fillPaint.setColorFilter(toSkSp(m_colorFilter)); On 2016/03/28 13:11:17, tomhudson wrote: > On ...

4 years, 8 months ago (2016-03-28 13:39:32 UTC) #20

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829093002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829093002/80001

4 years, 8 months ago (2016-03-30 13:31:45 UTC) #24

commit-bot: I haz the power

4 years, 8 months ago (2016-03-30 16:05:00 UTC) #28

Message was sent while issue was closed.

Patchset 5 (id:??) landed as
https://crrev.com/1f8d413031d8680ac02c4954c02e55fe3f6a7174
Cr-Commit-Position: refs/heads/master@{#383982}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages