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

Issue 2758413002: cc/paint: Remove PaintCanvas::peekPixels. (Closed)

Created:
3 years, 9 months ago by vmpstr
Modified:
3 years, 9 months ago
CC:
cc-bugs_chromium.org, chfremer+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

cc/paint: Remove PaintCanvas::peekPixels. This patch removes peekPixels and replaces callers with an explicit SkBitmap construction and PaintCanvas (as opposed to PaintSurface). R=enne@chromium.org TBR=jochen@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2758413002 Cr-Commit-Position: refs/heads/master@{#458880} Committed: https://chromium.googlesource.com/chromium/src/+/4f7674971344b7788b85caabba2a234aa3918e6b

Patch Set 1 #

Patch Set 2 : update #

Total comments: 1

Patch Set 3 : canvas #

Total comments: 3

Patch Set 4 : update #

Patch Set 5 : update #

Patch Set 6 : update #

Total comments: 12

Patch Set 7 : update #

Total comments: 4

Patch Set 8 : update #

Patch Set 9 : update #

Patch Set 10 : update #

Patch Set 11 : update #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -108 lines) Patch
M cc/paint/paint_canvas.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -1 line 0 comments Download
M cc/paint/skia_paint_canvas.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +5 lines, -1 line 0 comments Download
M cc/paint/skia_paint_canvas.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +1 line, -4 lines 0 comments Download
M chrome/browser/thumbnails/content_analysis_unittest.cc View 1 2 3 4 5 6 7 6 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/thumbnails/content_based_thumbnailing_algorithm_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/thumbnails/simple_thumbnail_crop_unittest.cc View 1 2 3 4 5 6 7 5 chunks +5 lines, -5 lines 0 comments Download
M components/wallpaper/wallpaper_color_calculator_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/webmediaplayer_ms_compositor.cc View 1 1 chunk +13 lines, -11 lines 0 comments Download
M content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc View 1 1 chunk +9 lines, -6 lines 0 comments Download
M content/renderer/pepper/plugin_instance_throttler_impl_unittest.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M ui/gfx/canvas.h View 1 2 3 4 5 6 7 4 chunks +10 lines, -6 lines 0 comments Download
M ui/gfx/canvas.cc View 1 2 3 4 5 6 7 8 9 5 chunks +30 lines, -25 lines 1 comment Download
M ui/gfx/canvas_paint_mac.mm View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -11 lines 0 comments Download
M ui/gfx/color_analysis_unittest.cc View 1 2 3 4 5 6 7 8 chunks +8 lines, -8 lines 0 comments Download
M ui/gfx/color_utils_unittest.cc View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M ui/gfx/nine_image_painter_unittest.cc View 1 2 3 4 5 6 7 5 chunks +5 lines, -5 lines 0 comments Download
M ui/gfx/render_text_unittest.cc View 1 2 3 4 5 6 4 chunks +14 lines, -14 lines 0 comments Download

Messages

Total messages: 70 (43 generated)
vmpstr
+enne for cc and overall +danakj for ui +dalecurtis for media +emircan for webrtc Please ...
3 years, 9 months ago (2017-03-20 20:22:57 UTC) #3
vmpstr
-danakj,+msw since it's rendertext unittest change.
3 years, 9 months ago (2017-03-20 21:29:09 UTC) #5
enne (OOO)
lgtm overall. This looks like a straightforward transformation. https://codereview.chromium.org/2758413002/diff/20001/content/renderer/media/webmediaplayer_ms_compositor.cc File content/renderer/media/webmediaplayer_ms_compositor.cc (right): https://codereview.chromium.org/2758413002/diff/20001/content/renderer/media/webmediaplayer_ms_compositor.cc#newcode65 content/renderer/media/webmediaplayer_ms_compositor.cc:65: video_renderer->Copy( ...
3 years, 9 months ago (2017-03-20 21:45:14 UTC) #7
DaleCurtis
lgtm
3 years, 9 months ago (2017-03-20 21:49:45 UTC) #8
emircan
content/renderer/media/webrtc/* lgtm
3 years, 9 months ago (2017-03-20 22:00:09 UTC) #9
msw
render_text_unittest.cc lgtm
3 years, 9 months ago (2017-03-20 22:03:13 UTC) #11
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/2758413002/20001
3 years, 9 months ago (2017-03-20 22:19:58 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/377606)
3 years, 9 months ago (2017-03-20 22:54:26 UTC) #15
vmpstr
Had to touch gfx::Canvas as well, since the mac version uses peekPixels. enne/danakj/ccameron, could you ...
3 years, 9 months ago (2017-03-20 23:50:33 UTC) #17
enne (OOO)
https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h#newcode479 ui/gfx/canvas.h:479: SkBitmap& get_bitmap() { Maybe ToBitmap() should return bitmap_ if ...
3 years, 9 months ago (2017-03-21 00:46:38 UTC) #18
vmpstr
https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h#newcode479 ui/gfx/canvas.h:479: SkBitmap& get_bitmap() { On 2017/03/21 00:46:38, enne wrote: > ...
3 years, 9 months ago (2017-03-21 17:47:13 UTC) #21
enne (OOO)
https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2758413002/diff/40001/ui/gfx/canvas.h#newcode479 ui/gfx/canvas.h:479: SkBitmap& get_bitmap() { On 2017/03/21 at 17:47:12, vmpstr wrote: ...
3 years, 9 months ago (2017-03-21 17:51:02 UTC) #22
vmpstr
PTAL. I had to add a few includes, since I removed gfx::Canvas include for SkSurface.
3 years, 9 months ago (2017-03-21 19:31:13 UTC) #31
enne (OOO)
lgtm
3 years, 9 months ago (2017-03-21 19:32:59 UTC) #32
vmpstr
danakj/ccameron could you take a look at ui/ bits please?
3 years, 9 months ago (2017-03-21 19:37:16 UTC) #33
danakj
https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc#newcode42 ui/gfx/canvas.cc:42: bool success = bitmap.tryAllocPixels(info); why not allocPixels? We should ...
3 years, 9 months ago (2017-03-21 20:02:57 UTC) #36
vmpstr
PTAL https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.cc#newcode42 ui/gfx/canvas.cc:42: bool success = bitmap.tryAllocPixels(info); On 2017/03/21 20:02:56, danakj ...
3 years, 9 months ago (2017-03-21 22:34:54 UTC) #38
danakj
https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h#newcode475 ui/gfx/canvas.h:475: SkBitmap ToBitmap(); On 2017/03/21 22:34:54, vmpstr wrote: > On ...
3 years, 9 months ago (2017-03-21 22:40:02 UTC) #40
danakj
https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h File ui/gfx/canvas.h (right): https://codereview.chromium.org/2758413002/diff/100001/ui/gfx/canvas.h#newcode475 ui/gfx/canvas.h:475: SkBitmap ToBitmap(); On 2017/03/21 22:40:01, danakj wrote: > On ...
3 years, 9 months ago (2017-03-21 22:42:48 UTC) #41
ccameron
ui lgtm
3 years, 9 months ago (2017-03-22 00:20:17 UTC) #47
ccameron
On 2017/03/22 00:20:17, ccameron wrote: > ui lgtm (% danakj's remaining comments)
3 years, 9 months ago (2017-03-22 00:38:04 UTC) #48
vmpstr
PTAL. Things to look out for: - I removed the now unneeded includes - GetBitmap ...
3 years, 9 months ago (2017-03-22 17:46:55 UTC) #56
danakj
ui/gfx LGTM https://codereview.chromium.org/2758413002/diff/200001/ui/gfx/canvas.cc File ui/gfx/canvas.cc (right): https://codereview.chromium.org/2758413002/diff/200001/ui/gfx/canvas.cc#newcode546 ui/gfx/canvas.cc:546: // When the bitmap is copied, it ...
3 years, 9 months ago (2017-03-22 18:46:39 UTC) #58
vmpstr
+jochen for chrome/, components/ and content/. Please take a look, it's just a function rename ...
3 years, 9 months ago (2017-03-22 19:04:31 UTC) #60
vmpstr
TBRing jochen for function name changes
3 years, 9 months ago (2017-03-22 21:17:08 UTC) #64
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/2758413002/200001
3 years, 9 months ago (2017-03-22 21:18:07 UTC) #67
commit-bot: I haz the power
3 years, 9 months ago (2017-03-22 21:24:58 UTC) #70
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/4f7674971344b7788b85caabba2a...

Powered by Google App Engine
This is Rietveld 408576698