Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(41)

Issue 2479563005: Create manager to track OffscreenCanvasSurfaceImpl instances (Closed)

Created:
1 year, 1 month ago by xlai (Olivia)
Modified:
1 year ago
CC:
Fady Samuel, chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, yzshen+watch_chromium.org, rwlbuis, darin (slow to review), krit, drott+blinkwatch_chromium.org, blink-reviews-html_chromium.org, jam, abarth-chromium, dglazkov+blink, Rik, darin-cc_chromium.org, blink-reviews, ajuma+watch_chromium.org, blink-reviews-api_chromium.org, jbroman, pdr+graphicswatchlist_chromium.org, haraken, Stephen Chennney, ajuma+watch-canvas_chromium.org, Aaron Boodman, f(malita), danakj+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Create manager to track OffscreenCanvasSurfaceImpl instances OffscreenCanvasSurfaceManager is a singleton in browser UI thread to keep track of OffscreenCanvasSurfaceImpl instances (which connects to renderer/main thread). Each instance corresponds to one single pair of html canvas and its transferred OffscreenCanvas in renderer process and is therefore identified by a unique FrameSinkId. The manager is lazily created. It is useful in two cases: (1) managing the situation of multiple html canvases that have transferred to offscreen. (2) allowing the OffscreenCanvasCompositorFrameSink instance (which connects to OffscreenCanvas on renderer/worker) to connect to its counterpart OffscreenCanvasSurfaceImpl; this is to facilitate OffscreenCanvas resizing workflow implementation in next CLs. BUG=661605, 662498 Committed: https://crrev.com/92e403ca88572c004290ab15450ce3f6b030cbbd Cr-Commit-Position: refs/heads/master@{#432930}

Patch Set 1 #

Total comments: 13

Patch Set 2 : rebase master #

Patch Set 3 : rebase master again #

Patch Set 4 : Add unit test and fix #

Patch Set 5 : Unregister instance in destructor #

Patch Set 6 : rebase #

Patch Set 7 : fix #

Total comments: 5

Patch Set 8 : remove compositorframesink from manager #

Total comments: 16

Patch Set 9 : further fix #

Total comments: 6

Patch Set 10 : compilation fix #

Patch Set 11 : Nits #

Patch Set 12 : Take away weak pointers #

Total comments: 2

Patch Set 13 : Nits #

Patch Set 14 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+266 lines, -6 lines) Patch
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/renderer_host/offscreen_canvas_surface_impl.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +5 lines, -2 lines 0 comments Download
M content/browser/renderer_host/offscreen_canvas_surface_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +26 lines, -4 lines 0 comments Download
A content/browser/renderer_host/offscreen_canvas_surface_manager.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +51 lines, -0 lines 0 comments Download
A content/browser/renderer_host/offscreen_canvas_surface_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +53 lines, -0 lines 0 comments Download
A content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +128 lines, -0 lines 0 comments Download
M content/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 62 (31 generated)
xlai (Olivia)
dcheng@chromium.org: Please review changes in offscreen_canvas_surface.mojom junov@chromium.org: Please review changes in third_party/WebKit/ kinuko@chromium.org: Please review ...
1 year, 1 month ago (2016-11-04 20:05:17 UTC) #2
kinuko
(I'm not too familiar with the offline canvas code, I'm happy to review once junov ...
1 year, 1 month ago (2016-11-08 05:17:17 UTC) #3
dcheng
Can you help me understand how OffscreenCanvasSurface is supposed to be used? Is it expected ...
1 year, 1 month ago (2016-11-08 18:30:23 UTC) #4
Justin Novosad
High-level comment: The manager gets populated but we don't do any lookups into it with ...
1 year, 1 month ago (2016-11-08 18:55:37 UTC) #5
xlai (Olivia)
unit test added. comments addressed. https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc#newcode19 content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:19: : canvas_id_(canvas_id), On 2016/11/08 ...
1 year, 1 month ago (2016-11-10 15:45:11 UTC) #6
dcheng
https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_host/offscreen_canvas_surface_impl.cc File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_host/offscreen_canvas_surface_impl.cc#newcode38 content/browser/renderer_host/offscreen_canvas_surface_impl.cc:38: DCHECK(surface_id_.is_null()); On 2016/11/10 15:45:11, xlai (Olivia) wrote: > On ...
1 year, 1 month ago (2016-11-11 08:36:40 UTC) #11
Fady Samuel
It seems like you're using canvas_id to uniquely identify a CompositorFrameSink. This isn't necessary. The ...
1 year, 1 month ago (2016-11-11 16:37:36 UTC) #13
xlai (Olivia)
The CL is much smaller now, thanks to fsamuel@'s suggestion to use FrameSinkId as an ...
1 year, 1 month ago (2016-11-11 22:56:57 UTC) #16
Fady Samuel
I'm not really understanding what an OffscreenCanvasSurface is and what an OffscreenCompositorFrameSink is. Could you ...
1 year, 1 month ago (2016-11-11 23:33:17 UTC) #17
xlai (Olivia)
On 2016/11/11 23:33:17, Fady Samuel wrote: > I'm not really understanding what an OffscreenCanvasSurface is ...
1 year, 1 month ago (2016-11-14 15:13:09 UTC) #18
xlai (Olivia)
https://codereview.chromium.org/2479563005/diff/160001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/160001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc#newcode18 content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:18: : surface_id_(surface_id), client_(std::move(client)), weak_factory_(this) { On 2016/11/11 23:33:16, Fady ...
1 year, 1 month ago (2016-11-14 15:13:19 UTC) #19
xlai (Olivia)
Based on discussion off-line with fsamuel@, make OffscreenCanvasSurfaceManager a subclass of Surfaceobserver and remove everything ...
1 year, 1 month ago (2016-11-14 21:10:16 UTC) #20
Fady Samuel
https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc#newcode25 content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:25: DCHECK(OffscreenCanvasSurfaceManager::GetInstance()->GetSurfaceInstance( Can we get rid of this? https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h File ...
1 year, 1 month ago (2016-11-14 21:15:40 UTC) #21
kinuko
https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc#newcode40 content/browser/renderer_host/offscreen_canvas_surface_impl.cc:40: if (frame_sink_id_.is_valid()) { https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#CHECK_DCHECK_and-NOTREACHED "you should not handle DCHECK() ...
1 year, 1 month ago (2016-11-15 01:45:48 UTC) #22
dcheng
https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc#newcode26 content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:26: surface_id_.frame_sink_id())); Do we need to check somewhere higher in ...
1 year, 1 month ago (2016-11-15 07:28:29 UTC) #23
xlai (Olivia)
The new patch addressed all the review comments. https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc#newcode25 content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:25: DCHECK(OffscreenCanvasSurfaceManager::GetInstance()->GetSurfaceInstance( ...
1 year, 1 month ago (2016-11-15 17:16:18 UTC) #25
Fady Samuel
https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc#newcode30 content/browser/renderer_host/offscreen_canvas_surface_impl.cc:30: mojo::MakeStrongBinding(base::MakeUnique<OffscreenCanvasSurfaceImpl>(), I'm confused by the lifetime management here. It ...
1 year, 1 month ago (2016-11-15 17:32:45 UTC) #26
xlai (Olivia)
https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc#newcode30 content/browser/renderer_host/offscreen_canvas_surface_impl.cc:30: mojo::MakeStrongBinding(base::MakeUnique<OffscreenCanvasSurfaceImpl>(), On 2016/11/15 17:32:45, Fady Samuel wrote: > I'm ...
1 year, 1 month ago (2016-11-15 17:56:52 UTC) #27
Fady Samuel
https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc#newcode30 content/browser/renderer_host/offscreen_canvas_surface_impl.cc:30: mojo::MakeStrongBinding(base::MakeUnique<OffscreenCanvasSurfaceImpl>(), On 2016/11/15 17:56:52, xlai (Olivia) wrote: > On ...
1 year, 1 month ago (2016-11-15 18:06:44 UTC) #30
xlai (Olivia)
https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_surface_impl.cc#newcode30 content/browser/renderer_host/offscreen_canvas_surface_impl.cc:30: mojo::MakeStrongBinding(base::MakeUnique<OffscreenCanvasSurfaceImpl>(), On 2016/11/15 18:06:43, Fady Samuel wrote: > On ...
1 year, 1 month ago (2016-11-15 19:12:18 UTC) #33
xlai (Olivia)
1 year, 1 month ago (2016-11-15 19:12:20 UTC) #34
Fady Samuel
I'm not really a fan of the lifetime management here, but I won't block it. ...
1 year, 1 month ago (2016-11-15 19:27:53 UTC) #35
xlai (Olivia)
https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h#newcode37 content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h:37: const cc::FrameSinkId& frame_sink_id() const { On 2016/11/15 19:27:52, Fady ...
1 year, 1 month ago (2016-11-15 20:07:47 UTC) #36
dcheng
LGTM, but let's store raw pointer in the manager rather than weak pointers. Weak pointers ...
1 year, 1 month ago (2016-11-16 14:25:43 UTC) #41
Fady Samuel
+1 to what dcheng@ said! I'm not a fan of the weak pointers! Store raw ...
1 year, 1 month ago (2016-11-16 14:36:29 UTC) #42
Fady Samuel
+1 to what dcheng@ said! I'm not a fan of the weak pointers! Store raw ...
1 year, 1 month ago (2016-11-16 14:36:30 UTC) #43
xlai (Olivia)
Took away WeakPtr now. Gentle ping to kinuko@.
1 year, 1 month ago (2016-11-16 21:58:11 UTC) #44
kinuko
Much simpler now :) lgtm % 2 nits https://codereview.chromium.org/2479563005/diff/280001/content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc File content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc (right): https://codereview.chromium.org/2479563005/diff/280001/content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc#newcode27 content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc:27: const ...
1 year ago (2016-11-17 05:26:10 UTC) #45
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/2479563005/320001
1 year ago (2016-11-17 19:02:25 UTC) #58
commit-bot: I haz the power
Committed patchset #14 (id:320001)
1 year ago (2016-11-17 19:26:16 UTC) #60
commit-bot: I haz the power
1 year ago (2016-11-17 20:28:34 UTC) #62
Message was sent while issue was closed.
Patchset 14 (id:??) landed as
https://crrev.com/92e403ca88572c004290ab15450ce3f6b030cbbd
Cr-Commit-Position: refs/heads/master@{#432930}

Powered by Google App Engine
This is Rietveld 0eb02b776