Mandoline: Allow submitting CompositorFrames directly to mojo::Views

Mandoline: Allow submitting CompositorFrames directly to mojo::Views

Right now, prior to this patch, Views and Surfaces are
similar but separate concepts. A View has a Surface ID. A
View is a thing that you manipulate on the main thread of
an html_viewer, and a Surface is the thing you submit
CompositorFrames to on the compositor thread.

This CL allows submitting frames directly to Views via a
Surface interface that is implemented upon request.

A Subsequent patch will teach the View Manager how to read
CompositorFrame messages so that it updates the view
hierarchy on each frame.

BUG=492389
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=jam@chromium.org for mechanical pdf_viewer.cc change.

Committed: https://crrev.com/856ded0fc87f766efd59642701aa215b4fdce66f
Cr-Commit-Position: refs/heads/master@{#345952}

[Fady Samuel, 2015-08-07 00:15:15]

fsamuel@chromium.org changed reviewers:
+ rjkroege@chromium.org, sadrul@chromium.org

[rjkroege, 2015-08-13 18:04:01]

please line-break the comment.

https://codereview.chromium.org/1281663002/diff/120001/components/pdf_viewer/...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1281663002/diff/120001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:75:
view_->RequestCompositorFrameReceiver(GetProxy(&receiver_));
i don't like this name receiver_ for the interface to the UIP compositor. 
display_? Argue about naming later.

https://codereview.chromium.org/1281663002/diff/120001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:283: mojo::CompositorFrameReceiverPtr
receiver_;
compositing_service_ ?

https://codereview.chromium.org/1281663002/diff/120001/components/view_manage...
File components/view_manager/connection_manager.h (right):

https://codereview.chromium.org/1281663002/diff/120001/components/view_manage...
components/view_manager/connection_manager.h:202: // SurfaceFactoryClient
implementation.
is this right?

https://codereview.chromium.org/1281663002/diff/120001/components/view_manage...
File components/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1281663002/diff/120001/components/view_manage...
components/view_manager/public/interfaces/view_manager.mojom:42: interface
CompositorFrameReceiver {
why didn't you put this with the compositor_frame?

https://codereview.chromium.org/1281663002/diff/120001/mojo/cc/output_surface...
File mojo/cc/output_surface_mojo.cc (right):

https://codereview.chromium.org/1281663002/diff/120001/mojo/cc/output_surface...
mojo/cc/output_surface_mojo.cc:48:
receiver_->SubmitFrame(CompositorFrame::From(*frame),
what thread and process does this code run on?

[Fady Samuel, 2015-08-17 21:11:11]

PTAL Rob! I believe I've addressed your comments and I've renamed SubmitFrame to
SubmitCompositorFrame.

https://codereview.chromium.org/1281663002/diff/120001/components/pdf_viewer/...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1281663002/diff/120001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:75:
view_->RequestCompositorFrameReceiver(GetProxy(&receiver_));
On 2015/08/13 18:04:01, rjkroege wrote:
> i don't like this name receiver_ for the interface to the UIP compositor. 
> display_? Argue about naming later.

Renamed to cc_frame_receiver_ as per offline conversation.

https://codereview.chromium.org/1281663002/diff/120001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:283: mojo::CompositorFrameReceiverPtr
receiver_;
On 2015/08/13 18:04:01, rjkroege wrote:
> compositing_service_ ?

Renamed to cc_frame_receiver as per our conversation offline.

https://codereview.chromium.org/1281663002/diff/120001/components/view_manage...
File components/view_manager/connection_manager.h (right):

https://codereview.chromium.org/1281663002/diff/120001/components/view_manage...
components/view_manager/connection_manager.h:202: // SurfaceFactoryClient
implementation.
On 2015/08/13 18:04:01, rjkroege wrote:
> is this right?

In what sense? It is an interface off SurfaceFactoryClient. It doesn't yet have
a meaningful implementation.

https://codereview.chromium.org/1281663002/diff/120001/components/view_manage...
File components/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1281663002/diff/120001/components/view_manage...
components/view_manager/public/interfaces/view_manager.mojom:42: interface
CompositorFrameReceiver {
On 2015/08/13 18:04:01, rjkroege wrote:
> why didn't you put this with the compositor_frame?

Moved

https://codereview.chromium.org/1281663002/diff/120001/mojo/cc/output_surface...
File mojo/cc/output_surface_mojo.cc (right):

https://codereview.chromium.org/1281663002/diff/120001/mojo/cc/output_surface...
mojo/cc/output_surface_mojo.cc:48:
receiver_->SubmitFrame(CompositorFrame::From(*frame),
On 2015/08/13 18:04:01, rjkroege wrote:
> what thread and process does this code run on?

html_viewer on the compositor thread currently. However, this isn't a part of
the html_viewer component (it's common code) so I don't want to refer to
html_viewer in this file.

[rjkroege, 2015-08-17 22:34:28]

did you line-break the description?

[Fady Samuel, 2015-08-17 22:40:16]

On 2015/08/17 22:34:28, rjkroege wrote:
> did you line-break the description?

I have now! :-)

[rjkroege, 2015-08-17 23:06:19]

Please fix up skia includes?

https://codereview.chromium.org/1281663002/diff/180001/components/view_manage...
File components/view_manager/DEPS (right):

https://codereview.chromium.org/1281663002/diff/180001/components/view_manage...
components/view_manager/DEPS:11: "+third_party/skia/include",
you didn't address this.

[Fady Samuel, 2015-08-18 20:28:31]

PTAL Rob. As discussed offline, this will need some changes to the cc interface.
I'll leave as is for now.

[rjkroege, 2015-08-19 17:44:23]

On 2015/08/18 20:28:31, Fady Samuel wrote:
> PTAL Rob. As discussed offline, this will need some changes to the cc
interface.
> I'll leave as is for now.

So is this ready for review or not?

[Fady Samuel, 2015-08-19 17:45:36]

On 2015/08/19 17:44:23, rjkroege wrote:
> On 2015/08/18 20:28:31, Fady Samuel wrote:
> > PTAL Rob. As discussed offline, this will need some changes to the cc
> interface.
> > I'll leave as is for now.
> 
> So is this ready for review or not?

Yes, I'll do the cc changes in another patch. Would you rather I do the cc
changes first?

[Fady Samuel, 2015-08-19 21:21:51]

PTAL Rob! Thanks

[rjkroege, 2015-08-19 22:49:02]

On 2015/08/19 21:21:51, Fady Samuel wrote:
> PTAL Rob! Thanks

update CL description per discussion.

[Fady Samuel, 2015-08-19 22:51:53]

PTAL Rob! I've updated the description.

[rjkroege, 2015-08-19 23:45:44]

looks largely reasonable. please update commit message. Add some todos. etc.

lgtm

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
components/html_viewer/html_frame.cc:605:
view()->RequestCompositorFrameReceiver(GetProxy(&receiver));
This mojo GetProxy name overlaps badly with the cc proxy stuff. Oh well

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
File components/html_viewer/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
components/html_viewer/web_layer_tree_view_impl.cc:71:
receiver.PassInterface().PassHandle()));
impl gets it here obviously. but you have an ordering issue? i.e.: there are
startup orderings that may occur that will not work?

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:234:
cc_frame_receiver_->SubmitCompositorFrame(frame.Pass(), mojo::Closure());
replacing this mojo::Closure() here with a real callback would probably permit
improving perf.

https://codereview.chromium.org/1281663002/diff/260001/components/view_manage...
File components/view_manager/connection_manager.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/view_manage...
components/view_manager/connection_manager.cc:466: void
ConnectionManager::ReturnResources(
this is not how I plumbed it

https://codereview.chromium.org/1281663002/diff/260001/mandoline/ui/aura/surf...
File mandoline/ui/aura/surface_binding.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/mandoline/ui/aura/surf...
mandoline/ui/aura/surface_binding.cc:67: mojo::Closure());
are you correctly interlocking?

[Fady Samuel, 2015-08-20 16:22:45]

PTAL Rob! I found lots of opportunities for cleanup! Yay!

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
components/html_viewer/html_frame.cc:605:
view()->RequestCompositorFrameReceiver(GetProxy(&receiver));
On 2015/08/19 23:45:44, rjkroege wrote:
> This mojo GetProxy name overlaps badly with the cc proxy stuff. Oh well

Acknowledged.

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
File components/html_viewer/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
components/html_viewer/web_layer_tree_view_impl.cc:71:
receiver.PassInterface().PassHandle()));
On 2015/08/19 23:45:44, rjkroege wrote:
> impl gets it here obviously. but you have an ordering issue? i.e.: there are
> startup orderings that may occur that will not work?

As per offline discussion, I think there was some confusion here between
cc::Proxy and mojo Proxy.

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:234:
cc_frame_receiver_->SubmitCompositorFrame(frame.Pass(), mojo::Closure());
On 2015/08/19 23:45:44, rjkroege wrote:
> replacing this mojo::Closure() here with a real callback would probably permit
> improving perf.

Added TODO.

https://codereview.chromium.org/1281663002/diff/260001/components/view_manage...
File components/view_manager/connection_manager.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/view_manage...
components/view_manager/connection_manager.cc:466: void
ConnectionManager::ReturnResources(
On 2015/08/19 23:45:44, rjkroege wrote:
> this is not how I plumbed it

Feel free to change. This boilerplate code exists to remind us to implement
this.

https://codereview.chromium.org/1281663002/diff/260001/mandoline/ui/aura/surf...
File mandoline/ui/aura/surface_binding.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/mandoline/ui/aura/surf...
mandoline/ui/aura/surface_binding.cc:67: mojo::Closure());
On 2015/08/19 23:45:44, rjkroege wrote:
> are you correctly interlocking?

I'm not sure what you mean here. We submit frames at full speed. We're not
throttling and we probably should. Added a TODO.

[rjkroege, 2015-08-21 18:54:45]

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
components/html_viewer/html_frame.cc:605:
view()->RequestCompositorFrameReceiver(GetProxy(&receiver));
On 2015/08/20 16:22:44, Fady Samuel wrote:
> On 2015/08/19 23:45:44, rjkroege wrote:
> > This mojo GetProxy name overlaps badly with the cc proxy stuff. Oh well
> 
> Acknowledged.

perhaps you could do mojo::GetProxy for clarity?

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
File components/html_viewer/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
components/html_viewer/web_layer_tree_view_impl.cc:71:
receiver.PassInterface().PassHandle()));
On 2015/08/20 16:22:44, Fady Samuel wrote:
> On 2015/08/19 23:45:44, rjkroege wrote:
> > impl gets it here obviously. but you have an ordering issue? i.e.: there are
> > startup orderings that may occur that will not work?
> 
> As per offline discussion, I think there was some confusion here between
> cc::Proxy and mojo Proxy. 

yes. this code is running on main and CC will get wired up to the
OutputSurfaceMojo later when the scheduler kicks it off

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:179: float width_ratio =
static_cast<float>(width_) / bounds.width();
you have tested this in high DIP mode?

https://codereview.chromium.org/1281663002/diff/280001/components/view_manage...
File components/view_manager/public/interfaces/compositor_frame.mojom (right):

https://codereview.chromium.org/1281663002/diff/280001/components/view_manage...
components/view_manager/public/interfaces/compositor_frame.mojom:27: interface
ResourceReturner {
where did SetResourceReturner go?

[Fady Samuel, 2015-08-21 21:31:10]

PTAL

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
components/html_viewer/html_frame.cc:605:
view()->RequestCompositorFrameReceiver(GetProxy(&receiver));
On 2015/08/21 18:54:45, rjkroege wrote:
> On 2015/08/20 16:22:44, Fady Samuel wrote:
> > On 2015/08/19 23:45:44, rjkroege wrote:
> > > This mojo GetProxy name overlaps badly with the cc proxy stuff. Oh well
> > 
> > Acknowledged.
> 
> perhaps you could do mojo::GetProxy for clarity?

Done.

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
File components/html_viewer/web_layer_tree_view_impl.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/html_viewer...
components/html_viewer/web_layer_tree_view_impl.cc:71:
receiver.PassInterface().PassHandle()));
On 2015/08/21 18:54:45, rjkroege wrote:
> On 2015/08/20 16:22:44, Fady Samuel wrote:
> > On 2015/08/19 23:45:44, rjkroege wrote:
> > > impl gets it here obviously. but you have an ordering issue? i.e.: there
are
> > > startup orderings that may occur that will not work?
> > 
> > As per offline discussion, I think there was some confusion here between
> > cc::Proxy and mojo Proxy. 
> 
> yes. this code is running on main and CC will get wired up to the
> OutputSurfaceMojo later when the scheduler kicks it off

Acknowledged.

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:179: float width_ratio =
static_cast<float>(width_) / bounds.width();
On 2015/08/21 18:54:45, rjkroege wrote:
> you have tested this in high DIP mode?

This code worked in the past on android, and this patch makes no changes in
behavior. This is just avoiding storing the size in a mojo::Size since we don't
need to..

https://codereview.chromium.org/1281663002/diff/280001/components/view_manage...
File components/view_manager/public/interfaces/compositor_frame.mojom (right):

https://codereview.chromium.org/1281663002/diff/280001/components/view_manage...
components/view_manager/public/interfaces/compositor_frame.mojom:27: interface
ResourceReturner {
On 2015/08/21 18:54:45, rjkroege wrote:
> where did SetResourceReturner go?

It's back!

[rjkroege, 2015-08-21 22:58:00]

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:179: float width_ratio =
static_cast<float>(width_) / bounds.width();
On 2015/08/21 21:31:10, Fady Samuel wrote:
> On 2015/08/21 18:54:45, rjkroege wrote:
> > you have tested this in high DIP mode?
> 
> This code worked in the past on android, and this patch makes no changes in
> behavior. This is just avoiding storing the size in a mojo::Size since we
don't
> need to..


you are making a presumption that bounds and size are in the same units. Did you
confirm that they are. there use to be subtleties with this.

https://codereview.chromium.org/1281663002/diff/280001/components/view_manage...
File components/view_manager/public/interfaces/compositor_frame.mojom (right):

https://codereview.chromium.org/1281663002/diff/280001/components/view_manage...
components/view_manager/public/interfaces/compositor_frame.mojom:27: interface
ResourceReturner {
On 2015/08/21 21:31:10, Fady Samuel wrote:
> On 2015/08/21 18:54:45, rjkroege wrote:
> > where did SetResourceReturner go?
> 
> It's back!

good

https://codereview.chromium.org/1281663002/diff/300001/components/html_viewer...
File components/html_viewer/html_frame.cc (right):

https://codereview.chromium.org/1281663002/diff/300001/components/html_viewer...
components/html_viewer/html_frame.cc:565: void
HTMLFrame::OnWillNavigate(uint32_t target_frame_id,
in the future, it would be nicer if you could separate rebase from comment
update.

https://codereview.chromium.org/1281663002/diff/300001/components/view_manage...
File components/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1281663002/diff/300001/components/view_manage...
components/view_manager/public/interfaces/view_manager.mojom:200: // Sent when
GPU resources are returned to the client for reuse.
These messages need to end up at cc.  in Phase2, they will go to impl-paint. In
either case, they should not go to main. But afaik, this endpoint goes to main
yes?

We can proxy the message through main in the short term if necessary but that is
most certainly the wrong architecture because it defeats the point of cc::impl.

Based on the SubmitCompositorFrame discussion, I propose that we need separate
On* interfaces for main, cw, impl-paint. WDYT?

[Fady Samuel, 2015-08-21 23:15:27]

PTAL

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1281663002/diff/260001/components/pdf_viewer/...
components/pdf_viewer/pdf_viewer.cc:179: float width_ratio =
static_cast<float>(width_) / bounds.width();
On 2015/08/21 22:58:00, rjkroege wrote:
> On 2015/08/21 21:31:10, Fady Samuel wrote:
> > On 2015/08/21 18:54:45, rjkroege wrote:
> > > you have tested this in high DIP mode?
> > 
> > This code worked in the past on android, and this patch makes no changes in
> > behavior. This is just avoiding storing the size in a mojo::Size since we
> don't
> > need to..
> 
> 
> you are making a presumption that bounds and size are in the same units. Did
you
> confirm that they are. there use to be subtleties with this.
> 

There is no unit conversion above.

https://codereview.chromium.org/1281663002/diff/300001/components/view_manage...
File components/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1281663002/diff/300001/components/view_manage...
components/view_manager/public/interfaces/view_manager.mojom:200: // Sent when
GPU resources are returned to the client for reuse.
On 2015/08/21 22:58:00, rjkroege wrote:
> These messages need to end up at cc.  in Phase2, they will go to impl-paint.
In
> either case, they should not go to main. But afaik, this endpoint goes to main
> yes?
> 
> We can proxy the message through main in the short term if necessary but that
is
> most certainly the wrong architecture because it defeats the point of
cc::impl.
> 
> Based on the SubmitCompositorFrame discussion, I propose that we need separate
> On* interfaces for main, cw, impl-paint. WDYT?
> 

Yes, I agree. Do you want me to change this in a separate patch? I'd rather not
iterate yet again here :-)

[rjkroege, 2015-08-21 23:33:16]

lgtm. But please fix in next cl

[Fady Samuel, 2015-08-21 23:33:51]

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

[Fady Samuel, 2015-08-21 23:33:51]

+sky@ for OWNER review! Thanks!

[sky, 2015-08-25 17:11:12]

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
File components/view_manager/public/cpp/view.h (right):

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
components/view_manager/public/cpp/view.h:65:
InterfaceRequest<CompositorFrameReceiver> receiver);
I think we would be better served by not exposing the raw pipe here, but instead
wrapping the return value in an object that owns the binding. I think this will
let you better deal with returning resources to the client too.

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
File components/view_manager/public/interfaces/compositor_frame.mojom (right):

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
components/view_manager/public/interfaces/compositor_frame.mojom:21: uint32 id;
Document all these fields. For example, what does id mean?

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
File components/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
components/view_manager/public/interfaces/view_manager.mojom:201:
OnResourcesReturned(array<ReturnedResource> resources);
How come resources are returned here rather than through the
CompositorFrameReceiver interface? It seems like CompositorFrameReceiver should
have a client that calls OnResourcesReturned.

[Fady Samuel, 2015-08-25 23:39:31]

PTAL Scott. Can I address your non-trivial suggestions in a separate patch?
Thanks! I've just about got a Surface wrapper for the viewmanager client lib
working locally, but I'd like that separate from this. Thanks!

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
File components/view_manager/public/cpp/view.h (right):

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
components/view_manager/public/cpp/view.h:65:
InterfaceRequest<CompositorFrameReceiver> receiver);
On 2015/08/25 17:11:12, sky wrote:
> I think we would be better served by not exposing the raw pipe here, but
instead
> wrapping the return value in an object that owns the binding. I think this
will
> let you better deal with returning resources to the client too.

I absolutely agree. I'm working on it now but I'd prefer doing it in a separate
patch so this one doesn't grow too much.

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
File components/view_manager/public/interfaces/compositor_frame.mojom (right):

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
components/view_manager/public/interfaces/compositor_frame.mojom:21: uint32 id;
On 2015/08/25 17:11:12, sky wrote:
> Document all these fields. For example, what does id mean?

Done in a separate patch which you reviewed.

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
File components/view_manager/public/interfaces/view_manager.mojom (right):

https://codereview.chromium.org/1281663002/diff/320001/components/view_manage...
components/view_manager/public/interfaces/view_manager.mojom:201:
OnResourcesReturned(array<ReturnedResource> resources);
On 2015/08/25 17:11:12, sky wrote:
> How come resources are returned here rather than through the
> CompositorFrameReceiver interface? It seems like CompositorFrameReceiver
should
> have a client that calls OnResourcesReturned.

This is wrong but I'd like to fix it in the next patch since this patch is
getting big as it is.

[sky, 2015-08-26 15:15:19]

LGTM

[Fady Samuel, 2015-08-26 21:51:28]

PTAL Scott! I added a uip::Surface which is a wrapper around the
surface/surfaceclient mojo interfaces as suggested.

[sky, 2015-08-26 22:58:04]

https://codereview.chromium.org/1281663002/diff/360001/components/html_viewer...
File components/html_viewer/html_widget.cc (right):

https://codereview.chromium.org/1281663002/diff/360001/components/html_viewer...
components/html_viewer/html_widget.cc:203: DCHECK(view);
Isn't this and 85-94 about the same? It seems like the two could be shared.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
File components/view_manager/public/cpp/surface.h (right):

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface.h:14: class CompositorFrame;
This shouldn't be needed.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface.h:21: namespace uip {
Why is this in the namespace uip? It's in the view manager directory. While I
think mojo is wrong for the view_manager source, you should be consistent.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface.h:24: class SurfaceObserver;
You don't seem to use this.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface.h:26: class Surface : public
mojo::SurfaceClient {
Add description.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
File components/view_manager/public/cpp/surface_client.h (right):

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface_client.h:8: namespace uip {
uip->mojo.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface_client.h:17: protected:
nit: newline between 16 and 17.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
File components/view_manager/server_view.cc (right):

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/server_view.cc:22: }
nit:  // namespace

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/server_view.cc:241:
//delegate_->SubmitCompositorFrame(this, frame.Pass(), callback);
?

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/server_view.cc:260: gfx::Size frame_size =
Don't you hvae this defined on 240 already?

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/server_view.cc:274: last_submitted_frame_size_ =
frame_size;
This is only needed @ 266 and 256ish, right?

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
File components/view_manager/view_tree_impl.cc (right):

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/view_tree_impl.cc:700: ServerView* view =
GetView(ViewIdFromTransportId(view_id));
You'll want to reset the surface/client if this is an embed root and the call is
from the embedded connection. Feel free to add a todo and come back to this as
we weren't doing it for the id previously.

[Fady Samuel, 2015-08-26 23:50:20]

Patchset #21 (id:400001) has been deleted

[Fady Samuel, 2015-08-26 23:51:01]

PTAL Scott!

https://codereview.chromium.org/1281663002/diff/360001/components/html_viewer...
File components/html_viewer/html_widget.cc (right):

https://codereview.chromium.org/1281663002/diff/360001/components/html_viewer...
components/html_viewer/html_widget.cc:203: DCHECK(view);
On 2015/08/26 22:58:03, sky wrote:
> Isn't this and 85-94 about the same? It seems like the two could be shared.

Done.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
File components/view_manager/public/cpp/surface.h (right):

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface.h:14: class CompositorFrame;
On 2015/08/26 22:58:03, sky wrote:
> This shouldn't be needed.

Removed.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface.h:21: namespace uip {
On 2015/08/26 22:58:03, sky wrote:
> Why is this in the namespace uip? It's in the view manager directory. While I
> think mojo is wrong for the view_manager source, you should be consistent.

Changed to mojo for now.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface.h:24: class SurfaceObserver;
On 2015/08/26 22:58:03, sky wrote:
> You don't seem to use this.

Removed.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface.h:26: class Surface : public
mojo::SurfaceClient {
On 2015/08/26 22:58:03, sky wrote:
> Add description.

Done.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
File components/view_manager/public/cpp/surface_client.h (right):

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface_client.h:8: namespace uip {
On 2015/08/26 22:58:03, sky wrote:
> uip->mojo.

Done.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/public/cpp/surface_client.h:17: protected:
On 2015/08/26 22:58:03, sky wrote:
> nit: newline between 16 and 17.

Done.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
File components/view_manager/server_view.cc (right):

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/server_view.cc:22: }
On 2015/08/26 22:58:03, sky wrote:
> nit:  // namespace

Done.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/server_view.cc:241:
//delegate_->SubmitCompositorFrame(this, frame.Pass(), callback);
On 2015/08/26 22:58:03, sky wrote:
> ?

Moving some code around and missed this. Deleted.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/server_view.cc:260: gfx::Size frame_size =
On 2015/08/26 22:58:03, sky wrote:
> Don't you hvae this defined on 240 already?

Removed

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/server_view.cc:274: last_submitted_frame_size_ =
frame_size;
On 2015/08/26 22:58:03, sky wrote:
> This is only needed @ 266 and 256ish, right?

Yes. We keep track of the last frame's size, so we know whether or not to create
a new surface.

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
File components/view_manager/view_tree_impl.cc (right):

https://codereview.chromium.org/1281663002/diff/360001/components/view_manage...
components/view_manager/view_tree_impl.cc:700: ServerView* view =
GetView(ViewIdFromTransportId(view_id));
On 2015/08/26 22:58:03, sky wrote:
> You'll want to reset the surface/client if this is an embed root and the call
is
> from the embedded connection. Feel free to add a todo and come back to this as
> we weren't doing it for the id previously.

Done.

[sky, 2015-08-27 00:06:08]

LGTM

[Fady Samuel, 2015-08-27 00:06:41]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-08-27 00:06:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org
Link to the patchset: https://codereview.chromium.org/1281663002/#ps420001
(title: "Addressed Scott's comments")

[commit-bot: I haz the power, 2015-08-27 00:07:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/420001

[commit-bot: I haz the power, 2015-08-27 00:10:03]

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

[commit-bot: I haz the power, 2015-08-27 00:10:04]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Fady Samuel, 2015-08-27 00:28:40]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-08-27 00:28:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1281663002/#ps460001
(title: "Fixed rebase")

[commit-bot: I haz the power, 2015-08-27 00:29:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/460001

[commit-bot: I haz the power, 2015-08-27 01:21:59]

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

[commit-bot: I haz the power, 2015-08-27 01:22:00]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2015-08-27 15:59:14]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-08-27 15:59:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1281663002/#ps480001
(title: "Fixed broken tests (I hope)")

[commit-bot: I haz the power, 2015-08-27 15:59:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/480001

[Fady Samuel, 2015-08-27 16:29:21]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-08-27 16:29:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1281663002/#ps300002
(title: "Fixed DEPS + BUILD.gn")

[commit-bot: I haz the power, 2015-08-27 16:29:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/300002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/300002

[Fady Samuel, 2015-08-27 16:50:03]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-08-27 16:50:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1281663002/#ps510001
(title: "Fixed a crash in tests")

[commit-bot: I haz the power, 2015-08-27 16:50:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/510001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/510001

[commit-bot: I haz the power, 2015-08-27 17:41:46]

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

[commit-bot: I haz the power, 2015-08-27 17:41:48]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fady Samuel, 2015-08-27 17:46:11]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-08-27 17:46:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1281663002/#ps530001
(title: "Fixed DEPS")

[commit-bot: I haz the power, 2015-08-27 17:47:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/530001

[commit-bot: I haz the power, 2015-08-27 17:55:19]

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

[commit-bot: I haz the power, 2015-08-27 17:55:20]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Fady Samuel, 2015-08-27 17:56:13]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2015-08-27 17:56:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/530001

[commit-bot: I haz the power, 2015-08-27 18:06:25]

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

[commit-bot: I haz the power, 2015-08-27 18:06:26]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Fady Samuel, 2015-08-27 18:15:51]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2015-08-27 18:16:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/530001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/530001

[commit-bot: I haz the power, 2015-08-27 18:51:28]

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

[commit-bot: I haz the power, 2015-08-27 18:51:30]

Failed to apply patch for components/view_manager/connection_manager.h:
While running git apply --index -3 -p1;
  error: patch failed: components/view_manager/connection_manager.h:19
  Falling back to three-way merge...
  Applied patch to 'components/view_manager/connection_manager.h' with
conflicts.
  U components/view_manager/connection_manager.h

Patch:       components/view_manager/connection_manager.h
Index: components/view_manager/connection_manager.h
diff --git a/components/view_manager/connection_manager.h
b/components/view_manager/connection_manager.h
index
0940398b83105c55a2420acd8beb162fd4e4d475..a48a8fcc0a9a82dc7dc258a10dc268e87872abc7
100644
--- a/components/view_manager/connection_manager.h
+++ b/components/view_manager/connection_manager.h
@@ -19,6 +19,7 @@
 #include "components/view_manager/public/interfaces/view_tree.mojom.h"
 #include "components/view_manager/server_view_delegate.h"
 #include "components/view_manager/server_view_observer.h"
+#include "components/view_manager/surfaces/surfaces_state.h"
 #include "components/view_manager/view_manager_root_impl.h"
 #include "third_party/mojo/src/mojo/public/cpp/bindings/array.h"
 #include "third_party/mojo/src/mojo/public/cpp/bindings/binding.h"
@@ -71,7 +72,9 @@ class ConnectionManager : public ServerViewDelegate,
     DISALLOW_COPY_AND_ASSIGN(ScopedChange);
   };
 
-  explicit ConnectionManager(ConnectionManagerDelegate* delegate);
+  ConnectionManager(
+      ConnectionManagerDelegate* delegate,
+      const scoped_refptr<surfaces::SurfacesState>& surfaces_state);
   ~ConnectionManager() override;
 
   // Adds a ViewManagerRoot.
@@ -223,6 +226,7 @@ class ConnectionManager : public ServerViewDelegate,
   ViewManagerRootImpl* GetViewManagerRootByView(const ServerView* view) const;
 
   // Overridden from ServerViewDelegate:
+  surfaces::SurfacesState* GetSurfacesState() override;
   void PrepareToDestroyView(ServerView* view) override;
   void PrepareToChangeViewHierarchy(ServerView* view,
                                     ServerView* new_parent,
@@ -261,6 +265,9 @@ class ConnectionManager : public ServerViewDelegate,
 
   ConnectionManagerDelegate* delegate_;
 
+  // State for rendering into a Surface.
+  scoped_refptr<surfaces::SurfacesState> surfaces_state_;
+
   // ID to use for next ViewTreeImpl.
   mojo::ConnectionSpecificId next_connection_id_;

[commit-bot: I haz the power, 2015-08-27 18:52:32]

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

[Fady Samuel, 2015-08-27 18:59:40]

The CQ bit was checked by fsamuel@chromium.org

[Fady Samuel, 2015-08-27 18:59:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
rjkroege@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1281663002/#ps550001
(title: "Rebased")

[commit-bot: I haz the power, 2015-08-27 19:00:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281663002/550001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281663002/550001

[commit-bot: I haz the power, 2015-08-27 19:41:45]

Committed patchset #28 (id:550001)

[commit-bot: I haz the power, 2015-08-27 19:42:39]

Patchset 28 (id:??) landed as
https://crrev.com/856ded0fc87f766efd59642701aa215b4fdce66f
Cr-Commit-Position: refs/heads/master@{#345952}