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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 23102018: Refactoring DrawLooper so that it can apply shadow effects as skia image filters (Closed)

Created:
7 years, 4 months ago by Justin Novosad
Modified:
6 years, 10 months ago
CC:
blink-reviews, jamesr, eae+blinkwatch, leviw+renderwatch, danakj, dglazkov+blink, Rik, f(malita), jchaffraix+rendering, pdr, Stephen Chennney, jeez, pdr., jbroman
Visibility:
Public.

Description

Refactoring DrawLooper so that it can apply shadow effects as skia image filters This fixes a bug where a bitmap's alpha channel was not being blurred when rendering 2D canvas shadows. BUG=100703 R=senorblanco TEST=LayoutTest fast/canvas/canvas-drawImage-shadow-blur.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156733 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156783

Patch Set 1 #

Patch Set 2 : updated baselines and expectations #

Total comments: 4

Patch Set 3 : Response to jbroman feedback #

Total comments: 2

Patch Set 4 : peppering some const #

Total comments: 1

Patch Set 5 : a bit more const #

Patch Set 6 : Adding missing adoptRefs #

Unified diffs Side-by-side diffs Delta from patch set Stats (+225 lines, -137 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/fast/canvas/canvas-draw-canvas-on-canvas-shadow-expected.txt View 1 1 chunk +12 lines, -12 lines 0 comments Download
M LayoutTests/fast/canvas/canvas-drawImage-shadow-expected.txt View 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/fast/canvas/script-tests/canvas-draw-canvas-on-canvas-shadow.js View 1 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/fast/canvas/script-tests/canvas-drawImage-shadow.js View 1 chunk +7 lines, -0 lines 0 comments Download
D LayoutTests/platform/mac/virtual/gpu/fast/canvas/canvas-draw-canvas-on-canvas-shadow-expected.txt View 1 1 chunk +0 lines, -57 lines 0 comments Download
M Source/core/platform/graphics/DrawLooper.h View 1 2 3 4 2 chunks +34 lines, -5 lines 0 comments Download
M Source/core/platform/graphics/DrawLooper.cpp View 1 2 3 4 2 chunks +127 lines, -34 lines 0 comments Download
M Source/core/platform/graphics/GraphicsContext.h View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M Source/core/platform/graphics/GraphicsContext.cpp View 1 2 3 4 5 3 chunks +9 lines, -9 lines 0 comments Download
M Source/core/platform/graphics/GraphicsContextState.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/core/platform/graphics/skia/NativeImageSkia.cpp View 2 chunks +9 lines, -1 line 0 comments Download
M Source/core/rendering/EllipsisBox.cpp View 1 2 3 4 5 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/rendering/InlineTextBox.cpp View 1 2 3 4 5 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/rendering/RenderBoxModelObject.cpp View 1 2 3 4 5 1 chunk +3 lines, -3 lines 0 comments Download
M Source/core/rendering/svg/SVGInlineTextBox.cpp View 1 2 3 4 5 1 chunk +4 lines, -4 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
Justin Novosad
PTAL
7 years, 4 months ago (2013-08-23 17:37:08 UTC) #1
Justin Novosad
On 2013/08/23 17:37:08, junov wrote: > PTAL This might require some pixel rebaselines, let's see ...
7 years, 4 months ago (2013-08-23 17:38:13 UTC) #2
Justin Novosad
New patch. Updated text baselines and added NeedsRebaseline to TestExpectations
7 years, 4 months ago (2013-08-23 18:29:37 UTC) #3
danakj
7 years, 4 months ago (2013-08-23 18:30:25 UTC) #4
jbroman
A couple drive-by comments since I added most of WebCore::DrawLooper a month or two ago. ...
7 years, 4 months ago (2013-08-23 19:59:37 UTC) #5
Justin Novosad
New Patch: -Applied corrections based on jbroman's feedback -Added Pass/Timeout expectation on virtual/gpu/fast/canvas/canvas-drawImage-shadow.html (Mesa, Y ...
7 years, 3 months ago (2013-08-26 17:43:07 UTC) #6
Stephen White
https://codereview.chromium.org/23102018/diff/16001/Source/core/platform/graphics/DrawLooper.cpp File Source/core/platform/graphics/DrawLooper.cpp (left): https://codereview.chromium.org/23102018/diff/16001/Source/core/platform/graphics/DrawLooper.cpp#oldcode50 Source/core/platform/graphics/DrawLooper.cpp:50: SkDrawLooper* DrawLooper::skDrawLooper() const Nit: if you wanted to keep ...
7 years, 3 months ago (2013-08-26 17:57:15 UTC) #7
Justin Novosad
https://codereview.chromium.org/23102018/diff/16001/Source/core/platform/graphics/DrawLooper.cpp File Source/core/platform/graphics/DrawLooper.cpp (left): https://codereview.chromium.org/23102018/diff/16001/Source/core/platform/graphics/DrawLooper.cpp#oldcode50 Source/core/platform/graphics/DrawLooper.cpp:50: SkDrawLooper* DrawLooper::skDrawLooper() const On 2013/08/26 17:57:16, Stephen White wrote: ...
7 years, 3 months ago (2013-08-26 18:46:41 UTC) #8
Stephen White
https://codereview.chromium.org/23102018/diff/23001/Source/core/platform/graphics/DrawLooper.h File Source/core/platform/graphics/DrawLooper.h (right): https://codereview.chromium.org/23102018/diff/23001/Source/core/platform/graphics/DrawLooper.h#newcode77 Source/core/platform/graphics/DrawLooper.h:77: bool shouldUseImageFilterToDrawBitmap(const SkBitmap&); Maybe this one should be const ...
7 years, 3 months ago (2013-08-26 19:04:22 UTC) #9
Justin Novosad
On 2013/08/26 19:04:22, Stephen White wrote: > https://codereview.chromium.org/23102018/diff/23001/Source/core/platform/graphics/DrawLooper.h > File Source/core/platform/graphics/DrawLooper.h (right): > > https://codereview.chromium.org/23102018/diff/23001/Source/core/platform/graphics/DrawLooper.h#newcode77 ...
7 years, 3 months ago (2013-08-26 19:17:58 UTC) #10
Stephen White
LGTM
7 years, 3 months ago (2013-08-26 19:21:08 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/junov@chromium.org/23102018/28001
7 years, 3 months ago (2013-08-26 19:22:20 UTC) #12
commit-bot: I haz the power
Change committed as 156733
7 years, 3 months ago (2013-08-26 23:39:25 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/junov@chromium.org/23102018/44001
7 years, 3 months ago (2013-08-27 15:03:38 UTC) #14
Justin Novosad
On 2013/08/26 23:39:25, I haz the power (commit-bot) wrote: > Change committed as 156733 Change ...
7 years, 3 months ago (2013-08-27 15:03:40 UTC) #15
commit-bot: I haz the power
Change committed as 156783
7 years, 3 months ago (2013-08-27 16:59:54 UTC) #16
Justin Novosad
On 2013/08/27 16:59:54, I haz the power (commit-bot) wrote: > Change committed as 156783 going ...
7 years, 3 months ago (2013-08-28 18:36:17 UTC) #17
Stephen White
6 years, 10 months ago (2014-01-29 21:49:37 UTC) #18
Message was sent while issue was closed. 
On 2013/08/28 18:36:17, junov wrote:
> On 2013/08/27 16:59:54, I haz the power (commit-bot) wrote:
> > Change committed as 156783
> 
> going to revert this again.  Performance is just not acceptable.  Need to
> figure-out why.

Note that once https://code.google.com/p/skia/source/detail?r=13221 rolls into
Chrome, it should be possible to re-try this approach.

Powered by Google App Engine
This is Rietveld 408576698