Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479563005:
Create manager to track OffscreenCanvasSurfaceImpl instances (Closed)
|
Created:
4 years, 1 month ago by xlai (Olivia) Modified:
4 years, 1 month 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. |
DescriptionCreate 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 #Messages
Total messages: 62 (31 generated)
xlai@chromium.org changed reviewers: + dcheng@chromium.org, junov@chromium.org, kinuko@chromium.org
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 changes in content/browser
(I'm not too familiar with the offline canvas code, I'm happy to review once junov and other code owners are happy with the change)
Can you help me understand how OffscreenCanvasSurface is supposed to be used? Is it expected that we connect to this interface, and the consumer must once (and only once) call GetSurfaceId()? https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:19: : canvas_id_(canvas_id), Please make sure that |canvas_id_| and |canvas_id| have matching types. (Actually, I'm not sure why this class needs |canvas_id_|, it's never used after the ctor. https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:38: DCHECK(surface_id_.is_null()); This can't DCHECK, since the renderer could do this an arbitrary number of times. Will we violate an invariant if we allow this to be called multiple times? https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_surface_impl.h (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_surface_impl.h:42: int canvas_id_; Nit: ditto, make sure the types match please. https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_surface_manager.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_surface_manager.cc:22: g_manager = new OffscreenCanvasSurfaceManager(); Let's just use a leaky LazyInstance, unless there's a reason to do otherwise. (Why do we need to do this cleanup? What breaks if we don't have Terminate()?) https://codereview.chromium.org/2479563005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom (right): https://codereview.chromium.org/2479563005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:17: GetSurfaceId(uint32 canvas_id) => (cc.mojom.SurfaceId surface_id); Help me understand =) What is canvas id? why do we need to identify these with canvas ids?
High-level comment: The manager gets populated but we don't do any lookups into it with this CL. I understand that will change in the near future, but you should at least have unit test in this CL to verify that what is coded in this CL is correct. otherwise, lgtm for third_party/WebKit/*.[h|cpp]
unit test added. comments addressed. https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:19: : canvas_id_(canvas_id), On 2016/11/08 18:30:23, dcheng wrote: > Please make sure that |canvas_id_| and |canvas_id| have matching types. > > (Actually, I'm not sure why this class needs |canvas_id_|, it's never used after > the ctor. This canvas_id_ is a unique identifier for this particular instance of OffscreenCanvasCompositorFrameSink. Both OffscreenCanvasSurfaceImpl instances and OffscreenCanvasCompositorFrameSink instances find their counterparts via the use of this canvas_id. Although it is not used in this patch (except the unit test), it's going to be used in the next few patches for a OffscreenCanvas style resizing workflow. https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:38: DCHECK(surface_id_.is_null()); On 2016/11/08 18:30:23, dcheng wrote: > This can't DCHECK, since the renderer could do this an arbitrary number of > times. > > Will we violate an invariant if we allow this to be called multiple times? This IPC call is supposed to be called only once for the initial set up of Surface Layer, for each HTMLCanvasElement that has been transferred to OffscreenCanvas. It is called twice, it means the caller does it wrongly. It's more like an implementation check to prevent future developers from making a mistake. https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_surface_impl.h (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_surface_impl.h:42: int canvas_id_; On 2016/11/08 18:30:23, dcheng wrote: > Nit: ditto, make sure the types match please. Sure. I make all of them to be int32_t. I think we do have a case when canvas_id is -1. https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_surface_manager.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_surface_manager.cc:22: g_manager = new OffscreenCanvasSurfaceManager(); On 2016/11/08 18:30:23, dcheng wrote: > Let's just use a leaky LazyInstance, unless there's a reason to do otherwise. > > (Why do we need to do this cleanup? What breaks if we don't have Terminate()?) I'm following the example of ImageTransportFactory where it has a global singleton and has Terminate() when the browser main thread is shutdown. The Terminate() must be triggered because it deletes this object and invokes its destructor to clear the two unordered_map. Note that the two unordered_map contains weak pointers of Mojo services. In the future, we rely on the weak pointer to make IPC call and will erase weak pointer if the Mojo service is shut down. Therefore, in the case when browser main thread is shut down, just before the mojo IPC service is reset, we must clean up these weak pointers. I take a look at the template LazyInstance and it does not offer me such an explicit call to destructor so it's not quite suitable in this case. https://codereview.chromium.org/2479563005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom (right): https://codereview.chromium.org/2479563005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:17: GetSurfaceId(uint32 canvas_id) => (cc.mojom.SurfaceId surface_id); On 2016/11/08 18:30:23, dcheng wrote: > Help me understand =) > > What is canvas id? why do we need to identify these with canvas ids? Canvas ID is a unique identifier for an HTMLCanvasElement. Actually, it is just a unique ID for every DOM object. We need to identify different canvas ID because we want to handle the case when there are multiple HTMLCanvasElement transfer control to Offscreen. In this case, the browser side needs to distinguish which HTMLCanvasElement is making a request to the browser.
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... 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 2016/11/08 18:30:23, dcheng wrote: > > This can't DCHECK, since the renderer could do this an arbitrary number of > > times. > > > > Will we violate an invariant if we allow this to be called multiple times? > > This IPC call is supposed to be called only once for the initial set up of > Surface Layer, for each HTMLCanvasElement that has been transferred to > OffscreenCanvas. It is called twice, it means the caller does it wrongly. It's > more like an implementation check to prevent future developers from making a > mistake. Right, but this is browser code being invoked by the renderer via a remote Mojo IPC call. The browser can make no assumptions that the renderer is behaving correctly. Given that, is it safe for this to be a DCHECK, or will this break important invariants? https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/offscreen_canvas_surface_manager.cc (right): https://codereview.chromium.org/2479563005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/offscreen_canvas_surface_manager.cc:22: g_manager = new OffscreenCanvasSurfaceManager(); On 2016/11/10 15:45:11, xlai (Olivia) wrote: > On 2016/11/08 18:30:23, dcheng wrote: > > Let's just use a leaky LazyInstance, unless there's a reason to do otherwise. > > > > (Why do we need to do this cleanup? What breaks if we don't have Terminate()?) > > I'm following the example of ImageTransportFactory where it has a global > singleton and has Terminate() when the browser main thread is shutdown. > > The Terminate() must be triggered because it deletes this object and invokes its > destructor to clear the two unordered_map. Note that the two unordered_map > contains weak pointers of Mojo services. In the future, we rely on the weak > pointer to make IPC call and will erase weak pointer if the Mojo service is shut > down. Therefore, in the case when browser main thread is shut down, just before > the mojo IPC service is reset, we must clean up these weak pointers. I take a > look at the template LazyInstance and it does not offer me such an explicit call > to destructor so it's not quite suitable in this case. I guess I'm missing something still. Don't the objects tracked in the map unregister themselves on destruction anyway? If they're not destroyed when the process is exiting, then leaving them in the map doesn't matter. And if they are destroyed, they'll be removed from the maps anyway? https://codereview.chromium.org/2479563005/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom (right): https://codereview.chromium.org/2479563005/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:17: GetSurfaceId(uint32 canvas_id) => (cc.mojom.SurfaceId surface_id); On 2016/11/10 15:45:11, xlai (Olivia) wrote: > On 2016/11/08 18:30:23, dcheng wrote: > > Help me understand =) > > > > What is canvas id? why do we need to identify these with canvas ids? > > Canvas ID is a unique identifier for an HTMLCanvasElement. Actually, it is just > a unique ID for every DOM object. > > We need to identify different canvas ID because we want to handle the case when > there are multiple HTMLCanvasElement transfer control > to Offscreen. In this case, the browser side needs to distinguish which > HTMLCanvasElement is making a request to the browser. In a Mojo world though, I would expect the endpoints to be associated by virtue of having a connected interface. We shouldn't need numeric IDs to associate different endpoints.
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
It seems like you're using canvas_id to uniquely identify a CompositorFrameSink. This isn't necessary. The FrameSinkId part of a SurfaceID uniquely identifies a CompositorFrameSInk.
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
The CL is much smaller now, thanks to fsamuel@'s suggestion to use FrameSinkId as an identifier. Also, I talked to dcheng@ regarding the transferring Mojo InterfacePtr part and had explained the reason why this could not be done on main->worker transfer. The rest of comments are addressed inline. https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:40: if (frame_sink_id_.is_valid()) { dcheng@ has asked if the failure of this DCHECK will break important invariants on browser. Therefore, I'm adding this if block to handle the unwanted situation when the DCHECK failed. https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_manager.cc (right): https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_manager.cc:12: LAZY_INSTANCE_INITIALIZER; I'm using a LazyInstance now, in response to dcheng@'s comment that when the g_manager is destroyed, objects tracked in the map unregister themselves on destruction anyway (which is true). https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_manager.h (right): https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_manager.h:38: std::unordered_map<cc::FrameSinkId, I'm using a FrameSinkId to identify the instance, in response to fsamuel@'s comment.
I'm not really understanding what an OffscreenCanvasSurface is and what an OffscreenCompositorFrameSink is. Could you please explain the roles of the two classes in your view? https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:18: : surface_id_(surface_id), client_(std::move(client)), weak_factory_(this) { Don't store this SurfaceId maybe? Store a const FrameSinkId. and store the LocalFrameId portion in a LocalFrameId member variable.
On 2016/11/11 23:33:17, Fady Samuel wrote: > I'm not really understanding what an OffscreenCanvasSurface is and what an > OffscreenCompositorFrameSink is. Could you please explain the roles of the two > classes in your view? Perhaps this diagram explains better: https://www.lucidchart.com/documents/view/ee79924f-2d46-46ee-8da7-9ecd220eb6b1 OffscreenCanvasSurface is a service to connect to the HTMLCanvasElement on the main thread when it is about to create a SurfaceLayer (that requires a SurfaceId in hand, however, in the future when SurfaceId can be created on renderer), the GetSurfaceId IPC may no longer be needed. But even at that time, OffscreenCanvasSurface still has its function to connect renderer/main thread to browser/UI thread. I have plans in OffscreenCanvas resizing to make it a two-way communication. OffscreenCanvasCompositorFrameSink is a service to connect to the OffscreenCanvas on the worker thread.
https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/160001/content/browser/render... 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 Samuel wrote: > Don't store this SurfaceId maybe? > > Store a const FrameSinkId. and store the LocalFrameId portion in a LocalFrameId > member variable. I'm going to do it in the next CL. Because right now the SubmitCompositorFrame IPC does not submit the SurfaceId together with a frame, we still rely on a stored surface Id to know where to send the frame.
Based on discussion off-line with fsamuel@, make OffscreenCanvasSurfaceManager a subclass of Surfaceobserver and remove everything about OffscreenCanvasCompositorFrameSink from OffscreenCanvasSurfaceManager (the manager should not keep info about compositorframesink).
https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... 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/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h:41: base::WeakPtr<OffscreenCanvasCompositorFrameSink> GetWeakPtr() { Why does this need a weak ptr?
https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... 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... "you should not handle DCHECK() failures" We should either only do DCHECK or handle the error case. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:47: << "OffscreenCanvasSurfaceManager twice."; Should this report a bad message? Does it mean the renderer is compromised (and should be killed if possible)?
https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:26: surface_id_.frame_sink_id())); Do we need to check somewhere higher in the stack that OffscreenCanvasCompositorFrameSink is always created so it's paired with a valid OffscreenSurface? https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:40: if (frame_sink_id_.is_valid()) { On 2016/11/15 01:45:48, kinuko wrote: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > "you should not handle DCHECK() failures" > > We should either only do DCHECK or handle the error case. Since this comes from the renderer, I think we have to handle this correctly. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:47: << "OffscreenCanvasSurfaceManager twice."; On 2016/11/15 01:45:48, kinuko wrote: > Should this report a bad message? Does it mean the renderer is compromised (and > should be killed if possible)? Yeah, this should call mojo::ReportBadMessage. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_manager.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_manager.cc:11: base::LazyInstance<OffscreenCanvasSurfaceManager> g_manager = Nit: let's make this a Leaky LazyInstance unless we have a reason to do otherwise. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc:111: base::WrapUnique(new OffscreenCanvasCompositorFrameSink( auto compositor_frame_sink = base::MakeUnique<OffscreenCanvasCompositorFrameSink>(...) here and elsewhere
Patchset #9 (id:200001) has been deleted
The new patch addressed all the review comments. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:25: DCHECK(OffscreenCanvasSurfaceManager::GetInstance()->GetSurfaceInstance( On 2016/11/14 21:15:40, Fady Samuel wrote: > Can we get rid of this? Done. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:26: surface_id_.frame_sink_id())); On 2016/11/15 07:28:29, dcheng wrote: > Do we need to check somewhere higher in the stack that > OffscreenCanvasCompositorFrameSink is always created so it's paired with a valid > OffscreenSurface? I'm removing it now. I think this DCHECK is better to be placed in the implementation of OffscreenCanvasSurfaceManager::OnSurfaceCreated in a resizing workflow but since I have not implemented that yet I'll delay doing this. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h:41: base::WeakPtr<OffscreenCanvasCompositorFrameSink> GetWeakPtr() { On 2016/11/14 21:15:40, Fady Samuel wrote: > Why does this need a weak ptr? Done. My mistake of not doing a clean delete after the previous patch. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:40: if (frame_sink_id_.is_valid()) { On 2016/11/15 01:45:48, kinuko wrote: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > "you should not handle DCHECK() failures" > > We should either only do DCHECK or handle the error case. Done. I remove the DCHECK and only handle the error case. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:47: << "OffscreenCanvasSurfaceManager twice."; On 2016/11/15 07:28:29, dcheng wrote: > On 2016/11/15 01:45:48, kinuko wrote: > > Should this report a bad message? Does it mean the renderer is compromised > (and > > should be killed if possible)? > > Yeah, this should call mojo::ReportBadMessage. Done. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_manager.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_manager.cc:11: base::LazyInstance<OffscreenCanvasSurfaceManager> g_manager = On 2016/11/15 07:28:29, dcheng wrote: > Nit: let's make this a Leaky LazyInstance unless we have a reason to do > otherwise. Done. https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc (right): https://codereview.chromium.org/2479563005/diff/180001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc:111: base::WrapUnique(new OffscreenCanvasCompositorFrameSink( On 2016/11/15 07:28:29, dcheng wrote: > auto compositor_frame_sink = > base::MakeUnique<OffscreenCanvasCompositorFrameSink>(...) > > here and elsewhere Done.
https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_impl.cc:30: mojo::MakeStrongBinding(base::MakeUnique<OffscreenCanvasSurfaceImpl>(), I'm confused by the lifetime management here. It seems more complicated than necessary. What if instead, offscreenCanvasSurfaceManager owned OffscreenCavansSurface. When OffscreenCanvasSurfaceImpl's binding_ gets a connection error then you tell the manager and it deletes it from the map.
https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... 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 confused by the lifetime management here. It seems more complicated than > necessary. > > What if instead, offscreenCanvasSurfaceManager owned OffscreenCavansSurface. > When OffscreenCanvasSurfaceImpl's binding_ gets a connection error then you tell > the manager and it deletes it from the map. I don't understand why this is a problem; this is exactly same as the create function of every other mojo service in browser/UI. It's added in RenderProcessHostImpl::RegisterMojoInterfaces() just as everyone else. Letting OffscreenCanvasSurfaceManager owns OffscreenCanvasSurface may create more troubles: in the case when user invokes transferControlToOffscreen() and did not do commit(), we are creating an unnecessary instance of *Manager; also, if it has a connection error, it won't register to *manager in the first place so why bother the deletion?
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... 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 2016/11/15 17:32:45, Fady Samuel wrote: > > I'm confused by the lifetime management here. It seems more complicated than > > necessary. > > > > What if instead, offscreenCanvasSurfaceManager owned OffscreenCavansSurface. > > When OffscreenCanvasSurfaceImpl's binding_ gets a connection error then you > tell > > the manager and it deletes it from the map. > > I don't understand why this is a problem; this is exactly same as the create > function of every other mojo service in browser/UI. It's added in > RenderProcessHostImpl::RegisterMojoInterfaces() just as everyone else. > > Letting OffscreenCanvasSurfaceManager owns OffscreenCanvasSurface may create > more troubles: in the case when user invokes transferControlToOffscreen() and > did not do commit(), we are creating an unnecessary instance of *Manager; also, > if it has a connection error, it won't register to *manager in the first place > so > why bother the deletion? I guess the part that I find really confusing is the weak pointers in OffscreenCanvasSurfaceManager. Will there ever be a point in time where OffscreenCanvasSurfaceManager is holding on to a nullptr for a FrameSinkId? I think the answer is no, but that's what the use of WeakPtr suggests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_impl.cc (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... 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 2016/11/15 17:56:52, xlai (Olivia) wrote: > > On 2016/11/15 17:32:45, Fady Samuel wrote: > > > I'm confused by the lifetime management here. It seems more complicated than > > > necessary. > > > > > > What if instead, offscreenCanvasSurfaceManager owned OffscreenCavansSurface. > > > When OffscreenCanvasSurfaceImpl's binding_ gets a connection error then you > > tell > > > the manager and it deletes it from the map. > > > > I don't understand why this is a problem; this is exactly same as the create > > function of every other mojo service in browser/UI. It's added in > > RenderProcessHostImpl::RegisterMojoInterfaces() just as everyone else. > > > > Letting OffscreenCanvasSurfaceManager owns OffscreenCanvasSurface may create > > more troubles: in the case when user invokes transferControlToOffscreen() and > > did not do commit(), we are creating an unnecessary instance of *Manager; > also, > > if it has a connection error, it won't register to *manager in the first place > > so > > why bother the deletion? > > I guess the part that I find really confusing is the weak pointers in > OffscreenCanvasSurfaceManager. Will there ever be a point in time where > OffscreenCanvasSurfaceManager is holding on to a nullptr for a FrameSinkId? I > think the answer is no, but that's what the use of WeakPtr suggests. No I don't think OffscreenCanvasSurfaceManager will ever hold a nullptr because when the instance is destructed it will always remember to unregister itself from manager. I must use WeakPtr or raw pointer because obviously OffscreenCanvasSurfaceManager is not the owner of those instances; WeakPtr look a better choice than raw pointer to me.
I'm not really a fan of the lifetime management here, but I won't block it. lgtm + nit. https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h:37: const cc::FrameSinkId& frame_sink_id() const { Is exposing this necessary?
https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.h (right): https://codereview.chromium.org/2479563005/diff/220001/content/browser/render... 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 Samuel wrote: > Is exposing this necessary? It's not necessary after *Manager and *CompositorFrameSink are made dis-associated in later patches. Removing it now.
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
LGTM, but let's store raw pointer in the manager rather than weak pointers. Weak pointers give the (wrong) impression that code needs to make sure there aren't nulled out weak pointers. Add a comment that the tracked objects self-unregister on destruction or something.
+1 to what dcheng@ said! I'm not a fan of the weak pointers! Store raw pointers if you're confident of the lifetime of these objects. Fady On Wed, Nov 16, 2016 at 9:25 AM <dcheng@chromium.org> wrote: > LGTM, but let's store raw pointer in the manager rather than weak pointers. > > Weak pointers give the (wrong) impression that code needs to make sure > there > aren't nulled out weak pointers. Add a comment that the tracked objects > self-unregister on destruction or something. > > https://codereview.chromium.org/2479563005/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
+1 to what dcheng@ said! I'm not a fan of the weak pointers! Store raw pointers if you're confident of the lifetime of these objects. Fady On Wed, Nov 16, 2016 at 9:25 AM <dcheng@chromium.org> wrote: > LGTM, but let's store raw pointer in the manager rather than weak pointers. > > Weak pointers give the (wrong) impression that code needs to make sure > there > aren't nulled out weak pointers. Add a comment that the tracked objects > self-unregister on destruction or something. > > https://codereview.chromium.org/2479563005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Took away WeakPtr now. Gentle ping to kinuko@.
Much simpler now :) lgtm % 2 nits https://codereview.chromium.org/2479563005/diff/280001/content/browser/render... File content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc (right): https://codereview.chromium.org/2479563005/diff/280001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc:27: const cc::SurfaceId getCurrentSurfaceId() { return current_surface_id_; } Did you mean const ref / const method? const cc::SurfaceId& getCurrentSurfaceId() const { return ... } https://codereview.chromium.org/2479563005/diff/280001/content/browser/render... content/browser/renderer_host/offscreen_canvas_surface_manager_unittest.cc:38: std::unique_ptr<BrowserThreadImpl> ui_thread_; Could we just use TestBrowserThread and initialize it as BrowserThread::UI ?
Description was changed from
==========
Create manager to track OffscreenCanvas(SurfaceImpl/CompositorFrameSink)
instances
OffscreenCanvasSurfaceManager is a singleton in browser UI thread to keep track
of OffscreenCanvasSurfaceImpl instances (which connects to renderer/main thread)
and OffscreenCanvasCompositorFrameSink instances (which connects to renderer/
worker 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 canvas Id.
The manager is lazily created only when OffscreenCanvas is working and is
shutdown when browser main thread shuts down. It is useful in two cases:
(1) managing the situation of multiple html canvases and offscreencanvases.
(2) connects the OffscreenCanvasSurfaceImpl instance and its corresponding
OffscreenCanvasCompositorFrameSink instance together in browser to
facilitate OffscreenCanvas resizing workflow implementation in next CLs.
BUG=661605,662498
==========
to
==========
Create manager to track OffscreenCanvas(SurfaceImpl/CompositorFrameSink)
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
==========
Description was changed from ========== Create manager to track OffscreenCanvas(SurfaceImpl/CompositorFrameSink) 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 ========== to ========== 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 ==========
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xlai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org, fsamuel@chromium.org, dcheng@chromium.org, kinuko@chromium.org Link to the patchset: https://codereview.chromium.org/2479563005/#ps320001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/92e403ca88572c004290ab15450ce3f6b030cbbd Cr-Commit-Position: refs/heads/master@{#432930} |
