Establish mojo service between Canvas (blink) and SurfaceManager (browser)

Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface. Some codes are added to an existing layout test to test if
the whole workflow works.

Client:
CanvasSurfaceLayerBridge is HTMLCanvasElement's communicator with SurfaceLayer.
It is created only when HTMLCanvasElement.transferControlToOffscreen() is invoked.
It has an instance of CanvasSurfaceLayerBridgeClient, which is a virtual interface
containing all mojo calls. To create a SurfaceLayer, it sends a sync msg
"GetSurfaceId" to browser to retrieve a unique SurfaceId. Immediately after that,
it sends an async msg "RequestSurfaceCreation" to request Surface getting created
for this particular Id. After that, it creates a SurfaceLayer (with SatisfyCallback
and RequireCallback) for HTMLCanvasElement and set the SurfaceId for the layer and
registers it to the graphics layer. A unit test is added; it fakes the browser side
and mocks the BridgeClient.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message pipe.
It is created when all the mojo connections are registered in render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId generation,
SurfaceFactory and Surface creation accordingly. It is a client of SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. I use typemaps to
convert cc::SurfaceId and cc::SurfaceSequence to the corresponding mojo intermediate
structs.

BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e93a0d29e98d8770319072162eb420fd81758f6d
Cr-Commit-Position: refs/heads/master@{#400838}

[xlai (Olivia), 2016-06-02 21:37:32]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked that push
CompositorFrame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so it will not block renderer/main for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked that push
CompositorFrame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so it will not block renderer/main for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it mocks the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

[xlai (Olivia), 2016-06-02 21:37:49]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked that push
CompositorFrame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so it will not block renderer/main for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it mocks the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked that push
CompositorFrame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so it will not block renderer/main for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

[xlai (Olivia), 2016-06-02 21:39:48]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked that push
CompositorFrame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so it will not block renderer/main for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked action that pushes
CompositorFrame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so it will not block renderer/main for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

[xlai (Olivia), 2016-06-02 21:40:34]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked action that pushes
CompositorFrame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so it will not block renderer/main for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked action that pushes
frame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so renderer/main is not blocked for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

[xlai (Olivia), 2016-06-02 22:18:58]

xlai@chromium.org changed reviewers:
+ chrishtr@chromium.org, dcheng@chromium.org, jbauman@chromium.org,
junov@chromium.org, piman@chromium.org

[xlai (Olivia), 2016-06-02 22:18:59]

This patch creates a new mojo service between canvas and surface management in
browser side, as part of the implementation of commit() flow in
OffscreenCanvas(https://wiki.whatwg.org/wiki/OffscreenCanvas).

Messages to individual code reviewers:

jbauman@: As in our previous discussion, I've created a new class inside
content/browser/renderer_host/ that's responsible in creating Surface. On the
client side, I created SurfaceLayer in CanvasSurfaceLayerBridge. Could you take
a look at this and see if the Surface-related code is right?

junov@: Could you take a look at CanvasSurfaceLayerBridge and its unit test? I
think we need a browser test or layout test in the future when we implement the
push commit. But right now I think a unit test will suffice. 

dcheng@: I'm adding a new mojo service in WebKit/public/platform/modules. Some
of them are highly similar to existing non-mojo IPC calls (e.g.
FrameHostMsg_RequireSequence, FrameHostMsg_SatisfySequence, etc). Could you
please review it?

piman@: I'm adding a new class to content/browser/renderer_host and would like
to have your approval.

chrishtr@: I'm adding new folder to public/platform/modules and would like to
have your approval.

Also, cc danakj@, ajuma@, jbroman@, fsamuel@, xidachen@ for interest. Comments
are welcomed.

[Fady Samuel, 2016-06-02 22:32:22]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2016-06-02 22:32:23]

My high level comment is I don't think you should be using a sync IPC. Surface
IDs have three components so that you can allocate local IDs and nonces client
side. Please also use struct traits and not type converters. Add them to cc/ipc
as necessary. Thanks!

https://codereview.chromium.org/2036663003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc
(right):

https://codereview.chromium.org/2036663003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:17:
static cc::SurfaceId Convert(const blink::mojom::SurfaceIdPtr& input) {
Please avoid using type converters.

https://codereview.chromium.org/2036663003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:90:
callback.Run(std::move(surface_id_ptr));
This shouldn't need to be sync.

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp
(right):

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:22:
struct TypeConverter<cc::SurfaceId, blink::mojom::blink::SurfaceIdPtr> {
Please avoid using type converters and use the cc::mojom::SurfaceId.

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:42:
struct TypeConverter<cc::SurfaceSequence,
blink::mojom::blink::SurfaceSequencePtr> {
This needs a StructTraits in cc/ipc. Pleasae avoid using type converters.
Consider them deprecated.

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom
(right):

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom:22:
GetSurfaceId() => (SurfaceId surface_id);
This shouldn't need to be sync.

[Justin Novosad, 2016-06-07 19:58:45]

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom
(right):

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom:22:
GetSurfaceId() => (SurfaceId surface_id);
On 2016/06/02 22:32:23, Fady Samuel wrote:
> This shouldn't need to be sync.

It is sync because the JS API that calls this
(HTMLCanvasElement.transferControlToOffscreen) is synchronous.  We absolutely
need a valid surface ID before that JS method returns.

[Fady Samuel, 2016-06-07 20:03:08]

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom
(right):

https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom:22:
GetSurfaceId() => (SurfaceId surface_id);
On 2016/06/07 19:58:44, Justin Novosad wrote:
> On 2016/06/02 22:32:23, Fady Samuel wrote:
> > This shouldn't need to be sync.
> 
> It is sync because the JS API that calls this
> (HTMLCanvasElement.transferControlToOffscreen) is synchronous.  We absolutely
> need a valid surface ID before that JS method returns.

SurfaceIds don't need to be allocated in the browser although the code is
currently structured that way. Ideally the renderer should be allocating the IDs
within its namespace but that's not what we are doing today. We need to fix
that. However I'm not going to block this CL on that.

[Justin Novosad, 2016-06-07 20:09:26]

On 2016/06/07 20:03:08, Fady Samuel wrote:
>
https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
> File
>
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom
> (right):
> 
>
https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
>
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom:22:
> GetSurfaceId() => (SurfaceId surface_id);
> On 2016/06/07 19:58:44, Justin Novosad wrote:
> > On 2016/06/02 22:32:23, Fady Samuel wrote:
> > > This shouldn't need to be sync.
> > 
> > It is sync because the JS API that calls this
> > (HTMLCanvasElement.transferControlToOffscreen) is synchronous.  We
absolutely
> > need a valid surface ID before that JS method returns.
> 
> SurfaceIds don't need to be allocated in the browser although the code is
> currently structured that way. Ideally the renderer should be allocating the
IDs
> within its namespace but that's not what we are doing today. We need to fix
> that. However I'm not going to block this CL on that.

Interesting.  So a follow-up optimization CL would be to have a synchronous call
only the first time transferControlToOffscreen is called (to obtain a
namespace)?

[Fady Samuel, 2016-06-07 20:20:58]

On 2016/06/07 20:09:26, Justin Novosad wrote:
> On 2016/06/07 20:03:08, Fady Samuel wrote:
> >
>
https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
> > File
> >
>
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom
> > (right):
> > 
> >
>
https://codereview.chromium.org/2036663003/diff/1/third_party/WebKit/public/p...
> >
>
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom:22:
> > GetSurfaceId() => (SurfaceId surface_id);
> > On 2016/06/07 19:58:44, Justin Novosad wrote:
> > > On 2016/06/02 22:32:23, Fady Samuel wrote:
> > > > This shouldn't need to be sync.
> > > 
> > > It is sync because the JS API that calls this
> > > (HTMLCanvasElement.transferControlToOffscreen) is synchronous.  We
> absolutely
> > > need a valid surface ID before that JS method returns.
> > 
> > SurfaceIds don't need to be allocated in the browser although the code is
> > currently structured that way. Ideally the renderer should be allocating the
> IDs
> > within its namespace but that's not what we are doing today. We need to fix
> > that. However I'm not going to block this CL on that.
> 
> Interesting.  So a follow-up optimization CL would be to have a synchronous
call
> only the first time transferControlToOffscreen is called (to obtain a
> namespace)?

Well, we need a namespace for the main thread anyway. The ideal solution would
be to allocate a namespace as soon as the render process is created and pass it
along very early on in RendererPreferences or something. Then the local_id and
nonce can be allocated in the renderer for each surface without any IPCs. In
fact, we shouldn't need to "create" a surface once we allocate an ID. It should
really be just submitting CompositorFrames with an associated ID. Thus
RequestSurfaceCreation can go away as well too!

[xlai (Olivia), 2016-06-09 20:07:50]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked action that pushes
frame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so renderer/main is not blocked for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked action that pushes
frame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so renderer/main is not blocked for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[xlai (Olivia), 2016-06-10 17:41:10]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. Note that we must have "GetSurfaceId" to be sync, otherwise
there will be a race condition for the next user-invoked action that pushes
frame
to Surface (not implemented yet); however, what browser needs to do here is just
generating a unique SurfaceId so renderer/main is not blocked for long. Also,
RequireCallback will only be invoked after the surface layer is registered to
layer tree host; so we do not worry the race condition when SurfaceManager may
be
searching a non-existent Surface.
A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. One important
aspect of this service is that I implement type converters between cc::SurfaceId
and the SurfaceId struct defined in mojo service, as well as that between
cc::SurfaceSequence and the SurfaceSequence struct in mojo service.


BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface. Some codes are added to an existing layout test to test if
the whole workflow works.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. I use typemaps
to
convert cc::SurfaceId and cc::SurfaceSequence to the corresponding mojo
intermediate
structs.


BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[xlai (Olivia), 2016-06-10 18:08:03]

xlai@chromium.org changed reviewers:
+ danakj@chromium.org

[xlai (Olivia), 2016-06-10 18:08:04]

PTAL. This patch creates a mojo service and implements the first two
mojo messages as described in this sequence diagram
(https://sequencediagram.googleplex.com/edit/5698180926668800).

To fsamuel@: 
I have addressed your concerns about type converters by removing all
 of them and replace it with typemaps. I have consulted yzshen@ that
 in order to use typemap both inside Blink and outside Blink, I must 
have two identical typemaps placed inside Blink and outside Blink; 
fortunately I can share one same struct_trait header by using a partial
 template specialization. In addition, I am able to import the existing
 cc/ipc/surface_id.mojom and cc/ipc/surface_sequence.mojom after landing 
https://codereview.chromium.org/2043833002/.
For the Sync IPC concern, based on our previous conversion, I cannot avoid
it because we need a valid (not a partial) SurfaceId to create layer, as 
what junov@ said in previous comment. But I put a TODO there so as long as
the Surface creation API is changed we'll come up with a follow-up CL to
change accordingly.

To junov@:
I have added some additional code to an existing layout test 
fast/canvas/OffscreenCanvas-transferControlToOffscreen.html so that we
can be assured that the layer is created without errors, even though right
now we can't have visual proof because we haven't pushed a frame.

I'm also adding danakj@ to reviewers to inspect the dependency between cc
and Blink.

[Justin Novosad, 2016-06-10 18:48:16]

lgtm with nit for core/html modules/canvas and platform/graphics

https://codereview.chromium.org/2036663003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp
(right):

https://codereview.chromium.org/2036663003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/canvas/HTMLCanvasElementModule.cpp:27:
exceptionState.throwDOMException(V8GeneralError, "Layer creation for offscreen
canvas fails.");
Nit: fails->failed
Also I don't think we should mention layers here: the user has no concept of
layers around this API. This is caused by an IPC timeout, right? Perhaps we want
to give users a bit of an explanation so that they know it's not their code that
has an error. Perhaps: "Offscreen canvas creation failed due to an internal
timeout."

[xlai (Olivia), 2016-06-13 03:08:18]

Patchset #5 (id:80001) has been deleted

[Fady Samuel, 2016-06-13 14:54:07]

lgtm

[danakj, 2016-06-13 18:38:11]

https://codereview.chromium.org/2036663003/diff/100001/cc/ipc/cc_ipc.gyp
File cc/ipc/cc_ipc.gyp (right):

https://codereview.chromium.org/2036663003/diff/100001/cc/ipc/cc_ipc.gyp#newc...
cc/ipc/cc_ipc.gyp:68:
'../../third_party/WebKit/Source/platform/mojo/SurfaceId.typemap',
I think you need the inverse. cc does not depend on blink.

[danakj, 2016-06-13 18:40:04]

https://codereview.chromium.org/2036663003/diff/100001/cc/ipc/cc_ipc.gyp
File cc/ipc/cc_ipc.gyp (right):

https://codereview.chromium.org/2036663003/diff/100001/cc/ipc/cc_ipc.gyp#newc...
cc/ipc/cc_ipc.gyp:68:
'../../third_party/WebKit/Source/platform/mojo/SurfaceId.typemap',
On 2016/06/13 18:38:10, danakj wrote:
> I think you need the inverse. cc does not depend on blink.

Also, if you change .gyp you should also change BUILD.gn normally. Is that not
applicable here?

[xlai (Olivia), 2016-06-13 19:21:52]

Patchset #6 (id:120001) has been deleted

[xlai (Olivia), 2016-06-13 19:31:27]

To danakj@: I tried to remove the duplicate typemap in Blink and it works in
both GN and GYP in my machine. I think it's because a duplicate typemap is only
required when one wants to map the same item to different types inside and
outside Blink; but in my case I don't actually need to do that. As a result, the
cc-Blink dependency concern you mentioned is removed; no change to
cc/ipc/cc_ipc.gyp in this patch.

[danakj, 2016-06-13 20:02:46]

https://codereview.chromium.org/2036663003/diff/140001/cc/ipc/surface_id_stru...
File cc/ipc/surface_id_struct_traits.h (right):

https://codereview.chromium.org/2036663003/diff/140001/cc/ipc/surface_id_stru...
cc/ipc/surface_id_struct_traits.h:12: template <typename T>
Do you still need these then? What are the types that T will resolve to?

[xlai (Olivia), 2016-06-13 20:20:14]

https://codereview.chromium.org/2036663003/diff/140001/cc/ipc/surface_id_stru...
File cc/ipc/surface_id_struct_traits.h (right):

https://codereview.chromium.org/2036663003/diff/140001/cc/ipc/surface_id_stru...
cc/ipc/surface_id_struct_traits.h:12: template <typename T>
On 2016/06/13 20:02:46, danakj wrote:
> Do you still need these then? What are the types that T will resolve to?

Yes, I need this partial template specialization. "T" is resolved to
"cc::mojom::SurfaceId" in the mojom generated file surface_id.mojom.h, as well
as "cc::mojom::blink::SurfaceId" in another mojom generated file called
surface_id.mojom-blink.h. The former one is used outside Blink and is needed in
offscreen_canvas_surface_service_impl for proper type conversion. The latter one
is used in CanvasSurfaceLayerBridge for proper type conversion.

[danakj, 2016-06-13 20:24:20]

cc/ changes LGTM. Let me know if there's anything else you'd like me to look at.

https://codereview.chromium.org/2036663003/diff/140001/cc/ipc/surface_id_stru...
File cc/ipc/surface_id_struct_traits.h (right):

https://codereview.chromium.org/2036663003/diff/140001/cc/ipc/surface_id_stru...
cc/ipc/surface_id_struct_traits.h:12: template <typename T>
On 2016/06/13 20:20:14, xlai (Olivia) wrote:
> On 2016/06/13 20:02:46, danakj wrote:
> > Do you still need these then? What are the types that T will resolve to?
> 
> Yes, I need this partial template specialization. "T" is resolved to
> "cc::mojom::SurfaceId" in the mojom generated file surface_id.mojom.h, as well
> as "cc::mojom::blink::SurfaceId" in another mojom generated file called
> surface_id.mojom-blink.h. The former one is used outside Blink and is needed
in
> offscreen_canvas_surface_service_impl for proper type conversion. The latter
one
> is used in CanvasSurfaceLayerBridge for proper type conversion. 

Ah ok thanks. Can you leave a comment about these two types here?

[danakj, 2016-06-13 20:53:39]

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

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:42:
const cc::SurfaceLayer::SatisfyCallback& satisfyCallback =
createBaseCallback(bind<cc::SurfaceSequence>(&CanvasSurfaceLayerBridge::satisfyCallback,
this));
don't use a const& here for the lvalue, you're storing and owning the lifetime
of the thing when you create it.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:44:
m_surfaceLayer = cc::SurfaceLayer::Create(satisfyCallback, requireCallback);
you can std::move the callbacks here, to drop the local ownership

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:45:
m_surfaceLayer->SetSurfaceId(m_surfaceId, 1.f, gfx::Size(canvasWidth,
canvasHeight));
How are you going to handle resizing (do you need to handle it)?

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:47: //
Convert SurfaceLayer to WebLayer and register it.
style: This type of comment that just says what the code says is discouraged.
Comments should always explain "why" something is done. If all they say is
"what" it does, drop the comment and let the code speak for itself.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:48:
m_webLayer =
adoptPtr(Platform::current()->compositorSupport()->createLayerFromCCLayer(m_surfaceLayer.get()));
Maybe you don't need to go through Platform etc anymore, and just do like this
and construct a WebLayerImpl:
https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0...

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:63:
bool CanvasSurfaceLayerBridge::syncGetServiceId()
do you mean GetSurfaceId?

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:34: //
Virtual functions for unit tests
nit: end comments with punctuation

A common strategy is to pass in a virtual interface to this class that provides
functionality you want to mock/override in tests, rather than putting them on
the class itself. This prevents coupling the implementation of these functions
to the class. Up to you if that makes sense here.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:37:
cc::SurfaceId m_surfaceId;
use whitespace to make this separate from the comment above.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:37:
cc::SurfaceId m_surfaceId;
This could be private if you made syncGetServiceId() return the SurfaceId (and
null for failure)?

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:48:
#endif // CanvasSurfaceLayerBridge
_h ?

[Justin Novosad, 2016-06-13 21:03:19]

On 2016/06/13 20:53:39, danakj wrote:

> How are you going to handle resizing (do you need to handle it)?

Nope, resizing will not be supported, at least not in v1. Mozilla is
implementing it this way too.
For now, apps that need to resize will have to create a new canvas and discard
the old one.

[danakj, 2016-06-13 21:30:40]

On 2016/06/13 21:03:19, Justin Novosad wrote:
> On 2016/06/13 20:53:39, danakj wrote:
> 
> > How are you going to handle resizing (do you need to handle it)?
> 
> Nope, resizing will not be supported, at least not in v1. Mozilla is
> implementing it this way too.
> For now, apps that need to resize will have to create a new canvas and discard
> the old one.

When that happens, I assume you make a new Bridge object then, rather than
reusing it and calling createSurfaceLayer() again?

If that's true maybe it makes sense to call that method Initialize() instead, as
it's something you always call after constructing the class, and only call once.
That might help document how to (or how not to) use the class.

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

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:24:
Platform::current()->serviceRegistry()->connectToRemoteService(mojo::GetProxy(&m_service));
It occured to me another way to deal with testing would be to have this service
be implemented by the tests, since this is the "virtual interface" in this case.
Then have tests do test-only things in the calls to this interface, instead of
overriding methods in this class to not use mojo.

I think that is how we intend to test things in cc, and leaves you without
ForTesting-type stuff in the code here.

[piman, 2016-06-14 00:28:58]

LGTM+nit for content/

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc
(right):

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:59:
LOG(ERROR) << "Attempting to require callback on nonexistent surface";
nit: DLOG

[jbauman, 2016-06-14 00:57:22]

lgtm

[dcheng, 2016-06-14 09:56:45]

mojom lgtm with nits

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc
(right):

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:26:
id_allocator_ = CreateSurfaceIdAllocator();
Nit: Put this in the init list for consistency?

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:41:
callback.Run(std::move(surface_id));
I don't think std::move() here has any effect, unless I'm missing something.

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:48:
surface_factory_ = base::WrapUnique(new cc::SurfaceFactory(manager, this));
Nit: base::MakeUnique<cc::SurfaceFactory>(manager, this)

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_service_impl.h
(right):

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.h:25:
~OffscreenCanvasSurfaceServiceImpl() override;
You can make this private (StrongBinding will delete through the base-class
pointer)

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.h:27: void
GetSurfaceId(const GetSurfaceIdCallback& callback) override;
Nit: comment on which thing this is overriding (like the comment on line 33)

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.h:35: void
WillDrawSurface(cc::SurfaceId id, const gfx::Rect& damage_rect) override;
It's not introduced by this CL, but how come we pass SurfaceId by value and
sometimes we pass by const ref?

[xlai (Olivia), 2016-06-14 21:10:18]

Based on discussion with danakj@, I cleaned up the unit test for
CanvasSurfaceLayerBridge so that the testing does not interfere with production
code, by abstracting the mojo calls in a virtual interface.

Also fixed the nits based on comments from various reviewers.

https://codereview.chromium.org/2036663003/diff/140001/cc/ipc/surface_id_stru...
File cc/ipc/surface_id_struct_traits.h (right):

https://codereview.chromium.org/2036663003/diff/140001/cc/ipc/surface_id_stru...
cc/ipc/surface_id_struct_traits.h:12: template <typename T>
On 2016/06/13 20:24:19, danakj wrote:
> On 2016/06/13 20:20:14, xlai (Olivia) wrote:
> > On 2016/06/13 20:02:46, danakj wrote:
> > > Do you still need these then? What are the types that T will resolve to?
> > 
> > Yes, I need this partial template specialization. "T" is resolved to
> > "cc::mojom::SurfaceId" in the mojom generated file surface_id.mojom.h, as
well
> > as "cc::mojom::blink::SurfaceId" in another mojom generated file called
> > surface_id.mojom-blink.h. The former one is used outside Blink and is needed
> in
> > offscreen_canvas_surface_service_impl for proper type conversion. The latter
> one
> > is used in CanvasSurfaceLayerBridge for proper type conversion. 
> 
> Ah ok thanks. Can you leave a comment about these two types here?

Done.

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc
(right):

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:26:
id_allocator_ = CreateSurfaceIdAllocator();
On 2016/06/14 09:56:44, dcheng wrote:
> Nit: Put this in the init list for consistency?

Done.

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:41:
callback.Run(std::move(surface_id));
On 2016/06/14 09:56:44, dcheng wrote:
> I don't think std::move() here has any effect, unless I'm missing something.

Done.

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:48:
surface_factory_ = base::WrapUnique(new cc::SurfaceFactory(manager, this));
On 2016/06/14 09:56:44, dcheng wrote:
> Nit: base::MakeUnique<cc::SurfaceFactory>(manager, this)

Done.

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.cc:59:
LOG(ERROR) << "Attempting to require callback on nonexistent surface";
On 2016/06/14 00:28:58, piman wrote:
> nit: DLOG

Done.

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
File content/browser/renderer_host/offscreen_canvas_surface_service_impl.h
(right):

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.h:25:
~OffscreenCanvasSurfaceServiceImpl() override;
On 2016/06/14 09:56:45, dcheng wrote:
> You can make this private (StrongBinding will delete through the base-class
> pointer)

Done.

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.h:27: void
GetSurfaceId(const GetSurfaceIdCallback& callback) override;
On 2016/06/14 09:56:45, dcheng wrote:
> Nit: comment on which thing this is overriding (like the comment on line 33)

Done.

https://codereview.chromium.org/2036663003/diff/140001/content/browser/render...
content/browser/renderer_host/offscreen_canvas_surface_service_impl.h:35: void
WillDrawSurface(cc::SurfaceId id, const gfx::Rect& damage_rect) override;
On 2016/06/14 09:56:45, dcheng wrote:
> It's not introduced by this CL, but how come we pass SurfaceId by value and
> sometimes we pass by const ref?

When I implement the functions in the next patch (Issue 619136), I'll see if I
can clean up this part on cc surfaceFactory API if it hasn't been done.

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

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:42:
const cc::SurfaceLayer::SatisfyCallback& satisfyCallback =
createBaseCallback(bind<cc::SurfaceSequence>(&CanvasSurfaceLayerBridge::satisfyCallback,
this));
On 2016/06/13 20:53:39, danakj wrote:
> don't use a const& here for the lvalue, you're storing and owning the lifetime
> of the thing when you create it.

Done.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:44:
m_surfaceLayer = cc::SurfaceLayer::Create(satisfyCallback, requireCallback);
On 2016/06/13 20:53:39, danakj wrote:
> you can std::move the callbacks here, to drop the local ownership

Done.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:47: //
Convert SurfaceLayer to WebLayer and register it.
On 2016/06/13 20:53:39, danakj wrote:
> style: This type of comment that just says what the code says is discouraged.
> Comments should always explain "why" something is done. If all they say is
> "what" it does, drop the comment and let the code speak for itself.

Done. Removed superficial comments.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:48:
m_webLayer =
adoptPtr(Platform::current()->compositorSupport()->createLayerFromCCLayer(m_surfaceLayer.get()));
On 2016/06/13 20:53:38, danakj wrote:
> Maybe you don't need to go through Platform etc anymore, and just do like this
> and construct a WebLayerImpl:
>
https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=0...

I tried this and find that it can only be done by including the whole cc_blink
target into WebKit/Source/platform. Right now, everybody in WebKit access
functions in cc_blink via this WebKit/public/platform/WebCompositorSupport.h; so
I'd rather not to break this dependency in this patch. If we want to change this
dependency, I would rather do it on a separate patch and then change everyone
else simultaneously as well.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:63:
bool CanvasSurfaceLayerBridge::syncGetServiceId()
On 2016/06/13 20:53:39, danakj wrote:
> do you mean GetSurfaceId?

The function name style in Blink starts with small letter. To distinguish it
from the mojo functions itself, I put sync as a prefix.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:34: //
Virtual functions for unit tests
On 2016/06/13 20:53:39, danakj wrote:
> nit: end comments with punctuation
> 
> A common strategy is to pass in a virtual interface to this class that
provides
> functionality you want to mock/override in tests, rather than putting them on
> the class itself. This prevents coupling the implementation of these functions
> to the class. Up to you if that makes sense here.

Done. Removed all the virtual testing things from Bridge and introduce
CanvasSurfaceLayerBridgeClient (a virtual interface) into it.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:37:
cc::SurfaceId m_surfaceId;
On 2016/06/13 20:53:39, danakj wrote:
> This could be private if you made syncGetServiceId() return the SurfaceId (and
> null for failure)?

Done.

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:48:
#endif // CanvasSurfaceLayerBridge
On 2016/06/13 20:53:39, danakj wrote:
> _h ?

Done.

[danakj, 2016-06-14 22:07:00]

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

https://codereview.chromium.org/2036663003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:63:
bool CanvasSurfaceLayerBridge::syncGetServiceId()
On 2016/06/14 21:10:17, xlai (Olivia) wrote:
> On 2016/06/13 20:53:39, danakj wrote:
> > do you mean GetSurfaceId?
> 
> The function name style in Blink starts with small letter. To distinguish it
> from the mojo functions itself, I put sync as a prefix.

Oh oops, I was referring to Service vs Surface.

[xlai (Olivia), 2016-06-16 18:38:45]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface. Some codes are added to an existing layout test to test if
the whole workflow works.

Client:
CanvasSurfaceLayerBridge is the main communicator for HTMLCanvasElement. It is
created only when user invokes HTMLCanvasElement.transferControlToOffscreen().
To create a SurfaceLayer, it sends a sync msg "GetSurfaceId" to browser to
retrieve a unique SurfaceId. Immediately after that, it sends an async msg
"RequestSurfaceCreation" to request Surface getting created for this particular
Id. After that, it creates a SurfaceLayer (with SatisfyCallback and
RequireCallback)
for HTMLCanvasElement and set the SurfaceId for the layer and registers it to
the graphics layer. A unit test is added; it fakes the browser side.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. I use typemaps
to
convert cc::SurfaceId and cc::SurfaceSequence to the corresponding mojo
intermediate
structs.


BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface. Some codes are added to an existing layout test to test if
the whole workflow works.

Client:
CanvasSurfaceLayerBridge is HTMLCanvasElement's communicator with SurfaceLayer.
It is created only when HTMLCanvasElement.transferControlToOffscreen() is
invoked.
It has an instance of CanvasSurfaceLayerBridgeClient, which is a virtual
interface
containing all mojo calls. To create a SurfaceLayer, it sends a sync msg
"GetSurfaceId" to browser to retrieve a unique SurfaceId. Immediately after
that,
it sends an async msg "RequestSurfaceCreation" to request Surface getting
created
for this particular Id. After that, it creates a SurfaceLayer (with
SatisfyCallback
and RequireCallback) for HTMLCanvasElement and set the SurfaceId for the layer
and
registers it to the graphics layer. A unit test is added; it fakes the browser
side
and mocks the BridgeClient.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. I use typemaps
to
convert cc::SurfaceId and cc::SurfaceSequence to the corresponding mojo
intermediate
structs.


BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[xlai (Olivia), 2016-06-16 18:44:55]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-06-16 18:44:55]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
fsamuel@chromium.org, piman@chromium.org, danakj@chromium.org,
dcheng@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2036663003/#ps220001
(title: "Fix GN build in cast_shell_linux")

[commit-bot: I haz the power, 2016-06-16 18:45:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/220001

[danakj, 2016-06-16 18:49:45]

Nice work with the client, LGTM

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

https://codereview.chromium.org/2036663003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:22:
m_client = std::move(client);
fwiw, normally a client would be owned by the thing that owns the
CanvasSurfaceLayerBridge, and it would just give a raw pointer here. This is
fine in this case for now though.

[xlai (Olivia), 2016-06-16 19:13:31]

The CQ bit was unchecked by xlai@chromium.org

[xlai (Olivia), 2016-06-16 20:28:45]

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

[commit-bot: I haz the power, 2016-06-16 20:29:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/240001

[jbroman, 2016-06-16 21:11:48]

I hope you don't mind a couple of drive-by comments about the platform/graphics/
bit of this.

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

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:27:
m_client.reset();
OwnPtrs should be automatically deleted. If this requires special handling,
would you mind adding a comment explaining why?

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:26:
virtual ~CanvasSurfaceLayerBridge();
I don't think this destructor needs to be virtual, does it?

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h
(right):

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h:22:
virtual void asyncRequestSurfaceCreation(const cc::SurfaceId&) = 0;
Elsewhere you pass cc::SurfaceId by value, but here it's by const&. Is this
intentional?

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
(right):

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:62:
FakeOffscreenCanvasSurfaceServiceImpl* m_surfaceService;
This seems to be an owning pointer, too.

std::unique_ptr (or OwnPtr, though that's on its way out), or simpler yet this
could probably just be a FakeOffscreenCanvasSurfaceServiceImpl), please?

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:94:
surfaceId = new cc::SurfaceId(10, 15, 0);
This doesn't set the out parameter; it just rewrites the local variable. I think
you want something like:

*surfaceId = cc::SurfaceId(10, 15, 0);

[commit-bot: I haz the power, 2016-06-16 22:16:04]

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

[commit-bot: I haz the power, 2016-06-16 22:16:06]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2016-06-16 22:38:44]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-06-16 22:38:44]

The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org,
piman@chromium.org, danakj@chromium.org, dcheng@chromium.org,
jbauman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2036663003/#ps240001
(title: "Fix GN build in android_dbg_recipe")

[commit-bot: I haz the power, 2016-06-16 22:39:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/240001

[xlai (Olivia), 2016-06-16 22:39:36]

The CQ bit was unchecked by xlai@chromium.org

[xlai (Olivia), 2016-06-16 23:16:21]

Edit based on jbroman's feedback

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

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.cpp:27:
m_client.reset();
On 2016/06/16 21:11:48, jbroman wrote:
> OwnPtrs should be automatically deleted. If this requires special handling,
> would you mind adding a comment explaining why?

Done.

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h
(right):

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridge.h:26:
virtual ~CanvasSurfaceLayerBridge();
On 2016/06/16 21:11:48, jbroman wrote:
> I don't think this destructor needs to be virtual, does it?

Done.

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h
(right):

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h:22:
virtual void asyncRequestSurfaceCreation(const cc::SurfaceId&) = 0;
On 2016/06/16 21:11:48, jbroman wrote:
> Elsewhere you pass cc::SurfaceId by value, but here it's by const&. Is this
> intentional?

I am trying to use const& as much as possible. However as the current Surface
APIs pass SurfaceId as value (e.g. the require/satisfy callbacks), I don't want
to change everything in one shot for this patch; will follow up with another
patch to make all places to pass SurfaceId by const& for consistency.

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
(right):

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:62:
FakeOffscreenCanvasSurfaceServiceImpl* m_surfaceService;
On 2016/06/16 21:11:48, jbroman wrote:
> This seems to be an owning pointer, too.
> 
> std::unique_ptr (or OwnPtr, though that's on its way out), or simpler yet this
> could probably just be a FakeOffscreenCanvasSurfaceServiceImpl), please?

OwnPtr is not suitable in this case. Due to the structure of the test,
FakeOffscreenCanvasSurfaceServiceImpl is kept at two different places--the Test
body itself and the MockCanvasSurfaceLayerBridgeClient. To avoid complicate the
code (after all, the class is not used in anywhere else), I didn't make it a
subclass of RefCounted so can't use RefPtr; instead, I make sure that I handle
the deletion of pointers carefully in both destructors.

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:94:
surfaceId = new cc::SurfaceId(10, 15, 0);
On 2016/06/16 21:11:48, jbroman wrote:
> This doesn't set the out parameter; it just rewrites the local variable. I
think
> you want something like:
> 
> *surfaceId = cc::SurfaceId(10, 15, 0);

Done.

[danakj, 2016-06-16 23:19:23]

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
(right):

https://codereview.chromium.org/2036663003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:62:
FakeOffscreenCanvasSurfaceServiceImpl* m_surfaceService;
On 2016/06/16 23:16:21, xlai (Olivia) wrote:
> On 2016/06/16 21:11:48, jbroman wrote:
> > This seems to be an owning pointer, too.
> > 
> > std::unique_ptr (or OwnPtr, though that's on its way out), or simpler yet
this
> > could probably just be a FakeOffscreenCanvasSurfaceServiceImpl), please?
> 
> OwnPtr is not suitable in this case. Due to the structure of the test,
> FakeOffscreenCanvasSurfaceServiceImpl is kept at two different places--the
Test
> body itself and the MockCanvasSurfaceLayerBridgeClient. To avoid complicate
the
> code (after all, the class is not used in anywhere else), I didn't make it a
> subclass of RefCounted so can't use RefPtr; instead, I make sure that I handle
> the deletion of pointers carefully in both destructors. 

Only one of the two owns it tho, right? So one would be a raw pointer and one an
OwnPtr?

[xlai (Olivia), 2016-06-17 00:01:32]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-06-17 00:01:33]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
piman@chromium.org, danakj@chromium.org, junov@chromium.org,
jbauman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2036663003/#ps300001
(title: "Minor Fix on OwnPtr on CanvasSurfaceLayerBridgeTest")

[commit-bot: I haz the power, 2016-06-17 00:02:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/300001

[commit-bot: I haz the power, 2016-06-17 00:14:02]

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

[commit-bot: I haz the power, 2016-06-17 00:14:04]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[danakj, 2016-06-17 00:33:28]

Thanks for the OwnPtr

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
(right):

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:62:
OwnPtr<FakeOffscreenCanvasSurfaceServiceImpl> m_surfaceService;
Since the m_surfaceLayerBridge has a pointer to this m_surfaceService, you
should destroy it after the m_surfaceLayerBridge (aka put this above it)

[Ben Goodger (Google), 2016-06-17 00:38:31]

ben@chromium.org changed reviewers:
+ ben@chromium.org

[Ben Goodger (Google), 2016-06-17 00:38:33]

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom
(right):

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface_service.mojom:10:
interface OffscreenCanvasSurfaceService {
Are you using the -Service suffix because this is logically a service, or just
because other Mojo interfaces do it?

I'd like to deprecate suffixing all interfaces with the term "Service" unless
there really is no better name. In this case, does "OffscreenCanvasSurface" make
sense?

[xlai (Olivia), 2016-06-17 16:04:07]

As suggested by ben@, removed the suffix "service" from mojo interface.

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
(right):

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:62:
OwnPtr<FakeOffscreenCanvasSurfaceServiceImpl> m_surfaceService;
On 2016/06/17 00:33:28, danakj wrote:
> Since the m_surfaceLayerBridge has a pointer to this m_surfaceService, you
> should destroy it after the m_surfaceLayerBridge (aka put this above it)

Actually m_surfaceLayerBridge itself doesn't contain a pointer to this
m_surfaceService.
Only its client keeps a pointer to it (rmb we had the discussion of separating
bridge
and bridgeclient to isolate all IPC-related stuffs out into bridgeclient?)
Also, I did set the pointer to nullptr in client's destructor. In addition,
since this
service is owned by test, I should let test destroy it and not other entities.

[xlai (Olivia), 2016-06-17 16:21:39]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-06-17 16:21:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
piman@chromium.org, danakj@chromium.org, junov@chromium.org,
jbauman@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/2036663003/#ps320001
(title: "Remove Service suffix on mojo interface name")

[commit-bot: I haz the power, 2016-06-17 16:21:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/320001

[danakj, 2016-06-17 17:53:04]

Long comment explanation sorry *^_^*

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
(right):

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:62:
OwnPtr<FakeOffscreenCanvasSurfaceServiceImpl> m_surfaceService;
On 2016/06/17 16:04:07, xlai (Olivia) wrote:
> On 2016/06/17 00:33:28, danakj wrote:
> > Since the m_surfaceLayerBridge has a pointer to this m_surfaceService, you
> > should destroy it after the m_surfaceLayerBridge (aka put this above it)
> 
> Actually m_surfaceLayerBridge itself doesn't contain a pointer to this
> m_surfaceService.
> Only its client keeps a pointer to it (rmb we had the discussion of separating
> bridge
> and bridgeclient to isolate all IPC-related stuffs out into bridgeclient?)

Sorry, what I mean is that on line 111-112 we have:

OwnPtr<CanvasSurfaceLayerBridgeClient> bridgeClient = adoptPtr(
    new MockCanvasSurfaceLayerBridgeClient(m_surfaceService.get()));
m_surfaceLayerBridge = adoptPtr(new CanvasSurfaceLayerBridge(
    std::move(bridgeClient)));

That puts an m_surfaceService* into MockCanvasSurfaceLayerBridgeClient, and
moves the mock client into the CanvasSurfaceLayerBridge, stored as
m_surfaceLayerBridge.

So that creates a dependency from m_surfaceLayerBridge on m_surfaceService. If
m_surfaceService is destroyed first, there is a dangling pointer somewhere in
the system briefly.

> Also, I did set the pointer to nullptr in client's destructor.

The MockCanvasSurfaceLayerBridgeClient only has a raw pointer, it can't use its
own members after its destructor runs, so setting the raw pointer to null there
doesn't help with this situation.

The m_surfaceService will destroy first (member variables are destroyed bottom
to top), which will run ~FakeOffscreenCanvasSurfaceServiceImpl() and then set
m_surfaceService to null. Now any pointers left in the system are dangling, such
as the one we handed to the MockCanvasSurfaceLayerBridgeClient.

The next thing to happen is the m_surfaceLayerBridge is destroyed, which will
run ~CanvasSurfaceLayerBridge(). Eventually this gets to destroying the
MockCanvasSurfaceLayerBridgeClient, but there's a dangerous pointer sitting
around until it does.

So I just meant that these two OwnPtrs should have their order inverted to be
destroyed in the other order. 

> In addition, since this
> service is owned by test, I should let test destroy it and not other entities.

I agree, it's just the ordering here. Right now nothing is using the pointer so
it doesn't crash, but if something did in the future, it wouldn't be null, so it
wouldn't necessarily be obvious things were going extremely wrong unless you
were running under ASAN or got unlucky.

[Ben Goodger (Google), 2016-06-17 17:58:03]

mojom interface name lgtm, see nits:

https://codereview.chromium.org/2036663003/diff/320001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom
(right):

https://codereview.chromium.org/2036663003/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:11:

nit: remove newline

https://codereview.chromium.org/2036663003/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:15:
[ Sync ]
nit: remove spaces around "Sync"

[commit-bot: I haz the power, 2016-06-17 19:36:25]

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

[commit-bot: I haz the power, 2016-06-17 19:36:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xlai (Olivia), 2016-06-17 20:47:02]

rebase master and edits based on danakj@ and ben@'s latest comments.

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp
(right):

https://codereview.chromium.org/2036663003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeTest.cpp:62:
OwnPtr<FakeOffscreenCanvasSurfaceServiceImpl> m_surfaceService;


Done. Thanks! I've reversed the order of OwnPtr in Test class.

https://codereview.chromium.org/2036663003/diff/320001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom
(right):

https://codereview.chromium.org/2036663003/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:11:

On 2016/06/17 17:58:02, Ben Goodger (Google) wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/2036663003/diff/320001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/offscreencanvas/offscreen_canvas_surface.mojom:15:
[ Sync ]
On 2016/06/17 17:58:03, Ben Goodger (Google) wrote:
> nit: remove spaces around "Sync"

Done.

[xlai (Olivia), 2016-06-18 15:47:02]

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

[commit-bot: I haz the power, 2016-06-18 15:47:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/340001

[commit-bot: I haz the power, 2016-06-18 17:21:50]

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

[commit-bot: I haz the power, 2016-06-18 17:21:52]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xlai (Olivia), 2016-06-20 19:56:18]

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

[commit-bot: I haz the power, 2016-06-20 19:57:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/360001

[commit-bot: I haz the power, 2016-06-20 20:08:14]

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

[commit-bot: I haz the power, 2016-06-20 20:08:17]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xlai (Olivia), 2016-06-20 22:05:47]

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

[commit-bot: I haz the power, 2016-06-20 22:06:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/380001

[commit-bot: I haz the power, 2016-06-20 23:36:43]

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

[commit-bot: I haz the power, 2016-06-20 23:36:46]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2016-06-20 23:37:25]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-06-20 23:37:25]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
piman@chromium.org, danakj@chromium.org, junov@chromium.org,
jbauman@chromium.org, fsamuel@chromium.org, ben@chromium.org
Link to the patchset: https://codereview.chromium.org/2036663003/#ps380001
(title: "OWNERS change on offscreencanvas folder")

[commit-bot: I haz the power, 2016-06-20 23:38:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036663003/380001

[commit-bot: I haz the power, 2016-06-20 23:49:25]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface. Some codes are added to an existing layout test to test if
the whole workflow works.

Client:
CanvasSurfaceLayerBridge is HTMLCanvasElement's communicator with SurfaceLayer.
It is created only when HTMLCanvasElement.transferControlToOffscreen() is
invoked.
It has an instance of CanvasSurfaceLayerBridgeClient, which is a virtual
interface
containing all mojo calls. To create a SurfaceLayer, it sends a sync msg
"GetSurfaceId" to browser to retrieve a unique SurfaceId. Immediately after
that,
it sends an async msg "RequestSurfaceCreation" to request Surface getting
created
for this particular Id. After that, it creates a SurfaceLayer (with
SatisfyCallback
and RequireCallback) for HTMLCanvasElement and set the SurfaceId for the layer
and
registers it to the graphics layer. A unit test is added; it fakes the browser
side
and mocks the BridgeClient.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. I use typemaps
to
convert cc::SurfaceId and cc::SurfaceSequence to the corresponding mojo
intermediate
structs.


BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface. Some codes are added to an existing layout test to test if
the whole workflow works.

Client:
CanvasSurfaceLayerBridge is HTMLCanvasElement's communicator with SurfaceLayer.
It is created only when HTMLCanvasElement.transferControlToOffscreen() is
invoked.
It has an instance of CanvasSurfaceLayerBridgeClient, which is a virtual
interface
containing all mojo calls. To create a SurfaceLayer, it sends a sync msg
"GetSurfaceId" to browser to retrieve a unique SurfaceId. Immediately after
that,
it sends an async msg "RequestSurfaceCreation" to request Surface getting
created
for this particular Id. After that, it creates a SurfaceLayer (with
SatisfyCallback
and RequireCallback) for HTMLCanvasElement and set the SurfaceId for the layer
and
registers it to the graphics layer. A unit test is added; it fakes the browser
side
and mocks the BridgeClient.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. I use typemaps
to
convert cc::SurfaceId and cc::SurfaceSequence to the corresponding mojo
intermediate
structs.


BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-20 23:49:28]

Committed patchset #18 (id:380001)

[commit-bot: I haz the power, 2016-06-20 23:51:10]

Description was changed from

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface. Some codes are added to an existing layout test to test if
the whole workflow works.

Client:
CanvasSurfaceLayerBridge is HTMLCanvasElement's communicator with SurfaceLayer.
It is created only when HTMLCanvasElement.transferControlToOffscreen() is
invoked.
It has an instance of CanvasSurfaceLayerBridgeClient, which is a virtual
interface
containing all mojo calls. To create a SurfaceLayer, it sends a sync msg
"GetSurfaceId" to browser to retrieve a unique SurfaceId. Immediately after
that,
it sends an async msg "RequestSurfaceCreation" to request Surface getting
created
for this particular Id. After that, it creates a SurfaceLayer (with
SatisfyCallback
and RequireCallback) for HTMLCanvasElement and set the SurfaceId for the layer
and
registers it to the graphics layer. A unit test is added; it fakes the browser
side
and mocks the BridgeClient.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. I use typemaps
to
convert cc::SurfaceId and cc::SurfaceSequence to the corresponding mojo
intermediate
structs.


BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Establish mojo service between Canvas (blink) and SurfaceManager (browser)

This patch adds a new mojo message pipe between Canvas in blink and
the Surface management on browser, to facilitate the out-of-process commit flow
from OffscreenCanvas to browser. This patch only implements the first two
IPC to ensure SurfaceLayer on renderer, SurfaceId/Surface on browser are
created.

The implementation can be seen in three parts: client, browser host, and mojo
service interface. Some codes are added to an existing layout test to test if
the whole workflow works.

Client:
CanvasSurfaceLayerBridge is HTMLCanvasElement's communicator with SurfaceLayer.
It is created only when HTMLCanvasElement.transferControlToOffscreen() is
invoked.
It has an instance of CanvasSurfaceLayerBridgeClient, which is a virtual
interface
containing all mojo calls. To create a SurfaceLayer, it sends a sync msg
"GetSurfaceId" to browser to retrieve a unique SurfaceId. Immediately after
that,
it sends an async msg "RequestSurfaceCreation" to request Surface getting
created
for this particular Id. After that, it creates a SurfaceLayer (with
SatisfyCallback
and RequireCallback) for HTMLCanvasElement and set the SurfaceId for the layer
and
registers it to the graphics layer. A unit test is added; it fakes the browser
side
and mocks the BridgeClient.

Browser:
OffscreenCanvasSurfaceServiceImpl sits on the other end of the mojo message
pipe.
It is created when all the mojo connections are registered in
render_process_host.
It responds to requests from CanvasSurfaceLayerBridge and do SurfaceId
generation,
SurfaceFactory and Surface creation accordingly. It is a client of
SurfaceFactory.

Mojo service interface:
It is defined in offscreen_canvas_surface_service.mojom. It defines two structs:
SurfaceId and SurfaceSequence, that correspondds to that in cc. I use typemaps
to
convert cc::SurfaceId and cc::SurfaceSequence to the corresponding mojo
intermediate
structs.

BUG=611796, 563852
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e93a0d29e98d8770319072162eb420fd81758f6d
Cr-Commit-Position: refs/heads/master@{#400838}
==========

[commit-bot: I haz the power, 2016-06-20 23:51:13]

Patchset 18 (id:??) landed as
https://crrev.com/e93a0d29e98d8770319072162eb420fd81758f6d
Cr-Commit-Position: refs/heads/master@{#400838}

[dcheng, 2016-09-15 20:45:11]

https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h
(right):

https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h:16:
class PLATFORM_EXPORT CanvasSurfaceLayerBridgeClient {
Apparently this class just exists as a layer of indirection for mojo vs tests?
Can we get rid of it? The test can just implement a mock version of the mojo
interface and use it directly. See
https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest...
for an example of how to do this all in a unit test.

[xlai (Olivia), 2016-09-15 22:53:10]

https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h
(right):

https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h:16:
class PLATFORM_EXPORT CanvasSurfaceLayerBridgeClient {
This class was added since Patch Set 7 and it was suggested by danakj@ (See her
comments
on Patch Set 6 for details). Basically, the major purpose of this class is to
make it cleaner between
code and test such that the unit test will not interfere too much with the
original code; it's a better
way to do testing. 
Another benefit is that the include of offscreen_canvas_surface.mojom-blink.h
(which occurs in/CanvasSurfaceLayerBridgeClientImpl.h) will not interfere with
HTMLCanvasElement.cpp that
include CanvasSurfaceLayerBridge.

On 2016/09/15 20:45:11, dcheng wrote:
> Apparently this class just exists as a layer of indirection for mojo vs tests?
> Can we get rid of it? The test can just implement a mock version of the mojo
> interface and use it directly. See
>
https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest...
> for an example of how to do this all in a unit test.

[xlai (Olivia), 2016-09-15 23:13:31]

On 2016/09/15 22:53:10, xlai (Olivia) wrote:
>
https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
> File
> third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h
> (right):
> 
>
https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h:16:
> class PLATFORM_EXPORT CanvasSurfaceLayerBridgeClient {
> This class was added since Patch Set 7 and it was suggested by danakj@ (See
her
> comments
> on Patch Set 6 for details). Basically, the major purpose of this class is to
> make it cleaner between
> code and test such that the unit test will not interfere too much with the
> original code; it's a better
> way to do testing. 
> Another benefit is that the include of offscreen_canvas_surface.mojom-blink.h
> (which occurs in/CanvasSurfaceLayerBridgeClientImpl.h) will not interfere with
> HTMLCanvasElement.cpp that
> include CanvasSurfaceLayerBridge.

Typo here. I meant HTMLCanvasElement.h.

> 
> On 2016/09/15 20:45:11, dcheng wrote:
> > Apparently this class just exists as a layer of indirection for mojo vs
tests?
> > Can we get rid of it? The test can just implement a mock version of the mojo
> > interface and use it directly. See
> >
>
https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest...
> > for an example of how to do this all in a unit test.

[danakj, 2016-09-15 23:48:47]

https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h
(right):

https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h:16:
class PLATFORM_EXPORT CanvasSurfaceLayerBridgeClient {
On 2016/09/15 22:53:10, xlai (Olivia) wrote:
> This class was added since Patch Set 7 and it was suggested by danakj@ (See
her
> comments
> on Patch Set 6 for details). Basically, the major purpose of this class is to
> make it cleaner between
> code and test such that the unit test will not interfere too much with the
> original code; it's a better
> way to do testing. 
> Another benefit is that the include of offscreen_canvas_surface.mojom-blink.h
> (which occurs in/CanvasSurfaceLayerBridgeClientImpl.h) will not interfere with
> HTMLCanvasElement.cpp that
> include CanvasSurfaceLayerBridge.

I think what Daniel is getting at is that we should just use a mojo interface
and make the implementation of that interface in the unit tests. That's what we
were going for but you ran into problems where you were unable to instantiate a
mojo service in the unit tests? So we went with an abstract class instead.
Daniel believes you should be able to do this with just a mojo interface and
have it work in tests tho, maybe his example would help explain how/.

> 
> On 2016/09/15 20:45:11, dcheng wrote:
> > Apparently this class just exists as a layer of indirection for mojo vs
tests?
> > Can we get rid of it? The test can just implement a mock version of the mojo
> > interface and use it directly. See
> >
>
https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest...
> > for an example of how to do this all in a unit test.
>

[dcheng, 2016-09-18 17:24:43]

https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h
(right):

https://codereview.chromium.org/2036663003/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/CanvasSurfaceLayerBridgeClient.h:16:
class PLATFORM_EXPORT CanvasSurfaceLayerBridgeClient {
On 2016/09/15 23:48:47, danakj wrote:
> On 2016/09/15 22:53:10, xlai (Olivia) wrote:
> > This class was added since Patch Set 7 and it was suggested by danakj@ (See
> her
> > comments
> > on Patch Set 6 for details). Basically, the major purpose of this class is
to
> > make it cleaner between
> > code and test such that the unit test will not interfere too much with the
> > original code; it's a better
> > way to do testing. 
> > Another benefit is that the include of
offscreen_canvas_surface.mojom-blink.h
> > (which occurs in/CanvasSurfaceLayerBridgeClientImpl.h) will not interfere
with
> > HTMLCanvasElement.cpp that
> > include CanvasSurfaceLayerBridge.
> 
> I think what Daniel is getting at is that we should just use a mojo interface
> and make the implementation of that interface in the unit tests. That's what
we
> were going for but you ran into problems where you were unable to instantiate
a
> mojo service in the unit tests? So we went with an abstract class instead.
> Daniel believes you should be able to do this with just a mojo interface and
> have it work in tests tho, maybe his example would help explain how/.
> 
> > 
> > On 2016/09/15 20:45:11, dcheng wrote:
> > > Apparently this class just exists as a layer of indirection for mojo vs
> tests?
> > > Can we get rid of it? The test can just implement a mock version of the
mojo
> > > interface and use it directly. See
> > >
> >
>
https://cs.chromium.org/chromium/src/url/mojo/url_gurl_struct_traits_unittest...
> > > for an example of how to do this all in a unit test.
> > 
> 

I have a CL for this (https://codereview.chromium.org/2345423002/) although
there's some weird issues with sync methods in unit tests. I'll figure out
what's going on and send it out for review.