Make OffscreenCanvas animation in sync with its placeholder canvas's parent frame rate

Make OffscreenCanvas animation in sync with its placeholder canvas's parent frame rate

This CL makes OffscreenCanvasFrameDispatcherImpl::OnBeginFrame get fired when
the OffscreenCanvas's corresponding placeholder canvas's parent frame is
starting a new frame. In this way, we make sure that OffscreenCanvas animation
matches the frame rate of display, which is part of the spec requirement in
OffscreenCanvas
(https://wiki.whatwg.org/wiki/OffscreenCanvas.requestAnimationFrame).

Implementation in this CL can be broadly seen as two parts:

1. Registration of OffscreenCanvas's frame sink id to the FrameSinkHierarchy of
its parent frame's frame sink id. The parent frame sink id is obtained by
plumbing into WebLayerTreeView from HTMLCanvasElement; it is then sent to
OffscreenCanvasSurfaceImpl and cached there. When
OffscreenCanvasCompositorFrameSink is created (together with the support_), we
then register this parent-child hierarchy to SurfaceManager.

2. OffscreenCanvas signals that it needs BeginFrame when users invoke commit().
This allows OffscreenCanvas.beginFrame() to be triggered to either resolve an
existing promise and doCommit on the last overdraw compositorFrame sent by the
user in the same frame window. When a promise is resolved, OffscreenCanvas
signals that it no longer needs BeginFrame, so as to avoid overhead from its
parent frame.

BUG=674744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2644653003
Cr-Commit-Position: refs/heads/master@{#446898}
Committed: https://chromium.googlesource.com/chromium/src/+/d0df339711fb34c94decb02ac90e0d9fddc1e8d1

[xlai (Olivia), 2017-01-18 20:15:46]

xlai@chromium.org changed reviewers:
+ fsamuel@chromium.org, junov@chromium.org

[xlai (Olivia), 2017-01-18 20:15:47]

Sending to fsamuel@ and junov@ first before reaching to other reviewers.

fsamuel@: Could you take a look at the way I register the offscreencanvas frame
sink id to its parent frame sink id to check whether that's okay? 

junov@: Could you take a look at the Blink side to see if the way I handle the
overdraw and non-overdraw frames are alright?

[Fady Samuel, 2017-01-19 04:05:43]

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc
(right):

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:33:
OffscreenCanvasSurfaceManager::GetInstance()->RegisterFrameSinkToItsParent(
Maybe move this to
OffscreenCanvasCompositorFrameSinkProviderImpl::CreateCompositorFrameSink?

The reason being I want this class to continue to look like
GpuCompositorFrameSink so we can merge those two soon.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:39:
->UnregisterFrameSinkFromItsParent(support_.frame_sink_id());
Maybe move this to
OffscreenCanvasCompositorFrameSinkProviderImpl::OnCompositorFrameSinkClientConnectionLost?

The reason being I want this class to continue to look like
GpuCompositorFrameSink so we can merge those two soon.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_surface_impl.h (right):

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_impl.h:50:
cc::FrameSinkId frame_sink_id_;
const

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_impl.h:52:
cc::FrameSinkId parent_frame_sink_id_;
const

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_surface_manager.cc (right):

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:30: void
OffscreenCanvasSurfaceManager::RegisterFrameSinkToItsParent(
nit: RegisterFrameSinkToParent?

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:31:
cc::FrameSinkId child_frame_sink_id) {
nit: const cc::FrameSinkId&

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:42: void
OffscreenCanvasSurfaceManager::UnregisterFrameSinkFromItsParent(
nit: UnregisterFrameSinkFromParent?

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:43:
cc::FrameSinkId child_frame_sink_id) {
const cc::FrameSinkId&

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:65:
cc::FrameSinkId frame_sink_id,
nit: const cc::FrameSinkId&

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:73:
cc::FrameSinkId frame_sink_id) {
nit: const cc::FrameSinkId&

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:79:
cc::FrameSinkId frame_sink_id) {
nit: const cc::FrameSinkId&

https://codereview.chromium.org/2644653003/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2644653003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1121: cc::FrameSinkId
RenderWidgetCompositor::getFrameSinkId() {
Return a const cc::FrameSinkId& instead?

https://codereview.chromium.org/2644653003/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/2644653003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:114: cc::FrameSinkId
getFrameSinkId() override;
Return a const cc::FrameSinkId& instead?

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:72: if
(layerTreeView) {
Is there any reason why this might be null?

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:63:
cc::FrameSinkId m_frameSinkId;
const?

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:65:
cc::FrameSinkId m_parentFrameSinkId;
const?

[xlai (Olivia), 2017-01-19 16:04:01]

Patchset #3 (id:40001) has been deleted

[xlai (Olivia), 2017-01-19 16:43:57]

All comments addressed.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc
(right):

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:33:
OffscreenCanvasSurfaceManager::GetInstance()->RegisterFrameSinkToItsParent(
On 2017/01/19 04:05:43, Fady Samuel wrote:
> Maybe move this to
> OffscreenCanvasCompositorFrameSinkProviderImpl::CreateCompositorFrameSink?
> 
> The reason being I want this class to continue to look like
> GpuCompositorFrameSink so we can merge those two soon.

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink.cc:39:
->UnregisterFrameSinkFromItsParent(support_.frame_sink_id());
On 2017/01/19 04:05:43, Fady Samuel wrote:
> Maybe move this to
>
OffscreenCanvasCompositorFrameSinkProviderImpl::OnCompositorFrameSinkClientConnectionLost?
> 
> The reason being I want this class to continue to look like
> GpuCompositorFrameSink so we can merge those two soon.

Simply moving this to OnCompositorFrameSinkClientConnectionLost will not work,
because OnCompositorFrameSinkClientConnectionLost is not fired when
OffscreenCanvasCompositorFrameSink is destroyed normally (i.e. closing down the
window), which will then result in assert failure in SurfaceManager's
destructor.

My workaround is to introduce
OffscreenCanvasCompositorFrameSinkProviderImpl::OnCompositorFrameSinkClientDestroyed
and call that when compositorframesink is destroyed. With that, I can avoid
seeing the assert error when I close down the window.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_surface_impl.h (right):

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_impl.h:50:
cc::FrameSinkId frame_sink_id_;
On 2017/01/19 04:05:43, Fady Samuel wrote:
> const 

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_impl.h:52:
cc::FrameSinkId parent_frame_sink_id_;
On 2017/01/19 04:05:43, Fady Samuel wrote:
> const

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_surface_manager.cc (right):

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:30: void
OffscreenCanvasSurfaceManager::RegisterFrameSinkToItsParent(
On 2017/01/19 04:05:43, Fady Samuel wrote:
> nit: RegisterFrameSinkToParent?

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:31:
cc::FrameSinkId child_frame_sink_id) {
On 2017/01/19 04:05:43, Fady Samuel wrote:
> nit: const cc::FrameSinkId&

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:42: void
OffscreenCanvasSurfaceManager::UnregisterFrameSinkFromItsParent(
On 2017/01/19 04:05:43, Fady Samuel wrote:
> nit: UnregisterFrameSinkFromParent?

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:43:
cc::FrameSinkId child_frame_sink_id) {
On 2017/01/19 04:05:43, Fady Samuel wrote:
> const cc::FrameSinkId&

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:65:
cc::FrameSinkId frame_sink_id,
On 2017/01/19 04:05:43, Fady Samuel wrote:
> nit: const cc::FrameSinkId&

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:73:
cc::FrameSinkId frame_sink_id) {
On 2017/01/19 04:05:43, Fady Samuel wrote:
> nit: const cc::FrameSinkId&

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.cc:79:
cc::FrameSinkId frame_sink_id) {
On 2017/01/19 04:05:43, Fady Samuel wrote:
> nit: const cc::FrameSinkId&

Done.

https://codereview.chromium.org/2644653003/diff/20001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.h (right):

https://codereview.chromium.org/2644653003/diff/20001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.h:114: cc::FrameSinkId
getFrameSinkId() override;
On 2017/01/19 04:05:43, Fady Samuel wrote:
> Return a const cc::FrameSinkId& instead?

Can't do that. This function is overriding another virtual function in
WebLayerTreeView, which must be creating an empty FrameSinkId in its
implementation.

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:72: if
(layerTreeView) {
On 2017/01/19 04:05:43, Fady Samuel wrote:
> Is there any reason why this might be null?

This variable is passed from HTMLCanvasElement. I've check the reference chain
and found that it can be null only when the parent frame is detached.

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:63:
cc::FrameSinkId m_frameSinkId;
On 2017/01/19 04:05:43, Fady Samuel wrote:
> const?

Done.

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:65:
cc::FrameSinkId m_parentFrameSinkId;
On 2017/01/19 04:05:43, Fady Samuel wrote:
> const?

Done.

[Fady Samuel, 2017-01-19 17:10:54]

lgtm + nits

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
File
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink_provider_impl.cc
(right):

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink_provider_impl.cc:52:
frame_sink_id);
I don't think this is necessary. compositor_frame_sinks_ owns the
CompositorFrameSink.

I think you can simply put this line just below
compositor_frame_sinks_.erase(frame_sink_id); in
OffscreenCanvasCompositorFrmaeSinkProviderImpl::OnCompositorFrameSinkClientConnectionLost.

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_surface_manager.h (right):

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.h:26: void
RegisterFrameSinkToParent(const cc::FrameSinkId&);
chromium style nit: give the param a name:

e.g. const cc::FrameSinkId& frame_sink_id

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.h:27: void
UnregisterFrameSinkFromParent(const cc::FrameSinkId&);
chromium style nit: give the param a name:

e.g. const cc::FrameSinkId& frame_sink_id

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.h:29: void
RegisterOffscreenCanvasSurfaceInstance(const cc::FrameSinkId&,
chromium style nit: give the param a name:

e.g. const cc::FrameSinkId& frame_sink_id

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.h:30:
OffscreenCanvasSurfaceImpl*);
chromium style nit: give the param a name:

e.g OffscreencanvasSurfaceImpl* offscreen_canvas_surface

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.h:31: void
UnregisterOffscreenCanvasSurfaceInstance(const cc::FrameSinkId&);
const cc::FrameSinkId& frame_sink_id

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.h:32:
OffscreenCanvasSurfaceImpl* GetSurfaceInstance(const cc::FrameSinkId&);
const cc::FrameSinkId& frame_sink_id

[Justin Novosad, 2017-01-19 18:06:49]

lgtm for third_party/WebKit/Source/core/ and third_party/WebKit/Source/platform/

+nits

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:270:
getOrCreateFrameDispatcher()->setNeedsBeginFrame(false);
Kudos for putting this in the right place.  A comment would help to explain why
we don't need this in the case where we have an overdrawFrame.

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h
(right):

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h:28:
virtual void setNeedsBeginFrame(bool doesNeedBeginFrame) = 0;
No need to name the argument here (name adds no info).

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:377:
bool doesNeedBeginFrame) {
This name doesNeedBeginFrame is a bit awkward. Just "needsBeginFrame" should not
have any conflicts

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
(right):

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:35:
void setNeedsBeginFrame(bool doesNeedBeginFrame);
Missing "final" (I am surprised this compiles)
No need to name the argument (name adds no info).

[xlai (Olivia), 2017-01-19 19:14:35]

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
File
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink_provider_impl.cc
(right):

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_compositor_frame_sink_provider_impl.cc:52:
frame_sink_id);
On 2017/01/19 17:10:53, Fady Samuel wrote:
> I don't think this is necessary. compositor_frame_sinks_ owns the
> CompositorFrameSink.
> 
> I think you can simply put this line just below
> compositor_frame_sinks_.erase(frame_sink_id); in
>
OffscreenCanvasCompositorFrmaeSinkProviderImpl::OnCompositorFrameSinkClientConnectionLost.

I just double check, OnConnectionLost is not fired when I close the window.

The current situation is that OnCompositorFrameSinkClientConnectionLost is not
fired before SurfaceManager is destroyed, resulting in an assert failure in
DCHECK_EQ(frame_sink_source_map_.size(), 0u); in SurfaceManager's destructor,
when I close down the window. So I think an explicit call to unregister is still
needed in compositorFrameSink's own destructor, until we figure out why the
OnConnectionLost itself is not sufficient. I will put a TODO inside
OnCompositorFrameSinkConnectionLost.

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
File content/browser/renderer_host/offscreen_canvas_surface_manager.h (right):

https://codereview.chromium.org/2644653003/diff/60001/content/browser/rendere...
content/browser/renderer_host/offscreen_canvas_surface_manager.h:26: void
RegisterFrameSinkToParent(const cc::FrameSinkId&);
On 2017/01/19 17:10:54, Fady Samuel wrote:
> chromium style nit: give the param a name:
> 
> e.g. const cc::FrameSinkId& frame_sink_id

Done for here and elsewhere.

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp (right):

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp:270:
getOrCreateFrameDispatcher()->setNeedsBeginFrame(false);
On 2017/01/19 18:06:49, Justin Novosad wrote:
> Kudos for putting this in the right place.  A comment would help to explain
why
> we don't need this in the case where we have an overdrawFrame.

Done.

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h
(right):

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcher.h:28:
virtual void setNeedsBeginFrame(bool doesNeedBeginFrame) = 0;
On 2017/01/19 18:06:49, Justin Novosad wrote:
> No need to name the argument here (name adds no info).

Done.

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:377:
bool doesNeedBeginFrame) {
On 2017/01/19 18:06:49, Justin Novosad wrote:
> This name doesNeedBeginFrame is a bit awkward. Just "needsBeginFrame" should
not
> have any conflicts

Done.

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h
(right):

https://codereview.chromium.org/2644653003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.h:35:
void setNeedsBeginFrame(bool doesNeedBeginFrame);
On 2017/01/19 18:06:49, Justin Novosad wrote:
> Missing "final" (I am surprised this compiles)
> No need to name the argument (name adds no info).

Done.

[xlai (Olivia), 2017-01-19 19:43:49]

xlai@chromium.org changed reviewers:
+ dcheng@chromium.org, kbr@chromium.org

[xlai (Olivia), 2017-01-19 19:43:49]

Sending to more reviewers.

kbr@: Could you please review content/renderer/gpu and
third_party/WebKit/Source/web? Basically, all these changes are just a way to
plumb the html canvas element's parent frame's frame sink id from renderer into
Blink.

dcheng@: Could you please review the .mojom change and WebKit/public/? 

Thank you!

[Ken Russell (switch to Gerrit), 2017-01-20 03:52:47]

lgtm for the parts I own. Nice work.

[dcheng, 2017-01-20 08:44:12]

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:72: if
(layerTreeView) {
On 2017/01/19 16:43:56, xlai (Olivia) wrote:
> On 2017/01/19 04:05:43, Fady Samuel wrote:
> > Is there any reason why this might be null?
> 
> This variable is passed from HTMLCanvasElement. I've check the reference chain
> and found that it can be null only when the parent frame is detached.

Out of curiosity, how do we create this object if the parent frame is detached?

https://codereview.chromium.org/2644653003/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2644653003/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1122: return frame_sink_id_;
Nit: function definitions should be ordered by order of declaration:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1430: if (frame &&
frame->host()) {
No need to check frame->host() as well: if Document::frame() is not null, that
means the frame is attached (which means FrameHost must not be null)

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:386:
if (client())
Can client() actually be null? It seems like it can't from how it's constructed.

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebLayerTreeView.h:188: // Returns the
FrameSinkId of the current render view
As in content::RenderView?

[xlai (Olivia), 2017-01-20 16:36:22]

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:72: if
(layerTreeView) {
On 2017/01/20 08:44:12, dcheng wrote:
> On 2017/01/19 16:43:56, xlai (Olivia) wrote:
> > On 2017/01/19 04:05:43, Fady Samuel wrote:
> > > Is there any reason why this might be null?
> > 
> > This variable is passed from HTMLCanvasElement. I've check the reference
chain
> > and found that it can be null only when the parent frame is detached.
> 
> Out of curiosity, how do we create this object if the parent frame is
detached?

Just double check with junov@. This variable can be null in the case when the
HTMLCanvasElement
does not have a parent frame (i.e. it is under a Document without a Frame). This
is a weird edge case
when the Document should not be seen and therefore no OnBeginFrame signals (and
so doesn't matter the empty parent frame sink id here, browser will simply
ignore it and not register it to frame sink hierarchy). 

But if the canvas is reparented with a new visible Document then we should turn
on OnBeginFrame signals then. I added TODO comment with the new issue
crbug.com/683172 to remind myself to 
follow up with another CL that handles this edge case.

https://codereview.chromium.org/2644653003/diff/80001/content/renderer/gpu/re...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/2644653003/diff/80001/content/renderer/gpu/re...
content/renderer/gpu/render_widget_compositor.cc:1122: return frame_sink_id_;
On 2017/01/20 08:44:12, dcheng wrote:
> Nit: function definitions should be ordered by order of declaration:
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1430: if (frame &&
frame->host()) {
On 2017/01/20 08:44:12, dcheng wrote:
> No need to check frame->host() as well: if Document::frame() is not null, that
> means the frame is attached (which means FrameHost must not be null)

Done.

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:386:
if (client())
On 2017/01/20 08:44:12, dcheng wrote:
> Can client() actually be null? It seems like it can't from how it's
constructed.

It cannot be null. I'm changing this to a DCHECK.

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2644653003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebLayerTreeView.h:188: // Returns the
FrameSinkId of the current render view
On 2017/01/20 08:44:12, dcheng wrote:
> As in content::RenderView?

I've edited the comment to reflect the origin of the returned frame sink id.

[Justin Novosad, 2017-01-20 18:08:21]

On 2017/01/20 16:36:22, xlai (Olivia) wrote:
>
https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
> (right):
> 
>
https://codereview.chromium.org/2644653003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:72:
if
> (layerTreeView) {
> On 2017/01/20 08:44:12, dcheng wrote:
> > On 2017/01/19 16:43:56, xlai (Olivia) wrote:
> > > On 2017/01/19 04:05:43, Fady Samuel wrote:
> > > > Is there any reason why this might be null?
> > > 
> > > This variable is passed from HTMLCanvasElement. I've check the reference
> chain
> > > and found that it can be null only when the parent frame is detached.
> > 
> > Out of curiosity, how do we create this object if the parent frame is
> detached?
> 
> Just double check with junov@. This variable can be null in the case when the
> HTMLCanvasElement
> does not have a parent frame (i.e. it is under a Document without a Frame).
This
> is a weird edge case
> when the Document should not be seen and therefore no OnBeginFrame signals
(and
> so doesn't matter the empty parent frame sink id here, browser will simply
> ignore it and not register it to frame sink hierarchy). 
> 
> But if the canvas is reparented with a new visible Document then we should
turn
> on OnBeginFrame signals then. I added TODO comment with the new issue
> crbug.com/683172 to remind myself to 
> follow up with another CL that handles this edge case.
> 

Could you still add a tests for the frame-less document case in this CL?  It
might be a while before we come around to fixing this case (not required for
launch). For now we need to at least verify that attempting to do this won't
crash or cause any other kind of security-sensitive errors.

[xlai (Olivia), 2017-01-20 19:38:48]

Description was changed from

==========
Make OffscreenCanvas animation in sync with its placeholder canvas's parent
frame rate

This CL makes OffscreenCanvasFrameDispatcherImpl::OnBeginFrame get fired when
the OffscreenCanvas's corresponding placeholder canvas's parent frame is
starting a new frame. In this way, we make sure that OffscreenCanvas animation
matches the frame rate of display, which is part of the spec requirement in
OffscreenCanvas
(https://wiki.whatwg.org/wiki/OffscreenCanvas.requestAnimationFrame).

Implementation in this CL can be broadly seen as two parts:

1. Registration of OffscreenCanvas's frame sink id to the FrameSinkHierarchy of
its parent frame's frame sink id. The parent frame sink id is obtained by
plumbing into WebLayerTreeView from HTMLCanvasElement; it is then sent to
OffscreenCanvasSurfaceImpl and cached there. When
OffscreenCanvasCompositorFrameSink is created (together with the support_), we
then register this parent-child hierarchy to SurfaceManager.

2. OffscreenCanvas signals that it needs BeginFrame when users invoke commit().
This allows OffscreenCanvas.beginFrame() to be triggered to either resolve an
existing promise and doCommit on the last overdraw compositorFrame sent by the
user in the same frame window. When a promise is resolved, OffscreenCanvas
signals that it no longer needs BeginFrame, so as to avoid overhead from its
parent frame.

BUG=674744
==========

to

==========
Make OffscreenCanvas animation in sync with its placeholder canvas's parent
frame rate

This CL makes OffscreenCanvasFrameDispatcherImpl::OnBeginFrame get fired when
the OffscreenCanvas's corresponding placeholder canvas's parent frame is
starting a new frame. In this way, we make sure that OffscreenCanvas animation
matches the frame rate of display, which is part of the spec requirement in
OffscreenCanvas
(https://wiki.whatwg.org/wiki/OffscreenCanvas.requestAnimationFrame).

Implementation in this CL can be broadly seen as two parts:

1. Registration of OffscreenCanvas's frame sink id to the FrameSinkHierarchy of
its parent frame's frame sink id. The parent frame sink id is obtained by
plumbing into WebLayerTreeView from HTMLCanvasElement; it is then sent to
OffscreenCanvasSurfaceImpl and cached there. When
OffscreenCanvasCompositorFrameSink is created (together with the support_), we
then register this parent-child hierarchy to SurfaceManager.

2. OffscreenCanvas signals that it needs BeginFrame when users invoke commit().
This allows OffscreenCanvas.beginFrame() to be triggered to either resolve an
existing promise and doCommit on the last overdraw compositorFrame sent by the
user in the same frame window. When a promise is resolved, OffscreenCanvas
signals that it no longer needs BeginFrame, so as to avoid overhead from its
parent frame.

BUG=674744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

[xlai (Olivia), 2017-01-20 19:40:09]

new pixel test for frameless canvas is now added.

[Justin Novosad, 2017-01-20 21:07:53]

On 2017/01/20 19:40:09, xlai (Olivia) wrote:
> new pixel test for frameless canvas is now added.

re- lgtm

[xlai (Olivia), 2017-01-23 18:45:33]

Gentle ping to dcheng@..

[dcheng, 2017-01-23 21:53:32]

https://codereview.chromium.org/2644653003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2644653003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1434: layerTreeView =
frame->host()->chromeClient().getWebLayerTreeView();
Sorry, one more dumb question: why do we call createLayer() still if the
document is detached? I read the MDN example:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/transferCo...

which appears to create a detached canvas element. But it's not clear to me what
effect committing in a detached canvas element would have. Is this something
that's supposed to work in the future?

https://codereview.chromium.org/2644653003/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2644653003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayerTreeView.h:167: virtual
cc::FrameSinkId getFrameSinkId() { return cc::FrameSinkId(); }
It's a bit of a layering violation to refer to content here.

Perhaps... "FrameSinkId of the widget associated with this layer tree view."?

[xlai (Olivia), 2017-01-24 20:51:06]

Patchset #7 (id:140001) has been deleted

[xlai (Olivia), 2017-01-24 20:54:12]

https://codereview.chromium.org/2644653003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2644653003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1434: layerTreeView =
frame->host()->chromeClient().getWebLayerTreeView();
On 2017/01/23 21:53:32, dcheng wrote:
> Sorry, one more dumb question: why do we call createLayer() still if the
> document is detached? I read the MDN example:
> 
>
https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/transferCo...
> 
> which appears to create a detached canvas element. But it's not clear to me
what
> effect committing in a detached canvas element would have. Is this something
> that's supposed to work in the future?

You are right, it doesn't make sense to have a layer for a frameless canvas.

I have done some changes so that for a frameless canvas, there is no mojo
channels built for this 
edge case. However, since a frameless canvas still needs to be used as
image source, the commit() call will still need to trigger a post of image from
worker thread to main
thread; no browser thread would be involved in this process though.

https://codereview.chromium.org/2644653003/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayerTreeView.h (right):

https://codereview.chromium.org/2644653003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayerTreeView.h:167: virtual
cc::FrameSinkId getFrameSinkId() { return cc::FrameSinkId(); }
On 2017/01/23 21:53:32, dcheng wrote:
> It's a bit of a layering violation to refer to content here.
> 
> Perhaps... "FrameSinkId of the widget associated with this layer tree view."?

Done.

https://codereview.chromium.org/2644653003/diff/160001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-commit-frameless-doc.html
(right):

https://codereview.chromium.org/2644653003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/canvas/OffscreenCanvas-commit-frameless-doc.html:5:
// This test aims to ensure that OffscreenCanvas.commit() does not
Changing the pixel test to layout test.

[xlai (Olivia), 2017-01-24 21:36:53]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 21:37:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 21:59:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 21:59:04]

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...)

[xlai (Olivia), 2017-01-24 22:10:45]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 22:11:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2017-01-24 22:31:37]

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1434: layerTreeView =
frame->host()->chromeClient().getWebLayerTreeView();
For OOPIF, I think we would want to use the layer tree view of the local root.
So we should plumb the current frame() in here so ChromeClient can get the local
root, and then get the layer tree view from WebFrameWidget::getLayerTreeView().

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:65:
m_parentFrameSinkId(layerTreeView ? layerTreeView->getFrameSinkId()
With the latest PS, do we still need a null check here?

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:48:
// mojo channel for this special case.
Can we DCHECK() this now, or are there still instances where it might not be
true?

[xlai (Olivia), 2017-01-24 22:57:13]

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp (right):

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp:1434: layerTreeView =
frame->host()->chromeClient().getWebLayerTreeView();
On 2017/01/24 22:31:37, dcheng wrote:
> For OOPIF, I think we would want to use the layer tree view of the local root.
> So we should plumb the current frame() in here so ChromeClient can get the
local
> root, and then get the layer tree view from
WebFrameWidget::getLayerTreeView().

Done.

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:65:
m_parentFrameSinkId(layerTreeView ? layerTreeView->getFrameSinkId()
On 2017/01/24 22:31:37, dcheng wrote:
> With the latest PS, do we still need a null check here?

Yes, in EmptyChromeClient or Unit test, layertreeview can be nullptr.

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp
(right):

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/OffscreenCanvasFrameDispatcherImpl.cpp:48:
// mojo channel for this special case.
On 2017/01/24 22:31:37, dcheng wrote:
> Can we DCHECK() this now, or are there still instances where it might not be
> true?

No, we cannot simply do a DCHECK. frame sink id is (0, 0) in the case when the
html canvas element is detached, because I don't create surfaceLayerBridge and
when the user transfers control to offscreen, this
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/canvas...)
is not set to a valid frame sink id.

In this case, we do not establish a mojo channel. But this class needs
to exist because we need it to post image from worker thread to main thread.

[xlai (Olivia), 2017-01-24 23:15:40]

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2644653003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:65:
m_parentFrameSinkId(layerTreeView ? layerTreeView->getFrameSinkId()
On 2017/01/24 22:57:12, xlai (Olivia) wrote:
> On 2017/01/24 22:31:37, dcheng wrote:
> > With the latest PS, do we still need a null check here?
> 
> Yes, in EmptyChromeClient or Unit test, layertreeview can be nullptr.

I took away the nullity check. I look through the instances that make
use of EmptyChromeClient and cannot think of a case when a canvas element that
has transfercontrol to offscreen appearing there.

[dcheng, 2017-01-24 23:16:37]

LGTM (we discussed the null checks offline, and we believe it's probably not
needed)

[xlai (Olivia), 2017-01-24 23:17:56]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 23:18:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 00:37:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 00:37:34]

Dry run: Try jobs failed on following builders:
  android_optional_gpu_tests_rel on master.tryserver.chromium.android
(JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...)

[xlai (Olivia), 2017-01-25 02:02:53]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 02:03:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 04:44:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 04:44:16]

Dry run: Try jobs failed on following builders:
  android_optional_gpu_tests_rel on master.tryserver.chromium.android
(JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_opti...)

[xlai (Olivia), 2017-01-27 19:56:19]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 19:57:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 22:23:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:23:38]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2017-01-27 22:24:04]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2017-01-27 22:24:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, kbr@chromium.org, junov@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2644653003/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2017-01-27 22:25:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 22:33:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:33:38]

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_presub...)

[xlai (Olivia), 2017-01-28 00:31:17]

The CQ bit was checked by xlai@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 00:32:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 02:48:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 02:48:36]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2017-01-28 03:19:57]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2017-01-28 03:19:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, junov@chromium.org, dcheng@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2644653003/#ps300001
(title: "rebase again")

[commit-bot: I haz the power, 2017-01-28 03:20:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 03:24:54]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1485573597929820,
"parent_rev": "7b07aa74bf9530e37de46c48ce071f4b11a5bf87", "commit_rev":
"d0df339711fb34c94decb02ac90e0d9fddc1e8d1"}

[commit-bot: I haz the power, 2017-01-28 03:25:28]

Description was changed from

==========
Make OffscreenCanvas animation in sync with its placeholder canvas's parent
frame rate

This CL makes OffscreenCanvasFrameDispatcherImpl::OnBeginFrame get fired when
the OffscreenCanvas's corresponding placeholder canvas's parent frame is
starting a new frame. In this way, we make sure that OffscreenCanvas animation
matches the frame rate of display, which is part of the spec requirement in
OffscreenCanvas
(https://wiki.whatwg.org/wiki/OffscreenCanvas.requestAnimationFrame).

Implementation in this CL can be broadly seen as two parts:

1. Registration of OffscreenCanvas's frame sink id to the FrameSinkHierarchy of
its parent frame's frame sink id. The parent frame sink id is obtained by
plumbing into WebLayerTreeView from HTMLCanvasElement; it is then sent to
OffscreenCanvasSurfaceImpl and cached there. When
OffscreenCanvasCompositorFrameSink is created (together with the support_), we
then register this parent-child hierarchy to SurfaceManager.

2. OffscreenCanvas signals that it needs BeginFrame when users invoke commit().
This allows OffscreenCanvas.beginFrame() to be triggered to either resolve an
existing promise and doCommit on the last overdraw compositorFrame sent by the
user in the same frame window. When a promise is resolved, OffscreenCanvas
signals that it no longer needs BeginFrame, so as to avoid overhead from its
parent frame.

BUG=674744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

to

==========
Make OffscreenCanvas animation in sync with its placeholder canvas's parent
frame rate

This CL makes OffscreenCanvasFrameDispatcherImpl::OnBeginFrame get fired when
the OffscreenCanvas's corresponding placeholder canvas's parent frame is
starting a new frame. In this way, we make sure that OffscreenCanvas animation
matches the frame rate of display, which is part of the spec requirement in
OffscreenCanvas
(https://wiki.whatwg.org/wiki/OffscreenCanvas.requestAnimationFrame).

Implementation in this CL can be broadly seen as two parts:

1. Registration of OffscreenCanvas's frame sink id to the FrameSinkHierarchy of
its parent frame's frame sink id. The parent frame sink id is obtained by
plumbing into WebLayerTreeView from HTMLCanvasElement; it is then sent to
OffscreenCanvasSurfaceImpl and cached there. When
OffscreenCanvasCompositorFrameSink is created (together with the support_), we
then register this parent-child hierarchy to SurfaceManager.

2. OffscreenCanvas signals that it needs BeginFrame when users invoke commit().
This allows OffscreenCanvas.beginFrame() to be triggered to either resolve an
existing promise and doCommit on the last overdraw compositorFrame sent by the
user in the same frame window. When a promise is resolved, OffscreenCanvas
signals that it no longer needs BeginFrame, so as to avoid overhead from its
parent frame.

BUG=674744
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2644653003
Cr-Commit-Position: refs/heads/master@{#446898}
Committed:
https://chromium.googlesource.com/chromium/src/+/d0df339711fb34c94decb02ac90e...
==========

[commit-bot: I haz the power, 2017-01-28 03:25:30]

Committed patchset #14 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/d0df339711fb34c94decb02ac90e...