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

Issue 2690583002: Make cc/paint have concrete types (Closed)

Created:
3 years, 10 months ago by enne (OOO)
Modified:
3 years, 9 months ago
CC:
ajuma+watch_chromium.org, apavlov+blink_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, caseq+blink_chromium.org, cc-bugs_chromium.org, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, f(malita), jam, jbauman+watch_chromium.org, jbroman, kalyank, kinuko+watch, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, mac-reviews_chromium.org, mcasas+watch+vc_chromium.org, mlamouri+watch-content_chromium.org, nona+watch_chromium.org, pdr+graphicswatchlist_chromium.org, pfeldman+blink_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org, rwlbuis, Stephen Chennney, shuchen+watch_chromium.org, James Su, tfarina, yusukes+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make cc/paint have concrete types This changes PaintCanvas, PaintFlags, PaintSurface, and PaintRecorder to all be real types (that forward to Skia types internally). PaintShader is left as-is for now. This will force callers to use the correct types in the rest of Chromium as the internals of these classes are rewritten in future patches. This code also changes a number of callers elsewhere in the codebase that want to wrap an SkCanvas in a PaintCanvas. As SkCanvas has no constructor that takes an SkCanvas*, this change had to wait for this final conversion patch. In general, if code wants to raster directly into a bitmap with local code, it should use Skia directly. If code wants to raster into a bitmap with paint callers, it can wrap that bitmap in a PaintCanvas. Similarly, if code wants to go into an accelerated SkSurface, it should wrap that surface's canvas in a PaintCanvas (as blink html canvas code does). BUG=671433 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2690583002 Cr-Commit-Position: refs/heads/master@{#455840} Committed: https://chromium.googlesource.com/chromium/src/+/d2501577f2df0764eda66614ddf429e230062562

Patch Set 1 #

Patch Set 2 : Concrete canvas/record/surface compiling #

Patch Set 3 : fully concrete #

Patch Set 4 : Rebase #

Patch Set 5 : Remove more functions #

Patch Set 6 : Rebase #

Patch Set 7 : Remove drawpicture #

Patch Set 8 : make paint record concrete #

Patch Set 9 : Rebase #

Patch Set 10 : Fix refcounting #

Patch Set 11 : Rebase #

Patch Set 12 : Rebase #

Patch Set 13 : Test win build #

Patch Set 14 : fix paint direct params #

Patch Set 15 : more win fixes, comment fixes #

Patch Set 16 : Rebase #

Patch Set 17 : Compile woes #

Patch Set 18 : More win fixes #

Patch Set 19 : rebase, more win fixes #

Patch Set 20 : More win fixes #

Patch Set 21 : Rebase #

Patch Set 22 : chromeos / win build fixes #

Patch Set 23 : No alloc for PaintCanvas in PaintRecorder #

Patch Set 24 : Rebase #

Patch Set 25 : Rebase on top of cleanup patch #

Patch Set 26 : remove unneeded ash change #

Patch Set 27 : Small cleanups #

Patch Set 28 : Rebase #

Total comments: 19

Patch Set 29 : Fix chromeos build #

Total comments: 22

Patch Set 30 : Fix canvas crashes #

Total comments: 3

Patch Set 31 : Address review comments #

Patch Set 32 : Rebase #

Patch Set 33 : Add missing file #

Total comments: 8

Patch Set 34 : Address reviews, fix printing issue #

Total comments: 2

Patch Set 35 : Rebase #

Patch Set 36 : Fix chromeos #

Patch Set 37 : Rebase #

Patch Set 38 : PaintRecord as typedef, fixup playback calls #

Unified diffs Side-by-side diffs Delta from patch set Stats (+931 lines, -142 lines) Patch
M ash/common/shelf/overflow_button.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +1 line, -1 line 0 comments Download
M ash/common/shelf/shelf_button.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 1 chunk +2 lines, -2 lines 0 comments Download
M cc/layers/painted_overlay_scrollbar_layer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 1 chunk +7 lines, -7 lines 0 comments Download
M cc/paint/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +8 lines, -1 line 0 comments Download
M cc/paint/paint_canvas.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 2 chunks +425 lines, -9 lines 0 comments Download
M cc/paint/paint_canvas.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 3 chunks +31 lines, -13 lines 0 comments Download
M cc/paint/paint_flags.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +201 lines, -3 lines 0 comments Download
M cc/paint/paint_recorder.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +59 lines, -2 lines 0 comments Download
A cc/paint/paint_recorder.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +12 lines, -0 lines 0 comments Download
M cc/paint/paint_shader.h View 1 chunk +2 lines, -1 line 0 comments Download
M cc/paint/paint_surface.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 1 chunk +47 lines, -2 lines 0 comments Download
A cc/paint/paint_surface.cc View 1 2 1 chunk +14 lines, -0 lines 0 comments Download
M content/child/browser_font_resource_trusted.cc View 3 chunks +5 lines, -4 lines 0 comments Download
M content/renderer/gpu/gpu_benchmarking_extension.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M media/renderers/skcanvas_video_renderer_unittest.cc View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M printing/pdf_metafile_skia.cc View 1 2 3 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/paint/SVGShapePainter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/svg/graphics/SVGImageTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/exported/WebFont.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 10 chunks +10 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +5 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 16 chunks +39 lines, -32 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/DeferredImageDecoderTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 5 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/GraphicsContext.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 8 chunks +10 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/RecordingImageBufferSurface.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.h View 1 2 3 4 5 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/UnacceleratedImageBufferSurface.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 2 chunks +11 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/gpu/AcceleratedImageBufferSurface.h View 1 2 3 4 5 2 chunks +3 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/gpu/AcceleratedImageBufferSurface.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 2 chunks +5 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/graphics/paint/PaintCanvas.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/PageWidgetDelegate.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/web/WebLocalFrameImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 2 chunks +2 lines, -2 lines 0 comments Download
M ui/gfx/paint_vector_icon_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 3 chunks +9 lines, -2 lines 0 comments Download

Messages

Total messages: 119 (82 generated)
enne (OOO)
This is waiting on a dependency patch for trivial type changes, but is otherwise ready ...
3 years, 9 months ago (2017-02-28 22:07:11 UTC) #23
enne (OOO)
(ping?)
3 years, 9 months ago (2017-03-02 18:19:55 UTC) #27
danakj
https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_canvas.h File cc/paint/paint_canvas.h (right): https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_canvas.h#newcode410 cc/paint/paint_canvas.h:410: std::unique_ptr<SkCanvas> owned_; DISALLOW_COPY_AND_ASSIGN for each class that shouldn't be ...
3 years, 9 months ago (2017-03-02 19:44:40 UTC) #33
vmpstr
https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h File cc/paint/paint_canvas.h (right): https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h#newcode48 cc/paint/paint_canvas.h:48: ALWAYS_INLINE bool peekPixels(SkPixmap* pixmap) { Where is this used? ...
3 years, 9 months ago (2017-03-02 21:52:42 UTC) #40
danakj
https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h File cc/paint/paint_canvas.h (right): https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h#newcode86 cc/paint/paint_canvas.h:86: enum { On 2017/03/02 21:52:42, vmpstr wrote: > Can ...
3 years, 9 months ago (2017-03-02 22:05:06 UTC) #41
vmpstr
https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h File cc/paint/paint_canvas.h (right): https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h#newcode86 cc/paint/paint_canvas.h:86: enum { On 2017/03/02 22:05:06, danakj wrote: > On ...
3 years, 9 months ago (2017-03-02 22:11:42 UTC) #42
danakj
https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h File cc/paint/paint_canvas.h (right): https://codereview.chromium.org/2690583002/diff/560001/cc/paint/paint_canvas.h#newcode86 cc/paint/paint_canvas.h:86: enum { On 2017/03/02 22:11:42, vmpstr wrote: > On ...
3 years, 9 months ago (2017-03-02 22:14:43 UTC) #43
Justin Novosad
https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode572 third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:572: WTF::wrapUnique(new PaintCanvas(m_surface->getCanvas())); Why do we need to make of ...
3 years, 9 months ago (2017-03-02 22:19:24 UTC) #44
Justin Novosad
https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode572 third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:572: WTF::wrapUnique(new PaintCanvas(m_surface->getCanvas())); On 2017/03/02 22:19:24, Justin Novosad wrote: > ...
3 years, 9 months ago (2017-03-02 22:23:15 UTC) #45
danakj
https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2690583002/diff/580001/third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp#newcode572 third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:572: WTF::wrapUnique(new PaintCanvas(m_surface->getCanvas())); On 2017/03/02 22:23:15, Justin Novosad wrote: > ...
3 years, 9 months ago (2017-03-02 22:35:36 UTC) #46
enne (OOO)
Addressed comments. Specific replies below. https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_recorder.h File cc/paint/paint_recorder.h (right): https://codereview.chromium.org/2690583002/diff/540001/cc/paint/paint_recorder.h#newcode23 cc/paint/paint_recorder.h:23: SkBBHFactory* bbhFactory = nullptr, ...
3 years, 9 months ago (2017-03-02 23:59:44 UTC) #49
vmpstr
cc lgtm aside from the nits: https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_flags.h File cc/paint/paint_flags.h (right): https://codereview.chromium.org/2690583002/diff/640001/cc/paint/paint_flags.h#newcode23 cc/paint/paint_flags.h:23: PaintFlags() = default; ...
3 years, 9 months ago (2017-03-03 07:12:08 UTC) #59
danakj
LGTM, looks like you changed most of the parameter names, thanks! I spotted a few ...
3 years, 9 months ago (2017-03-03 18:33:19 UTC) #60
enne (OOO)
Thanks. Addressed review comments. Found a few more parameters not in Chromium style to rename ...
3 years, 9 months ago (2017-03-03 19:26:10 UTC) #63
danakj
https://codereview.chromium.org/2690583002/diff/660001/cc/paint/paint_recorder.h File cc/paint/paint_recorder.h (right): https://codereview.chromium.org/2690583002/diff/660001/cc/paint/paint_recorder.h#newcode49 cc/paint/paint_recorder.h:49: canvas_.reset(); On 2017/03/03 19:26:10, enne wrote: > Apparently getRecordingCanvas ...
3 years, 9 months ago (2017-03-03 19:27:22 UTC) #64
enne (OOO)
junov: blink Some more trivial owner changes now that this is closer to landing: derat: ...
3 years, 9 months ago (2017-03-03 20:54:51 UTC) #67
Daniel Erat
lgtm for /ash
3 years, 9 months ago (2017-03-03 21:12:03 UTC) #68
Justin Novosad
lgtm for blink
3 years, 9 months ago (2017-03-03 21:47:32 UTC) #71
enne (OOO)
ddorwin, nick, vitalybuka: ping for small owners review
3 years, 9 months ago (2017-03-06 17:27:46 UTC) #72
ddorwin
media/ LGTM
3 years, 9 months ago (2017-03-06 17:37:23 UTC) #73
ncarter (slow)
content lgtm
3 years, 9 months ago (2017-03-06 19:20:40 UTC) #78
Vitaly Buka (NO REVIEWS)
lgtm
3 years, 9 months ago (2017-03-06 21:23:03 UTC) #86
Vitaly Buka (NO REVIEWS)
printing/ lgtm
3 years, 9 months ago (2017-03-06 21:23:28 UTC) #87
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690583002/700001
3 years, 9 months ago (2017-03-06 21:25:34 UTC) #90
commit-bot: I haz the power
Committed patchset #36 (id:700001) as https://chromium.googlesource.com/chromium/src/+/762276ecdfbff938d3b7431ad0ed00c3ab6a4136
3 years, 9 months ago (2017-03-06 21:31:16 UTC) #93
enne (OOO)
A revert of this CL (patchset #36 id:700001) has been created in https://codereview.chromium.org/2739533003/ by enne@chromium.org. ...
3 years, 9 months ago (2017-03-07 18:51:42 UTC) #94
enne (OOO)
danakj, vmpstr: ptal at changes Due to SkMiniPicture which is a derived SkPicture class, I ...
3 years, 9 months ago (2017-03-07 21:37:27 UTC) #96
danakj
LGTM
3 years, 9 months ago (2017-03-07 23:58:35 UTC) #101
vmpstr
lgtm as well
3 years, 9 months ago (2017-03-08 01:13:11 UTC) #102
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690583002/740001
3 years, 9 months ago (2017-03-08 17:58:52 UTC) #105
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/381052)
3 years, 9 months ago (2017-03-08 18:10:46 UTC) #107
enne (OOO)
+pdr for Blink Source/web
3 years, 9 months ago (2017-03-08 19:04:50 UTC) #110
enne (OOO)
On 2017/03/08 at 19:04:50, enne wrote: > +pdr for Blink Source/web +pdr for real this ...
3 years, 9 months ago (2017-03-09 01:40:24 UTC) #112
pdr.
On 2017/03/09 at 01:40:24, enne wrote: > On 2017/03/08 at 19:04:50, enne wrote: > > ...
3 years, 9 months ago (2017-03-09 17:58:41 UTC) #113
enne (OOO)
On 2017/03/09 at 17:58:41, pdr wrote: > On 2017/03/09 at 01:40:24, enne wrote: > > ...
3 years, 9 months ago (2017-03-09 17:59:56 UTC) #114
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2690583002/740001
3 years, 9 months ago (2017-03-09 18:00:20 UTC) #116
commit-bot: I haz the power
3 years, 9 months ago (2017-03-09 20:00:55 UTC) #119
Message was sent while issue was closed.
Committed patchset #38 (id:740001) as
https://chromium.googlesource.com/chromium/src/+/d2501577f2df0764eda66614ddf4...

Powered by Google App Engine
This is Rietveld 408576698