|
|
Created:
4 years, 1 month ago by xlai (Olivia) Modified:
4 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, dglazkov+blink, darin-cc_chromium.org, blink-reviews, blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow Blink to generate cc::FrameSinkId from RenderThread
This CL allows Blink to use generate cc::FrameSinkId, internally
implemented using RenderThread's functions.
Currently, Mus is working on making FrameSinkId being able to
generated on Renderer process, getting rid of the need of IPC
to browser just to get a FrameSinkId.
By making this generation function available in Blink, this CL
prepares for a refactoring effort on the current
OffscreenCanvas.commit() workflow--it would facilitate
simplification on CanvasSurfaceLayerBridge
that will remove the current sync IPC that relies on the
browser process to generate FrameSinkId (which is part of
SurfaceId) for it.
BUG=664547
Committed: https://crrev.com/2956c331e20ecb5d730d17fdf637ee72b6fd3a6c
Cr-Commit-Position: refs/heads/master@{#434535}
Patch Set 1 #
Total comments: 4
Patch Set 2 : change to generateFrameSinkId #
Messages
Total messages: 23 (9 generated)
xlai@chromium.org changed reviewers: + kinuko@chromium.org
Description was changed from ========== Expose RenderThread ID getters to Blink This is a prepartory CL to allow Blink to use RenderThread::GetClientId() and RenderThread::GenerateRoutingId(). These two functions are essential in getting a unique cc::FrameSinkId on renderer process. With that, it would facilitate a refactoring effort on CanvasSurfaceLayerBridge that will remove the current sync IPC that relies on the browser process to generate FrameSinkId (which is part of SurfaceId) for it. BUG=664547 ========== to ========== Expose RenderThread ID getters to Blink This CL allows Blink to use RenderThread::GetClientId() and RenderThread::GenerateRoutingId(). These two functions are essential in generating a unique cc::FrameSinkId on renderer process. This is a preparatory CL for a bigger refactoring effort on the current OffscreenCanvas.commit() workflow. This would facilitate simplification on CanvasSurfaceLayerBridge that will remove the current sync IPC that relies on the browser process to generate FrameSinkId (which is part of SurfaceId) for it. BUG=664547 ==========
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
https://codereview.chromium.org/2514323006/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2514323006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/Platform.h:327: virtual uint32_t getClientId() { return 0; } How about just exposing a "generateFrameSinkId" which is what you really need?
https://codereview.chromium.org/2514323006/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2514323006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/Platform.h:327: virtual uint32_t getClientId() { return 0; } On 2016/11/22 23:41:18, Fady Samuel wrote: > How about just exposing a "generateFrameSinkId" which is what you really need? I would rather keep an interface function as straightforward as possible. Another reason, I'm wondering can we directly use cc::FrameSinkId here? WDYT, kinoku@?
Platform is a global public layer where all public platform APIs need to go through, could we avoid introducing too generic API names / make them more specific? https://codereview.chromium.org/2514323006/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/Platform.h (right): https://codereview.chromium.org/2514323006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/Platform.h:327: virtual uint32_t getClientId() { return 0; } On 2016/11/22 23:51:55, xlai (Olivia) wrote: > On 2016/11/22 23:41:18, Fady Samuel wrote: > > How about just exposing a "generateFrameSinkId" which is what you really need? > > I would rather keep an interface function as straightforward as possible. > > Another reason, I'm wondering can we directly use cc::FrameSinkId here? WDYT, > kinoku@? (Putting aside the discussion if it's recommended or not) yes it's possible, a few public headers actually include cc headers and refer cc types. https://codereview.chromium.org/2514323006/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/Platform.h:330: virtual int generateRoutingId() { return 0; } Is the 'routing ID' about IPC routing or render thread specific term? 'routing ID' is not the concept blink usually understands, do we really need this interface here?
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
We've intentionally avoided exposing routing IDs to Blink. I'm not sure we should be doing this.
Description was changed from ========== Expose RenderThread ID getters to Blink This CL allows Blink to use RenderThread::GetClientId() and RenderThread::GenerateRoutingId(). These two functions are essential in generating a unique cc::FrameSinkId on renderer process. This is a preparatory CL for a bigger refactoring effort on the current OffscreenCanvas.commit() workflow. This would facilitate simplification on CanvasSurfaceLayerBridge that will remove the current sync IPC that relies on the browser process to generate FrameSinkId (which is part of SurfaceId) for it. BUG=664547 ========== to ========== Allow Blink to generate cc::FrameSinkId from RenderThread This CL allows Blink to use generate cc::FrameSinkId, internally implemented using RenderThread's functions. Currently, Mus is working on making FrameSinkId being able to generated on Renderer process, getting rid of the need of IPC to browser just to get a FrameSinkId. By making this generation function available in Blink, this CL prepares for a refactoring effort on the current OffscreenCanvas.commit() workflow--it would facilitate simplification on CanvasSurfaceLayerBridge that will remove the current sync IPC that relies on the browser process to generate FrameSinkId (which is part of SurfaceId) for it. BUG=664547 ==========
Based on fsamuel@ and kinuko@'s feedback, changing the two functions to become only one specific function generateFrameSinkId() which hides the internal details of how a FrameSinkId is generated outside of Blink's view. On 2016/11/23 12:51:41, dcheng wrote: > We've intentionally avoided exposing routing IDs to Blink. I'm not sure we > should be doing this. dcheng@: Now I don't expose routing IDs directly to Blink, the patch is changed. Also I changed the description of this CL. Mus team is currently making a simplification effort to allow many classes that use Surface in Chromium to be able to generate a unique FrameSinkId in renderer process, without needing an IPC to browser to get that information. Since OffscreenCanvas in Blink is also using Surface, we also want to join the rest of classes in Chromium and be able to get rid of a sync IPC that is currently slowing down the API. This CL would facilitate that.
On 2016/11/23 15:23:45, xlai (Olivia) wrote: > Based on fsamuel@ and kinuko@'s feedback, changing the two functions to become > only one specific function generateFrameSinkId() which hides the internal > details of how a FrameSinkId is generated outside of Blink's view. > > > On 2016/11/23 12:51:41, dcheng wrote: > > We've intentionally avoided exposing routing IDs to Blink. I'm not sure we > > should be doing this. > > dcheng@: Now I don't expose routing IDs directly to Blink, the patch is changed. > > Also I changed the description of this CL. > Mus team is currently making a simplification effort to allow many > classes that use Surface in Chromium to be able to generate a unique FrameSinkId > in renderer process, without needing an IPC to browser to get that information. > Since OffscreenCanvas in Blink is also using Surface, we also > want to join the rest of classes in Chromium and be able to get rid of a sync > IPC that is currently slowing down the API. This CL would facilitate that. The FrameSinkId uniquely identifies a CompositorFrameSink within Chrome. We are currently leveraging routing ID but in principle we don't necessarily need to use the routing ID moving forward. I think this approach makes sense because we can change how FrameSinkIds are allocated under the hood (i.e. in the implementation) without exposing that detail to Blink code.
non-OWNER lgtm. This is compatible with the direction we'd like to go in for Mus. How FrameSinkIds are generated may change in the future, but the FrameSinkId concept will likely stick around for a while.
Gentle ping to kinuko@..
lgtm
The CQ bit was checked by xlai@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480089628138260, "parent_rev": "497e8f39a26cc23feb9aca7422509804c1d70e56", "commit_rev": "4128a1c81c70905d104db1adb592e369927f12fc"}
Message was sent while issue was closed.
Description was changed from ========== Allow Blink to generate cc::FrameSinkId from RenderThread This CL allows Blink to use generate cc::FrameSinkId, internally implemented using RenderThread's functions. Currently, Mus is working on making FrameSinkId being able to generated on Renderer process, getting rid of the need of IPC to browser just to get a FrameSinkId. By making this generation function available in Blink, this CL prepares for a refactoring effort on the current OffscreenCanvas.commit() workflow--it would facilitate simplification on CanvasSurfaceLayerBridge that will remove the current sync IPC that relies on the browser process to generate FrameSinkId (which is part of SurfaceId) for it. BUG=664547 ========== to ========== Allow Blink to generate cc::FrameSinkId from RenderThread This CL allows Blink to use generate cc::FrameSinkId, internally implemented using RenderThread's functions. Currently, Mus is working on making FrameSinkId being able to generated on Renderer process, getting rid of the need of IPC to browser just to get a FrameSinkId. By making this generation function available in Blink, this CL prepares for a refactoring effort on the current OffscreenCanvas.commit() workflow--it would facilitate simplification on CanvasSurfaceLayerBridge that will remove the current sync IPC that relies on the browser process to generate FrameSinkId (which is part of SurfaceId) for it. BUG=664547 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Allow Blink to generate cc::FrameSinkId from RenderThread This CL allows Blink to use generate cc::FrameSinkId, internally implemented using RenderThread's functions. Currently, Mus is working on making FrameSinkId being able to generated on Renderer process, getting rid of the need of IPC to browser just to get a FrameSinkId. By making this generation function available in Blink, this CL prepares for a refactoring effort on the current OffscreenCanvas.commit() workflow--it would facilitate simplification on CanvasSurfaceLayerBridge that will remove the current sync IPC that relies on the browser process to generate FrameSinkId (which is part of SurfaceId) for it. BUG=664547 ========== to ========== Allow Blink to generate cc::FrameSinkId from RenderThread This CL allows Blink to use generate cc::FrameSinkId, internally implemented using RenderThread's functions. Currently, Mus is working on making FrameSinkId being able to generated on Renderer process, getting rid of the need of IPC to browser just to get a FrameSinkId. By making this generation function available in Blink, this CL prepares for a refactoring effort on the current OffscreenCanvas.commit() workflow--it would facilitate simplification on CanvasSurfaceLayerBridge that will remove the current sync IPC that relies on the browser process to generate FrameSinkId (which is part of SurfaceId) for it. BUG=664547 Committed: https://crrev.com/2956c331e20ecb5d730d17fdf637ee72b6fd3a6c Cr-Commit-Position: refs/heads/master@{#434535} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2956c331e20ecb5d730d17fdf637ee72b6fd3a6c Cr-Commit-Position: refs/heads/master@{#434535} |