Allow out-of-process iframes to render to compositing surfaces.

Allow out-of-process iframes to render to compositing surfaces.

When compositing surfaces are enabled, the browser process upon
receiving a frame from an OOPIF will pass a surface identifier to the
embedding renderer process rather than the frame itself. This prevents
frames from having to be composited at an arbitrary number of levels
when OOPIFs are chained.

BUG=478802
TEST=RenderWidgetHostViewChildFrameTest.SwapCompositorFrame

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

[kenrb, 2015-04-30 20:22:20]

kenrb@chromium.org changed reviewers:
+ jbauman@chromium.org

[kenrb, 2015-04-30 20:30:01]

kenrb@chromium.org changed required reviewers:
+ jbauman@chromium.org

[kenrb, 2015-04-30 20:30:02]

John: I'm not sure I have worked out all the compile issues in the test yet, but
would you mind having a pass at reviewing this?

[jbauman, 2015-05-01 23:58:11]

https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:268:
->AddDestructionDependency(sequence);
I guess we'll also need to figure out what happens with surface sequences when
one of these two renderers is destroyed.

https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:381: const
cc::ReturnedResourceArray& resources) {
This needs to store the resources if there's an ack outstanding (and the ack
needs to send them as well) or otherwise send a reclaim message with them.

https://codereview.chromium.org/1094113003/diff/60001/content/renderer/child_...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/1094113003/diff/60001/content/renderer/child_...
content/renderer/child_frame_compositing_helper.cc:303:
cc::SurfaceLayer::SatisfyCallback satisfy_callback = base::Bind(
I think we'll need to plumb down this renderer's surface id namespace (from its
DelegatedFrameHost in the browser) into
LayerTreeHost::set_surface_id_namespace(). Otherwise every renderer will think
it has the same id namespace.

[jamesr, 2015-05-02 00:05:13]

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

[jamesr, 2015-05-02 00:05:13]

If we make sure everything render prices is using its proper id namespace then
when the browser detects that a render process has crashed it can tell surfaces
that the associated id namespace is gone so it can ignore all sequence points
from that namespace.

[kenrb, 2015-05-05 16:24:38]

https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:268:
->AddDestructionDependency(sequence);
On 2015/05/01 23:58:10, jbauman wrote:
> I guess we'll also need to figure out what happens with surface sequences when
> one of these two renderers is destroyed.

I don't entirely understand James' response, but it sounds like we want to do
something from RWHV*::RenderProcessGone. That should probably be a separate CL.

https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:381: const
cc::ReturnedResourceArray& resources) {
On 2015/05/01 23:58:10, jbauman wrote:
> This needs to store the resources if there's an ack outstanding (and the ack
> needs to send them as well) or otherwise send a reclaim message with them.

Done.

https://codereview.chromium.org/1094113003/diff/60001/content/renderer/child_...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/1094113003/diff/60001/content/renderer/child_...
content/renderer/child_frame_compositing_helper.cc:303:
cc::SurfaceLayer::SatisfyCallback satisfy_callback = base::Bind(
On 2015/05/01 23:58:10, jbauman wrote:
> I think we'll need to plumb down this renderer's surface id namespace (from
its
> DelegatedFrameHost in the browser) into
> LayerTreeHost::set_surface_id_namespace(). Otherwise every renderer will think
> it has the same id namespace.

I see what you mean. Would this be another parameter for RenderWidget creation?

Should I add that to this CL or would you prefer I split that off?

[jbauman, 2015-05-06 00:48:37]

On 2015/05/05 16:24:38, kenrb wrote:
>
https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
> File content/browser/frame_host/render_widget_host_view_child_frame.cc
(right):
> 
>
https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
> content/browser/frame_host/render_widget_host_view_child_frame.cc:268:
> ->AddDestructionDependency(sequence);
> On 2015/05/01 23:58:10, jbauman wrote:
> > I guess we'll also need to figure out what happens with surface sequences
when
> > one of these two renderers is destroyed.
> 
> I don't entirely understand James' response, but it sounds like we want to do
> something from RWHV*::RenderProcessGone. That should probably be a separate
CL.

We'll probably need to think how to do that first before checking this in; this
code is currently used by webview, right?
> 
>
https://codereview.chromium.org/1094113003/diff/60001/content/browser/frame_h...
> content/browser/frame_host/render_widget_host_view_child_frame.cc:381: const
> cc::ReturnedResourceArray& resources) {
> On 2015/05/01 23:58:10, jbauman wrote:
> > This needs to store the resources if there's an ack outstanding (and the ack
> > needs to send them as well) or otherwise send a reclaim message with them.
> 
> Done.
> 
>
https://codereview.chromium.org/1094113003/diff/60001/content/renderer/child_...
> File content/renderer/child_frame_compositing_helper.cc (right):
> 
>
https://codereview.chromium.org/1094113003/diff/60001/content/renderer/child_...
> content/renderer/child_frame_compositing_helper.cc:303:
> cc::SurfaceLayer::SatisfyCallback satisfy_callback = base::Bind(
> On 2015/05/01 23:58:10, jbauman wrote:
> > I think we'll need to plumb down this renderer's surface id namespace (from
> its
> > DelegatedFrameHost in the browser) into
> > LayerTreeHost::set_surface_id_namespace(). Otherwise every renderer will
think
> > it has the same id namespace.
> 
> I see what you mean. Would this be another parameter for RenderWidget
creation?

I think that would make sense.
> 
> Should I add that to this CL or would you prefer I split that off?

Probably best to add that to this CL to avoid breaking anything.

[kenrb, 2015-05-06 14:48:56]

On 2015/05/06 00:48:37, jbauman wrote:
> On 2015/05/05 16:24:38, kenrb wrote:
> We'll probably need to think how to do that first before checking this in;
this
> code is currently used by webview, right?

I was talking to webview people about this yesterday. After this lands, they
will need to replicate my RWHVChildFrame changes to RWHVGuest, and plumb the
messages through BrowserPluginGuest instead of CrossProcessFrameConnector. This
CL won't affect webview until that is done.

Do you have any sense of how large a change it would be to have the right thing
happen on a renderer crash?

> > 
> > I see what you mean. Would this be another parameter for RenderWidget
> creation?
> 
> I think that would make sense.
> > 
> > Should I add that to this CL or would you prefer I split that off?
> 
> Probably best to add that to this CL to avoid breaking anything.

I will see if I can do this today.

[kenrb, 2015-05-11 21:17:52]

On 2015/05/02 00:05:13, jamesr wrote:
> If we make sure everything render prices is using its proper id namespace then
> when the browser detects that a render process has crashed it can tell
surfaces
> that the associated id namespace is gone so it can ignore all sequence points
> from that namespace.

I'm looking at how this might be done. Would this approach imply that the
SurfaceManager has to keep an ever-expanding vector of id namespaces for crashed
renderers? How could we know when a namespace is no longer associated with any
destruction dependencies for remaining surfaces?

[jamesr, 2015-05-11 21:22:31]

On 2015/05/11 21:17:52, kenrb wrote:
> On 2015/05/02 00:05:13, jamesr wrote:
> > If we make sure everything render prices is using its proper id namespace
then
> > when the browser detects that a render process has crashed it can tell
> surfaces
> > that the associated id namespace is gone so it can ignore all sequence
points
> > from that namespace.
> 
> I'm looking at how this might be done. Would this approach imply that the
> SurfaceManager has to keep an ever-expanding vector of id namespaces for
crashed
> renderers? 

No

> How could we know when a namespace is no longer associated with any
> destruction dependencies for remaining surfaces?

When being notified of a namespace going away it could immediately satisfy all
dependencies for that ID - i.e. iterate over surfaces_to_destroy_ and tell each
surface to satisfy any sequence point matching that ID namespace.

[kenrb, 2015-05-11 21:28:54]

On 2015/05/11 21:22:31, jamesr wrote:
> 
> When being notified of a namespace going away it could immediately satisfy all
> dependencies for that ID - i.e. iterate over surfaces_to_destroy_ and tell
each
> surface to satisfy any sequence point matching that ID namespace.

Is there a reason that we might not have sequence points with that ID namespace
as destruction dependencies for Surfaces that have not yet had Destroy() called
(i.e. not yet added to surfaces_to_destroy_)? Wouldn't these get orphaned?

[jamesr, 2015-05-11 21:40:29]

Let's say you have renderers [A] and [B].  [B] wants to have a dependency on
frames from [A], but [A] crashes at some point.  The browser will learn of [A]'s
destruction and I think we can assume that at some point or another [B] must
learn about this destruction as well since it must have been collaborating and
communicating with [A] in order to require the dependency.  Thus at some point
[B] will learn of [A] going away as well, although we don't know whether [B] or
the browser will learn first.  I think in the case where the browser learns
about [A] going away before [B] responds it's up to [B] to update its
dependencies to not require [A] once it learns about [A] going away - i.e.
trigger something like SurfaceLayer::SatisfyDestroySequence().

[jbauman, 2015-05-11 21:42:47]

On 2015/05/11 21:17:52, kenrb wrote:
> On 2015/05/02 00:05:13, jamesr wrote:
> > If we make sure everything render prices is using its proper id namespace
then
> > when the browser detects that a render process has crashed it can tell
> surfaces
> > that the associated id namespace is gone so it can ignore all sequence
points
> > from that namespace.
> 
> I'm looking at how this might be done. Would this approach imply that the
> SurfaceManager has to keep an ever-expanding vector of id namespaces for
crashed
> renderers? How could we know when a namespace is no longer associated with any
> destruction dependencies for remaining surfaces?

I think instead we could keep a set of allowed id namespaces - a namespace would
be added upon creation and destroyed when its SurfaceFactory is. Then any
SurfaceSequence that's not in a listed namespace is assumed satisfied.

[jamesr, 2015-05-12 00:03:38]

On 2015/05/11 21:42:47, jbauman wrote:
> On 2015/05/11 21:17:52, kenrb wrote:
> > On 2015/05/02 00:05:13, jamesr wrote:
> > > If we make sure everything render prices is using its proper id namespace
> then
> > > when the browser detects that a render process has crashed it can tell
> > surfaces
> > > that the associated id namespace is gone so it can ignore all sequence
> points
> > > from that namespace.
> > 
> > I'm looking at how this might be done. Would this approach imply that the
> > SurfaceManager has to keep an ever-expanding vector of id namespaces for
> crashed
> > renderers? How could we know when a namespace is no longer associated with
any
> > destruction dependencies for remaining surfaces?
> 
> I think instead we could keep a set of allowed id namespaces - a namespace
would
> be added upon creation and destroyed when its SurfaceFactory is. Then any
> SurfaceSequence that's not in a listed namespace is assumed satisfied.

That'd also work well.

[kenrb, 2015-05-22 20:21:08]

John: PTAL?

The two issues discussed above have been fixed in other CLs. I have updated this
CL to let RWHVChildFrame pass its surface ID namespace to the OOPIF renderer
process.

[jbauman, 2015-05-23 01:14:34]

Seems like some of the unit tests are red for some reason.

https://codereview.chromium.org/1094113003/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/220001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:257:
surface_factory_.reset();
I think you should only reset the surface_factory_ if output_surface_id !=
last_output_surface_id (and you should probably do it higher in the function so
it'll be recreated).

https://codereview.chromium.org/1094113003/diff/220001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/1094113003/diff/220001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:140:
background_layer_->SetBackgroundColor(SkColorSetARGBInline(255, 0, 128, 0));
Is this change necessary?

[kenrb, 2015-05-28 19:21:46]

John: Another look?

https://codereview.chromium.org/1094113003/diff/220001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/220001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:257:
surface_factory_.reset();
On 2015/05/23 01:14:34, jbauman wrote:
> I think you should only reset the surface_factory_ if output_surface_id !=
> last_output_surface_id (and you should probably do it higher in the function
so
> it'll be recreated).

Done.

https://codereview.chromium.org/1094113003/diff/220001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/1094113003/diff/220001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:140:
background_layer_->SetBackgroundColor(SkColorSetARGBInline(255, 0, 128, 0));
On 2015/05/23 01:14:34, jbauman wrote:
> Is this change necessary?

No, sorry about that. A debugging change I forgot to revert.

Fixed.

[jbauman, 2015-05-28 19:32:49]

https://codereview.chromium.org/1094113003/diff/320001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:262: if
(surface_id_.is_null()) {
You need to create a new surface_id_ if the frame_size or scale_factor changes,
but the surface factory can remain the same.

[kenrb, 2015-05-28 19:59:39]

https://codereview.chromium.org/1094113003/diff/320001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/320001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:262: if
(surface_id_.is_null()) {
On 2015/05/28 19:32:49, jbauman wrote:
> You need to create a new surface_id_ if the frame_size or scale_factor
changes,
> but the surface factory can remain the same.

Sorry, I misunderstood your earlier comment. Fixed.

[jbauman, 2015-05-28 20:09:00]

https://codereview.chromium.org/1094113003/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/340001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:255:
surface_id_ = cc::SurfaceId();
if (surface_factory_ && !surface_id_.is_null())
  surface_factory_->Destroy(surface_id_)

in this case as well.

[kenrb, 2015-05-28 20:17:53]

https://codereview.chromium.org/1094113003/diff/340001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/340001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:255:
surface_id_ = cc::SurfaceId();
On 2015/05/28 20:08:59, jbauman wrote:
> if (surface_factory_ && !surface_id_.is_null())
>   surface_factory_->Destroy(surface_id_)
> 
> in this case as well.

Done.

[jbauman, 2015-05-28 20:20:17]

lgtm

[kenrb, 2015-05-28 20:29:44]

kenrb@chromium.org changed reviewers:
+ nasko@chromium.org
- jamesr@chromium.org

[kenrb, 2015-05-28 20:29:44]

kenrb@chromium.org changed required reviewers:
+ nasko@chromium.org

[kenrb, 2015-05-28 20:29:45]

Nasko: Can you please review as a content owner?

[nasko, 2015-05-28 20:42:35]

nasko@chromium.org changed reviewers:
+ nick@chromium.org

[nasko, 2015-05-28 20:42:36]

Mostly nits, though I think Nick can do much better review on this CL. Adding
him to do owners review.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:214: if
(host_)
nit: Two line body requires {}.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:276: // The
renderer will satisfy this dependency when it creates a
nit: s/renderer/renderer process/?

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:165: // Flag
determining whether we render into a compositing surface.
nit: Is it Surface? You used capitalized version right below, so just checking
for consistency.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:176: uint32
ack_pending_count_;
nit: All other variables have some sort of "surface" substring in them. Should
this one have too?

https://codereview.chromium.org/1094113003/diff/360001/content/common/cc_mess...
File content/common/cc_messages.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/common/cc_mess...
content/common/cc_messages.h:152: IPC_STRUCT_TRAITS_MEMBER(id_namespace)
nit: Those and the above should be indented to match other struct declarations.

https://codereview.chromium.org/1094113003/diff/360001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/common/frame_m...
content/common/frame_messages.h:849: // TODO
Missing comments?

[dcheng, 2015-05-28 22:16:30]

Some random drive-by comments.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.h:90: virtual void
SetChildFrameSurface(cc::SurfaceId& surface_id,
No mutable references in arguments in Chrome.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.h:91: gfx::Size
frame_size,
This should probably be const gfx::Size&.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:276:
scoped_ptr<ImageTransportFactory>(
make_scoped_ptr will save you some typing.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
(right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:129:
gfx::Rect damage) {
gfx::Rect should be passed a const ref.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:138:
return frame.Pass();
This Pass() should be unnecessary due to NRVO.

https://codereview.chromium.org/1094113003/diff/360001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.h:110: cc::SurfaceSequence
sequence);
Is there a convention for whether this is passed by value or reference?

[dcheng, 2015-05-28 22:24:53]

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

[dcheng, 2015-05-28 22:24:55]

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:42: public
base::SupportsWeakPtr<RenderWidgetHostViewChildFrame> {
Prefer to embed a WeakPtrFactory rather than inheriting from SupportsWeakPtr<>.
Doing the latter allows other people to grab a weak pointer to your class, which
we don't need (or want) in this case.

[dcheng, 2015-05-28 22:25:07]

dcheng@chromium.org changed reviewers:
- dcheng@chromium.org

[ncarter (slow), 2015-05-28 22:32:52]

Amazing CL.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:40:
RenderWidgetHostViewChildFrame::~RenderWidgetHostViewChildFrame() {
If there are still entries in surface_returned_resources_ at this point (because
the RWHVCF is going away while we still have an ack pending) is it possible that
we'd be leaking something in the child process?

Or does surface_factory_->Destroy take care of that?

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:250:
surface_factory_.reset();
Why do we need a new SurfaceFactory each time the output surface id changes?

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:288:
ack_pending_count_++;
Could we dcheck some upper limit for ack_pending_count_ (the idea being that
getting more than a few frames ahead is madness anyhow, and such a check might
detect a bug where our increments and decrements don't balance out?)

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
(right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:149:
//
//

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:393: // Compositing
surfaces not supported.
Is this a job for NOTIMPLEMENTED()? Or will we not eventually have to make this
work?

https://codereview.chromium.org/1094113003/diff/360001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:68:
resource_collection_->SetClient(nullptr);
We've given pointers to this object to the surface_layer_ (via the
satisfy/require callbacks). And the surface_layer_ is refcounted so it
presumably could outlive the CFCH.

What guarantees that the callbacks won't be invoked after the CFCH is deleted?

[jbauman, 2015-05-28 22:43:38]

On 2015/05/28 22:32:52, ncarter wrote:
> Amazing CL.
> 
>
https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
> File content/browser/frame_host/render_widget_host_view_child_frame.cc
(right):
> 
>
https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
> content/browser/frame_host/render_widget_host_view_child_frame.cc:40:
> RenderWidgetHostViewChildFrame::~RenderWidgetHostViewChildFrame() {
> If there are still entries in surface_returned_resources_ at this point
(because
> the RWHVCF is going away while we still have an ack pending) is it possible
that
> we'd be leaking something in the child process?

The renderer compositor will release them when it goes away. That works
essentially the same as with delegated rendering currently.
> 
> Or does surface_factory_->Destroy take care of that?
> 
>
https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
> content/browser/frame_host/render_widget_host_view_child_frame.cc:250:
> surface_factory_.reset();
> Why do we need a new SurfaceFactory each time the output surface id changes?

The resource ids in a frame are scoped to a single OutputSurface in a renderer.
When the output surface id changes, the surface factory needs to be destroyed so
that resource ids from the previous output surface aren't returned to the new
output surface. The resources in the renderer were actually destroyed already
when the renderer created the new output surface/id.

[kenrb, 2015-05-29 19:29:37]

Updated.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.h:90: virtual void
SetChildFrameSurface(cc::SurfaceId& surface_id,
On 2015/05/28 22:16:30, dcheng wrote:
> No mutable references in arguments in Chrome.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/cross_process_frame_connector.h:91: gfx::Size
frame_size,
On 2015/05/28 22:16:30, dcheng wrote:
> This should probably be const gfx::Size&.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:276:
scoped_ptr<ImageTransportFactory>(
On 2015/05/28 22:16:30, dcheng wrote:
> make_scoped_ptr will save you some typing.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:40:
RenderWidgetHostViewChildFrame::~RenderWidgetHostViewChildFrame() {
On 2015/05/28 22:32:52, ncarter wrote:
> If there are still entries in surface_returned_resources_ at this point
(because
> the RWHVCF is going away while we still have an ack pending) is it possible
that
> we'd be leaking something in the child process?
> 
> Or does surface_factory_->Destroy take care of that?

John answered this separately, but yes: RWHVCF being destroyed means the
RenderWidget and the RenderWidgetCompositor in the renderer are being destroyed,
so resources will be released.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:214: if
(host_)
On 2015/05/28 20:42:36, nasko wrote:
> nit: Two line body requires {}.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:250:
surface_factory_.reset();
On 2015/05/28 22:32:52, ncarter wrote:
> Why do we need a new SurfaceFactory each time the output surface id changes?

Answered.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:276: // The
renderer will satisfy this dependency when it creates a
On 2015/05/28 20:42:36, nasko wrote:
> nit: s/renderer/renderer process/?

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.cc:288:
ack_pending_count_++;
On 2015/05/28 22:32:52, ncarter wrote:
> Could we dcheck some upper limit for ack_pending_count_ (the idea being that
> getting more than a few frames ahead is madness anyhow, and such a check might
> detect a bug where our increments and decrements don't balance out?)

That's a good idea. I don't really know what a safe threshold would be, but I
would expect 1000 to indicate a bug somewhere.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:42: public
base::SupportsWeakPtr<RenderWidgetHostViewChildFrame> {
On 2015/05/28 22:24:55, dcheng wrote:
> Prefer to embed a WeakPtrFactory rather than inheriting from
SupportsWeakPtr<>.
> Doing the latter allows other people to grab a weak pointer to your class,
which
> we don't need (or want) in this case.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:165: // Flag
determining whether we render into a compositing surface.
On 2015/05/28 20:42:36, nasko wrote:
> nit: Is it Surface? You used capitalized version right below, so just checking
> for consistency.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame.h:176: uint32
ack_pending_count_;
On 2015/05/28 20:42:36, nasko wrote:
> nit: All other variables have some sort of "surface" substring in them. Should
> this one have too?

It is tracking expected CompositorFrameAcks. It is only used on the context of
surfaces but I think the name is fine for what it is.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc
(right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:129:
gfx::Rect damage) {
On 2015/05/28 22:16:30, dcheng wrote:
> gfx::Rect should be passed a const ref.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:138:
return frame.Pass();
On 2015/05/28 22:16:30, dcheng wrote:
> This Pass() should be unnecessary due to NRVO.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_child_frame_unittest.cc:149:
//
On 2015/05/28 22:32:52, ncarter wrote:
> //

Missing test description added.

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
File content/browser/frame_host/render_widget_host_view_guest.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/browser/frame_...
content/browser/frame_host/render_widget_host_view_guest.cc:393: // Compositing
surfaces not supported.
On 2015/05/28 22:32:52, ncarter wrote:
> Is this a job for NOTIMPLEMENTED()? Or will we not eventually have to make
this
> work?

We will want to make it work later, but in the meantime this method will get
called sometimes anyway. The placeholder '0' ends up getting passed to the
LayerTreeHost in the renderer process and never used (because no surfaces will
be created).

https://codereview.chromium.org/1094113003/diff/360001/content/common/cc_mess...
File content/common/cc_messages.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/common/cc_mess...
content/common/cc_messages.h:152: IPC_STRUCT_TRAITS_MEMBER(id_namespace)
On 2015/05/28 20:42:36, nasko wrote:
> nit: Those and the above should be indented to match other struct
declarations.

Done.

https://codereview.chromium.org/1094113003/diff/360001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/common/frame_m...
content/common/frame_messages.h:849: // TODO
On 2015/05/28 20:42:36, nasko wrote:
> Missing comments?

Added.

https://codereview.chromium.org/1094113003/diff/360001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.cc (right):

https://codereview.chromium.org/1094113003/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.cc:68:
resource_collection_->SetClient(nullptr);
On 2015/05/28 22:32:52, ncarter wrote:
> We've given pointers to this object to the surface_layer_ (via the
> satisfy/require callbacks). And the surface_layer_ is refcounted so it
> presumably could outlive the CFCH.
> 
> What guarantees that the callbacks won't be invoked after the CFCH is deleted?

I changed the arguments to weak pointers, because I couldn't find a reason why
that would never happen.

https://codereview.chromium.org/1094113003/diff/360001/content/renderer/child...
File content/renderer/child_frame_compositing_helper.h (right):

https://codereview.chromium.org/1094113003/diff/360001/content/renderer/child...
content/renderer/child_frame_compositing_helper.h:110: cc::SurfaceSequence
sequence);
On 2015/05/28 22:16:30, dcheng wrote:
> Is there a convention for whether this is passed by value or reference?

It has to match the base::Callback definitions in surface_layer.h.

[ncarter (slow), 2015-05-29 20:23:38]

lgtm

[nasko, 2015-05-29 20:48:51]

lgtm

[kenrb, 2015-05-29 21:11:31]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2015-05-29 21:11:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/1094113003/#ps400001
(title: "Missed one")

[commit-bot: I haz the power, 2015-05-29 21:13:26]

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

[commit-bot: I haz the power, 2015-05-29 22:21:05]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2015-05-29 22:22:09]

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