Adding RenderWidgetHostViewChildFrame for OOPIF view.

Adding RenderWidgetHostViewChildFrame for OOPIF view.

RenderWidgetHostViewChildFrame becomes the view class for child frames being
rendered in a different process from their parent. CrossProcessFrameConnector
is a supporting class for that, encapsulating state specific to the
parent/child frame relationship.

RenderWidgetHostViewGuest is made a subclass of RenderWidgetHostViewChildFrame
in order to keep them synchronized. Gradually we will move all functionality
from RWHVGuest to RWHVChildFrame and then get rid of RWHVGuest altogether.

TBR=sadrul
BUG=325803
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242942

[kenrb, 2013-12-10 21:28:09]

Antoine, Fady: PTAL?

[kenrb, 2013-12-11 22:54:33]

Adding Istiaque as an alternative reviewer for the RenderWidgetHostViewGuest
change (Fady is OOO), and Charlie for thoughts on CrossProcessFrameConnector.

[Charlie Reis, 2013-12-12 02:42:04]

Just a few comments so far to help me understand.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:23: //
CrossProcessFrameConnector mediates between a parent frame and an embedded
Thanks-- I like the name much more than CrossProcessChildFrame.

I'm not sure what "mediates" means here.  Can you elaborate in the comment?

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:25: // process state
for the child frame and has logic for message passing between
nit: passing messages

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:33:
RenderFrameHostImpl* frame_proxy_to_parent_renderer);
Please document what this parameter is supposed to be.  I'm not sure what I
would pass in if I called it.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:64: //
frame_proxy_to_parent_renderer_ the RenderFrameHostProxy that routes
nit: is the

Actually, just start with "The" and skip the member name, here and below.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:73:
RenderWidgetHostImpl* child_frame_widget_;
Why isn't this on RenderFrameHostImpl instead?  I thought we were eventually
going to move to RFHs having a RWH.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:54: void
SetCrossProcessFrameConnector(CrossProcessFrameConnector*);
style: Please name parameters in .h files.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:85: // This will move to
RenderFrameProxyHost when that class is created.
Does that mean it's only used (i.e., non-NULL) when this RFH is swapped out?

[kenrb, 2013-12-13 18:41:16]

It might be worth clarifying message flows.

Child frame to parent frame messages (not postmessage or anything generic, but
those specific to child->parent) go like this:
(Child Process)RenderFrame/RenderWidget --(IPC)--> (Child
Process)RenderFrameHost/RenderWidgetHost --> RenderWidgetHostViewChildFrame -->
CrossProcessFrameConnector --> (Parent Process)RenderFrameProxyHost -(IPC)->
(Parent Process)RenderFrameProxy

Message going from parent frame process to child go along the same path in the
reverse direction.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:23: //
CrossProcessFrameConnector mediates between a parent frame and an embedded
On 2013/12/12 02:42:05, creis wrote:
> Thanks-- I like the name much more than CrossProcessChildFrame.
> 
> I'm not sure what "mediates" means here.  Can you elaborate in the comment?

Done.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:33:
RenderFrameHostImpl* frame_proxy_to_parent_renderer);
On 2013/12/12 02:42:05, creis wrote:
> Please document what this parameter is supposed to be.  I'm not sure what I
> would pass in if I called it.

Would this be redundant with the comment on the member variable of the same
name? Or would the description be better here than down there?

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:64: //
frame_proxy_to_parent_renderer_ the RenderFrameHostProxy that routes
On 2013/12/12 02:42:05, creis wrote:
> nit: is the
> 
> Actually, just start with "The" and skip the member name, here and below.

Done.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:73:
RenderWidgetHostImpl* child_frame_widget_;
On 2013/12/12 02:42:05, creis wrote:
> Why isn't this on RenderFrameHostImpl instead?  I thought we were eventually
> going to move to RFHs having a RWH.

RenderWidgetHostImpl will be owned by RenderFrameHostImpl (eventually), and
RenderFrameHostImpl is responsible for setting and clearing the pointer in
CrossProcessFrameConnector. It needs to be cached rather than accessed directly
because this class shouldn't need a pointer to RenderFrameHostImpl (not counting
the stub RFH in the parent process, which should be a RenderFrameProxyHostImpl).

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:54: void
SetCrossProcessFrameConnector(CrossProcessFrameConnector*);
On 2013/12/12 02:42:05, creis wrote:
> style: Please name parameters in .h files.

Done.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:85: // This will move to
RenderFrameProxyHost when that class is created.
On 2013/12/12 02:42:05, creis wrote:
> Does that mean it's only used (i.e., non-NULL) when this RFH is swapped out?

Yes, although it will also be NULL on swapped out RFH's that correspond to
frames that are not direct children (which I don't think we have, yet).

I have made the comment more explicit about that.

[awong, 2013-12-14 02:25:13]

Added a few nits for Ken.

piman & fsamuel: we'd like to try to get this CL in next week. I'm guessing both
of you are already quite familiar with this code. If you don't mind taking a
look, "this CL doesn't seem all to complicated..." (famous last words)

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:32: bool
CrossProcessFrameConnector::OnMessageReceived(const IPC::Message &msg) {
& should associate left to be consistent with rest of file.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:32: static
CrossProcessFrameConnector* CreateCrossProcessFrameConnector(
Why bother with the creation method?

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:56:
RenderFrameHostImpl* frame_proxy_to_parent_renderer);
explicit

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.cc:146:
frame_connector_->Destroy();
Hmm...this looks like RenderWidgetHostViewChildFrame has an own-relationship
with |frame_connectgor_|.

Why don't we just use a scoped_ptr<CrossProcessFrameConnector> frame_connector_
here and not bother with the Destroy() method? Seems cleaner than having a
self-deleting function in CrossProcessFrameConnector.

[Charlie Reis, 2013-12-16 19:31:08]

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:33:
RenderFrameHostImpl* frame_proxy_to_parent_renderer);
On 2013/12/13 18:41:17, kenrb wrote:
> On 2013/12/12 02:42:05, creis wrote:
> > Please document what this parameter is supposed to be.  I'm not sure what I
> > would pass in if I called it.
> 
> Would this be redundant with the comment on the member variable of the same
> name? Or would the description be better here than down there?

That comment says it's initially NULL, which doesn't make sense if we're
supposed to pass it into a creation method.

https://codereview.chromium.org/100473010/diff/50001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:73:
RenderWidgetHostImpl* child_frame_widget_;
On 2013/12/13 18:41:17, kenrb wrote:
> On 2013/12/12 02:42:05, creis wrote:
> > Why isn't this on RenderFrameHostImpl instead?  I thought we were eventually
> > going to move to RFHs having a RWH.
> 
> RenderWidgetHostImpl will be owned by RenderFrameHostImpl (eventually), and
> RenderFrameHostImpl is responsible for setting and clearing the pointer in
> CrossProcessFrameConnector. It needs to be cached rather than accessed
directly
> because this class shouldn't need a pointer to RenderFrameHostImpl (not
counting
> the stub RFH in the parent process, which should be a
RenderFrameProxyHostImpl).

What do you mean by cached?  Will it ever be stale?  Or do you mean it's just
stored here rather than there?

It seems like this is information that belongs on RenderFrameProxyHost, and
might more naturally belong on RenderFrameHost in the meantime.  Then we might
not need this class at all.  (Sorry, I'm sure I'm missing something, but I want
to understand why this class is needed before we proceed with it.)

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:86: // swapped out and
corresponds to a direct child frame of a frame in its
What does "corresponds to a direct child frame of a frame in its renderer
process" mean?  Maybe an example would help.

[Charlie Reis, 2013-12-16 21:35:32]

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:28: class
CrossProcessFrameConnector {
Ok, Albert tried to explain this class to me, and I think I understand it a bit
more.  Let's see if I can summarize:

Suppose we have a top-level frame A1 (site A, frame 1), with a subframe B2. 
When painting, we need some link between the *RenderWidgetHostViewChildFrame* of
the active RenderFrameHost B2 and the swapped out *RenderFrameHost* A2.  This
allows buffer swap messages to pass between them.

The key reason this class is useful is the difference in layers: one side is a
RWHVCF, and the other is a RFH.  Normally RFHs shouldn't have direct pointers to
their RWHV.  If it weren't for that, I would argue that the RFH B2 and the RFH
A2 should point to each other (or that the RFHM should keep track of the link),
rather than introducing a new class for it.

That motivation doesn't come across in the class-level description above.  If
that's actually the reason this exists, we should clarify it in the comments to
make it easier for others to understand.

[nasko, 2013-12-16 23:16:32]

A drive-by review, mostly style and nits.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:16:
RenderFrameHostImpl* frame_proxy_to_parent_renderer) :
style: The colon should be on next line, 4 spaces indented.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:46:
child_frame_widget_ =
style: Two spaces indentation for nested blocks.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:76: int route_id,
style: The latter arguments should be aligned with the first one.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:64: // The
RenderFrameHostProxy that routes messages to the parent frame's
nit: RenderFrameHostProxy doesn't exist yet. Did you mean RenderFrameHostImpl?

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:45:
cross_process_frame_connector_(0),
nit: Use NULL instead of 0, which is the Chromium code style.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.cc:155: void
RenderWidgetHostViewChildFrame::OnAcceleratedCompositingStateChange() {
There are several empty methods here, why not keep them in the header file only?

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.cc:196: const
NativeWebKeyboardEvent& event) {
style: Only 4 spaces for indent.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.cc:242: const
gfx::Rect& src_subrect,
style: Only 4 spaces needed.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.cc:261: const
gfx::Size& desired_size) {
style: Only 4 space needed.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.h:113: virtual
void AcceleratedSurfaceSuspend() OVERRIDE;
Why do some methods have empty bodies and some not?

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.h:159: OVERRIDE
{}
style: OVERRIDE shouldn't be on its own line, combine with others.

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
content/common/frame_messages.h:113: 
nit: just one empty line

[piman, 2013-12-17 00:14:56]

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
content/common/frame_messages.h:30: int /* route_id */,
route_id of what? How does that relate to the implicit routing id from this
ROUTED message?

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
content/common/frame_messages.h:31: int /* gpu_host_id */)
2 things about these messages:
1- I don't see the renderer-side handlers for these.
2- These seem to duplicate the
BrowserPluginMsg_BuffersSwapped/BrowserPluginMsg_CompositorFrameSwapped
messages... is it possible to factor the logic between the 2? It's a non-trivial
amount of code with subtle things (though we're trying to make it better), and
I'd hate to duplicate it all (see the BrowserPluginCompositingHelper)

[awong, 2013-12-17 00:44:13]

Still working through comments, but wanted to publish updates to
cross_process_frame_connector.h

@creis, @nasko: can you both look at this in parallel with me making the other
changes?

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.cc (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:16:
RenderFrameHostImpl* frame_proxy_to_parent_renderer) :
On 2013/12/16 23:16:32, nasko wrote:
> style: The colon should be on next line, 4 spaces indented.

Done.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:32: bool
CrossProcessFrameConnector::OnMessageReceived(const IPC::Message &msg) {
On 2013/12/14 02:25:14, awong wrote:
> & should associate left to be consistent with rest of file.

Done.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:46:
child_frame_widget_ =
On 2013/12/16 23:16:32, nasko wrote:
> style: Two spaces indentation for nested blocks.

rewrote function.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.cc:76: int route_id,
On 2013/12/16 23:16:32, nasko wrote:
> style: The latter arguments should be aligned with the first one.

Done.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:28: class
CrossProcessFrameConnector {
On 2013/12/16 21:35:32, creis wrote:
> Ok, Albert tried to explain this class to me, and I think I understand it a
bit
> more.  Let's see if I can summarize:
> 
> Suppose we have a top-level frame A1 (site A, frame 1), with a subframe B2. 
> When painting, we need some link between the *RenderWidgetHostViewChildFrame*
of
> the active RenderFrameHost B2 and the swapped out *RenderFrameHost* A2.  This
> allows buffer swap messages to pass between them.
> 
> The key reason this class is useful is the difference in layers: one side is a
> RWHVCF, and the other is a RFH.  Normally RFHs shouldn't have direct pointers
to
> their RWHV.  If it weren't for that, I would argue that the RFH B2 and the RFH
> A2 should point to each other (or that the RFHM should keep track of the
link),
> rather than introducing a new class for it.
> 
> That motivation doesn't come across in the class-level description above.  If
> that's actually the reason this exists, we should clarify it in the comments
to
> make it easier for others to understand.

I rewrote the comment. Left a TODO for ken to verify when he gets back.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:32: static
CrossProcessFrameConnector* CreateCrossProcessFrameConnector(
On 2013/12/14 02:25:14, awong wrote:
> Why bother with the creation method?

Done.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:56:
RenderFrameHostImpl* frame_proxy_to_parent_renderer);
On 2013/12/14 02:25:14, awong wrote:
> explicit

Done.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/cross_process_frame_connector.h:64: // The
RenderFrameHostProxy that routes messages to the parent frame's
On 2013/12/16 23:16:32, nasko wrote:
> nit: RenderFrameHostProxy doesn't exist yet. Did you mean RenderFrameHostImpl?

Done.

[nasko, 2013-12-17 00:56:52]

Couple of comments.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:47: // to allow for
this communiation. (Note B1 is only mentioned for completeness.
nit: communication

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:93: //   (b) has a child
frame that
This doesn't end properly, nor does it read coherently if you continue with next
line.

[Charlie Reis, 2013-12-17 01:09:30]

Thanks, that class-level comment is much more helpful.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:44: //
RenderWidgetHostViewChildFrame. This RenderWidgetHostViewChildFrame needs
nit: needs to

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:46: // B2's data with
A1's. CrossProcessFrameConnector bridges between B2 to A2
bridges between B2's RWHVChildFrame and A2

(or am I mistaken?)

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:57: explicit
CrossProcessFrameConnector(
Since you've spelled out the example so nicely above, can you add a comment here
saying this expects RFH A2 from the example?

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:89:
RenderFrameHostImpl* frame_proxy_in_parent_renderer_;
This name is much clearer.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:73:
cross_process_frame_connector_ = cross_process_frame_connector;
This is simple and can be inlined in the .h file, with the name
set_cross_process_frame_connector.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:55: void
SetCrossProcessFrameConnector(
It feels odd to me to have no comments on the public contract.  We should at
least tell people to look at the member declaration for an explanation, but
perhaps we can say something about when this should be called.

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

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:27:
frame_connector_ = frame_connector;
Can be inlined in the .h file, unix_hacker_style.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:48: void
RenderWidgetHostViewChildFrame::Focus() {
Do we need this (and other empty methods) in the .cc file?  You have {} in the
.h file for other functions.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:83: return 0;
0 -> NULL?
(throughout the class)

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:34: void
SetCrossProcessFrameConnector(CrossProcessFrameConnector*);
Needs a parameter name.

https://codereview.chromium.org/100473010/diff/170001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/common/frame_me...
content/common/frame_messages.h:24: // When HW accelerated buffers are swapped
in an out of process child frame
nit: out-of-process

[awong, 2013-12-17 02:45:26]

All comments addressed.  PTAL

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:45:
cross_process_frame_connector_(0),
On 2013/12/16 23:16:32, nasko wrote:
> nit: Use NULL instead of 0, which is the Chromium code style.

Done.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:86: // swapped out and
corresponds to a direct child frame of a frame in its
On 2013/12/16 19:31:08, creis wrote:
> What does "corresponds to a direct child frame of a frame in its renderer
> process" mean?  Maybe an example would help.

Fixed.

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.h:113: virtual
void AcceleratedSurfaceSuspend() OVERRIDE;
On 2013/12/16 23:16:32, nasko wrote:
> Why do some methods have empty bodies and some not?

Made consistent

https://codereview.chromium.org/100473010/diff/90001/content/browser/frame_ho...
content/browser/frame_host/render_widget_host_view_child_frame.h:159: OVERRIDE
{}
On 2013/12/16 23:16:32, nasko wrote:
> style: OVERRIDE shouldn't be on its own line, combine with others.

Fixed

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
content/common/frame_messages.h:30: int /* route_id */,
On 2013/12/17 00:14:57, piman wrote:
> route_id of what? How does that relate to the implicit routing id from this
> ROUTED message?

Ooh...good catch. It's the gpu_route_id.  Comes from
GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params.route_id (later in the
plumbing CL).

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
content/common/frame_messages.h:31: int /* gpu_host_id */)
On 2013/12/17 00:14:57, piman wrote:
> 2 things about these messages:
> 1- I don't see the renderer-side handlers for these.
> 2- These seem to duplicate the
> BrowserPluginMsg_BuffersSwapped/BrowserPluginMsg_CompositorFrameSwapped
> messages... is it possible to factor the logic between the 2? It's a
non-trivial
> amount of code with subtle things (though we're trying to make it better), and
> I'd hate to duplicate it all (see the BrowserPluginCompositingHelper)

This is part 1 of a set of CLs that are being upstreamed.  The big-ball
prototype is here:
  https://codereview.chromium.org/download/issue23889010_209001.diff

As for combining with BrowserPluginMsg_, yes, that is definitely on the roadmap.
Ken was working with Fady on this. I think the plan was to land these
first...unblock the Site Isolation work, and the migrate webview over.  Is that
an okay plan, or do you want me to find a way to combine in this CL?

https://codereview.chromium.org/100473010/diff/90001/content/common/frame_mes...
content/common/frame_messages.h:113: 
On 2013/12/16 23:16:32, nasko wrote:
> nit: just one empty line

Done.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:44: //
RenderWidgetHostViewChildFrame. This RenderWidgetHostViewChildFrame needs
On 2013/12/17 01:09:31, creis wrote:
> nit: needs to

Done.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:46: // B2's data with
A1's. CrossProcessFrameConnector bridges between B2 to A2
On 2013/12/17 01:09:31, creis wrote:
> bridges between B2's RWHVChildFrame and A2
> 
> (or am I mistaken?)

Good catch. Fixed.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:47: // to allow for
this communiation. (Note B1 is only mentioned for completeness.
On 2013/12/17 00:56:52, nasko wrote:
> nit: communication

Done.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:57: explicit
CrossProcessFrameConnector(
On 2013/12/17 01:09:31, creis wrote:
> Since you've spelled out the example so nicely above, can you add a comment
here
> saying this expects RFH A2 from the example?

Done.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/cross_process_frame_connector.h:89:
RenderFrameHostImpl* frame_proxy_in_parent_renderer_;
On 2013/12/17 01:09:31, creis wrote:
> This name is much clearer.

yay.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:73:
cross_process_frame_connector_ = cross_process_frame_connector;
On 2013/12/17 01:09:31, creis wrote:
> This is simple and can be inlined in the .h file, with the name
> set_cross_process_frame_connector.

Done.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:55: void
SetCrossProcessFrameConnector(
On 2013/12/17 01:09:31, creis wrote:
> It feels odd to me to have no comments on the public contract.  We should at
> least tell people to look at the member declaration for an explanation, but
> perhaps we can say something about when this should be called.

Done.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:93: //   (b) has a child
frame that
On 2013/12/17 00:56:52, nasko wrote:
> This doesn't end properly, nor does it read coherently if you continue with
next
> line.

Yep...sorry...I hadn't gotten this far yet.

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

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:27:
frame_connector_ = frame_connector;
On 2013/12/17 01:09:31, creis wrote:
> Can be inlined in the .h file, unix_hacker_style.

Done.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:48: void
RenderWidgetHostViewChildFrame::Focus() {
On 2013/12/17 01:09:31, creis wrote:
> Do we need this (and other empty methods) in the .cc file?  You have {} in the
> .h file for other functions.
Went the other direction to match the other RWHVs

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:83: return 0;
On 2013/12/17 01:09:31, creis wrote:
> 0 -> NULL?
> (throughout the class)

Done.

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:34: void
SetCrossProcessFrameConnector(CrossProcessFrameConnector*);
On 2013/12/17 01:09:31, creis wrote:
> Needs a parameter name.

Done.

https://codereview.chromium.org/100473010/diff/170001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/100473010/diff/170001/content/common/frame_me...
content/common/frame_messages.h:24: // When HW accelerated buffers are swapped
in an out of process child frame
On 2013/12/17 01:09:31, creis wrote:
> nit: out-of-process

Done.

[piman, 2013-12-17 03:55:13]

On Mon, Dec 16, 2013 at 6:45 PM, <ajwong@chromium.org> wrote:

> All comments addressed.  PTAL
>
>
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/browser/frame_host/render_frame_host_impl.cc
> File content/browser/frame_host/render_frame_host_impl.cc (right):
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/browser/frame_host/render_frame_host_impl.cc#newcode45
> content/browser/frame_host/render_frame_host_impl.cc:45:
> cross_process_frame_connector_(0),
> On 2013/12/16 23:16:32, nasko wrote:
>
>> nit: Use NULL instead of 0, which is the Chromium code style.
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/browser/frame_host/render_frame_host_impl.h
> File content/browser/frame_host/render_frame_host_impl.h (right):
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/browser/frame_host/render_frame_host_impl.h#newcode86
> content/browser/frame_host/render_frame_host_impl.h:86: // swapped out
> and corresponds to a direct child frame of a frame in its
> On 2013/12/16 19:31:08, creis wrote:
>
>> What does "corresponds to a direct child frame of a frame in its
>>
> renderer
>
>> process" mean?  Maybe an example would help.
>>
>
> Fixed.
>
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/browser/frame_host/render_widget_host_view_child_frame.h
> File content/browser/frame_host/render_widget_host_view_child_frame.h
> (right):
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/browser/frame_host/render_widget_host_view_child_
> frame.h#newcode113
> content/browser/frame_host/render_widget_host_view_child_frame.h:113:
> virtual void AcceleratedSurfaceSuspend() OVERRIDE;
> On 2013/12/16 23:16:32, nasko wrote:
>
>> Why do some methods have empty bodies and some not?
>>
>
> Made consistent
>
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/browser/frame_host/render_widget_host_view_child_
> frame.h#newcode159
> content/browser/frame_host/render_widget_host_view_child_frame.h:159:
> OVERRIDE {}
> On 2013/12/16 23:16:32, nasko wrote:
>
>> style: OVERRIDE shouldn't be on its own line, combine with others.
>>
>
> Fixed
>
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/common/frame_messages.h
> File content/common/frame_messages.h (right):
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/common/frame_messages.h#newcode30
> content/common/frame_messages.h:30: int /* route_id */,
> On 2013/12/17 00:14:57, piman wrote:
>
>> route_id of what? How does that relate to the implicit routing id from
>>
> this
>
>> ROUTED message?
>>
>
> Ooh...good catch. It's the gpu_route_id.  Comes from
> GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params.route_id (later in
> the plumbing CL).
>
>
> https://codereview.chromium.org/100473010/diff/90001/
> content/common/frame_messages.h#newcode31
> content/common/frame_messages.h:31: int /* gpu_host_id */)
> On 2013/12/17 00:14:57, piman wrote:
>
>> 2 things about these messages:
>> 1- I don't see the renderer-side handlers for these.
>> 2- These seem to duplicate the
>>
>
> BrowserPluginMsg_BuffersSwapped/BrowserPluginMsg_CompositorFrameSwapped
>
>> messages... is it possible to factor the logic between the 2? It's a
>>
> non-trivial
>
>> amount of code with subtle things (though we're trying to make it
>>
> better), and
>
>> I'd hate to duplicate it all (see the BrowserPluginCompositingHelper)
>>
>
> This is part 1 of a set of CLs that are being upstreamed.  The big-ball
> prototype is here:
>   https://codereview.chromium.org/download/issue23889010_209001.diff
>
> As for combining with BrowserPluginMsg_, yes, that is definitely on the
> roadmap. Ken was working with Fady on this. I think the plan was to land
> these first...unblock the Site Isolation work, and the migrate webview
> over.  Is that an okay plan, or do you want me to find a way to combine
> in this CL?
>

Well... it kinda depends on how long this will take.
We're working towards simplifying some of this cross-process GPU stuff, in
particular we would like to remove the BuffersSwapped messages (and have
everything go through the SwapCompositorFrame path). It gets harder if we
keep adding users of them as we try to remove others. Also, looking at the
big patch, there's definitely a lot of commonality on the renderer side
between ChildFrameCompositingHelper and BrowserPluginCompositingHelper.
If it's incremental steps, then sg, but if it's going to linger for a long
time, then I'd rather we try to reuse rather than copy&paste.


>
> https://codereview.chromium.org/100473010/diff/90001/
> content/common/frame_messages.h#newcode113
> content/common/frame_messages.h:113:
>
> On 2013/12/16 23:16:32, nasko wrote:
>
>> nit: just one empty line
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/cross_process_frame_connector.h
> File content/browser/frame_host/cross_process_frame_connector.h (right):
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/cross_process_frame_connector.h#newcode44
> content/browser/frame_host/cross_process_frame_connector.h:44: //
> RenderWidgetHostViewChildFrame. This RenderWidgetHostViewChildFrame
> needs
> On 2013/12/17 01:09:31, creis wrote:
>
>> nit: needs to
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/cross_process_frame_connector.h#newcode46
> content/browser/frame_host/cross_process_frame_connector.h:46: // B2's
> data with A1's. CrossProcessFrameConnector bridges between B2 to A2
> On 2013/12/17 01:09:31, creis wrote:
>
>> bridges between B2's RWHVChildFrame and A2
>>
>
>  (or am I mistaken?)
>>
>
> Good catch. Fixed.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/cross_process_frame_connector.h#newcode47
> content/browser/frame_host/cross_process_frame_connector.h:47: // to
> allow for this communiation. (Note B1 is only mentioned for
> completeness.
> On 2013/12/17 00:56:52, nasko wrote:
>
>> nit: communication
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/cross_process_frame_connector.h#newcode57
> content/browser/frame_host/cross_process_frame_connector.h:57: explicit
> CrossProcessFrameConnector(
> On 2013/12/17 01:09:31, creis wrote:
>
>> Since you've spelled out the example so nicely above, can you add a
>>
> comment here
>
>> saying this expects RFH A2 from the example?
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/cross_process_frame_connector.h#newcode89
> content/browser/frame_host/cross_process_frame_connector.h:89:
> RenderFrameHostImpl* frame_proxy_in_parent_renderer_;
> On 2013/12/17 01:09:31, creis wrote:
>
>> This name is much clearer.
>>
>
> yay.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_frame_host_impl.cc
> File content/browser/frame_host/render_frame_host_impl.cc (right):
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_frame_host_impl.cc#newcode73
> content/browser/frame_host/render_frame_host_impl.cc:73:
> cross_process_frame_connector_ = cross_process_frame_connector;
> On 2013/12/17 01:09:31, creis wrote:
>
>> This is simple and can be inlined in the .h file, with the name
>> set_cross_process_frame_connector.
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_frame_host_impl.h
> File content/browser/frame_host/render_frame_host_impl.h (right):
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_frame_host_impl.h#newcode55
> content/browser/frame_host/render_frame_host_impl.h:55: void
> SetCrossProcessFrameConnector(
> On 2013/12/17 01:09:31, creis wrote:
>
>> It feels odd to me to have no comments on the public contract.  We
>>
> should at
>
>> least tell people to look at the member declaration for an
>>
> explanation, but
>
>> perhaps we can say something about when this should be called.
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_frame_host_impl.h#newcode93
> content/browser/frame_host/render_frame_host_impl.h:93: //   (b) has a
> child frame that
> On 2013/12/17 00:56:52, nasko wrote:
>
>> This doesn't end properly, nor does it read coherently if you continue
>>
> with next
>
>> line.
>>
>
> Yep...sorry...I hadn't gotten this far yet.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_widget_host_view_child_frame.cc
> File content/browser/frame_host/render_widget_host_view_child_frame.cc
> (right):
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_widget_host_view_child_
> frame.cc#newcode27
> content/browser/frame_host/render_widget_host_view_child_frame.cc:27:
> frame_connector_ = frame_connector;
> On 2013/12/17 01:09:31, creis wrote:
>
>> Can be inlined in the .h file, unix_hacker_style.
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_widget_host_view_child_
> frame.cc#newcode48
> content/browser/frame_host/render_widget_host_view_child_frame.cc:48:
> void RenderWidgetHostViewChildFrame::Focus() {
> On 2013/12/17 01:09:31, creis wrote:
>
>> Do we need this (and other empty methods) in the .cc file?  You have
>>
> {} in the
>
>> .h file for other functions.
>>
> Went the other direction to match the other RWHVs
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_widget_host_view_child_
> frame.cc#newcode83
> content/browser/frame_host/render_widget_host_view_child_frame.cc:83:
> return 0;
> On 2013/12/17 01:09:31, creis wrote:
>
>> 0 -> NULL?
>> (throughout the class)
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_widget_host_view_child_frame.h
> File content/browser/frame_host/render_widget_host_view_child_frame.h
> (right):
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/browser/frame_host/render_widget_host_view_child_frame.h#newcode34
> content/browser/frame_host/render_widget_host_view_child_frame.h:34:
> void SetCrossProcessFrameConnector(CrossProcessFrameConnector*);
> On 2013/12/17 01:09:31, creis wrote:
>
>> Needs a parameter name.
>>
>
> Done.
>
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/common/frame_messages.h
> File content/common/frame_messages.h (right):
>
> https://codereview.chromium.org/100473010/diff/170001/
> content/common/frame_messages.h#newcode24
> content/common/frame_messages.h:24: // When HW accelerated buffers are
> swapped in an out of process child frame
> On 2013/12/17 01:09:31, creis wrote:
>
>> nit: out-of-process
>>
>
> Done.
>
> https://codereview.chromium.org/100473010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[awong, 2013-12-17 04:47:24]

On Mon, Dec 16, 2013 at 7:54 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Mon, Dec 16, 2013 at 6:45 PM, <ajwong@chromium.org> wrote:
>
>> All comments addressed.  PTAL
>>
>>
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/browser/frame_host/render_frame_host_impl.cc
>> File content/browser/frame_host/render_frame_host_impl.cc (right):
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/browser/frame_host/render_frame_host_impl.cc#newcode45
>> content/browser/frame_host/render_frame_host_impl.cc:45:
>> cross_process_frame_connector_(0),
>> On 2013/12/16 23:16:32, nasko wrote:
>>
>>> nit: Use NULL instead of 0, which is the Chromium code style.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/browser/frame_host/render_frame_host_impl.h
>> File content/browser/frame_host/render_frame_host_impl.h (right):
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/browser/frame_host/render_frame_host_impl.h#newcode86
>> content/browser/frame_host/render_frame_host_impl.h:86: // swapped out
>> and corresponds to a direct child frame of a frame in its
>> On 2013/12/16 19:31:08, creis wrote:
>>
>>> What does "corresponds to a direct child frame of a frame in its
>>>
>> renderer
>>
>>> process" mean?  Maybe an example would help.
>>>
>>
>> Fixed.
>>
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/browser/frame_host/render_widget_host_view_child_frame.h
>> File content/browser/frame_host/render_widget_host_view_child_frame.h
>> (right):
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/browser/frame_host/render_widget_host_view_child_
>> frame.h#newcode113
>> content/browser/frame_host/render_widget_host_view_child_frame.h:113:
>> virtual void AcceleratedSurfaceSuspend() OVERRIDE;
>> On 2013/12/16 23:16:32, nasko wrote:
>>
>>> Why do some methods have empty bodies and some not?
>>>
>>
>> Made consistent
>>
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/browser/frame_host/render_widget_host_view_child_
>> frame.h#newcode159
>> content/browser/frame_host/render_widget_host_view_child_frame.h:159:
>> OVERRIDE {}
>> On 2013/12/16 23:16:32, nasko wrote:
>>
>>> style: OVERRIDE shouldn't be on its own line, combine with others.
>>>
>>
>> Fixed
>>
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/common/frame_messages.h
>> File content/common/frame_messages.h (right):
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/common/frame_messages.h#newcode30
>> content/common/frame_messages.h:30: int /* route_id */,
>> On 2013/12/17 00:14:57, piman wrote:
>>
>>> route_id of what? How does that relate to the implicit routing id from
>>>
>> this
>>
>>> ROUTED message?
>>>
>>
>> Ooh...good catch. It's the gpu_route_id.  Comes from
>> GpuHostMsg_AcceleratedSurfaceBuffersSwapped_Params.route_id (later in
>> the plumbing CL).
>>
>>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/common/frame_messages.h#newcode31
>> content/common/frame_messages.h:31: int /* gpu_host_id */)
>> On 2013/12/17 00:14:57, piman wrote:
>>
>>> 2 things about these messages:
>>> 1- I don't see the renderer-side handlers for these.
>>> 2- These seem to duplicate the
>>>
>>
>> BrowserPluginMsg_BuffersSwapped/BrowserPluginMsg_CompositorFrameSwapped
>>
>>> messages... is it possible to factor the logic between the 2? It's a
>>>
>> non-trivial
>>
>>> amount of code with subtle things (though we're trying to make it
>>>
>> better), and
>>
>>> I'd hate to duplicate it all (see the BrowserPluginCompositingHelper)
>>>
>>
>> This is part 1 of a set of CLs that are being upstreamed.  The big-ball
>> prototype is here:
>>   https://codereview.chromium.org/download/issue23889010_209001.diff
>>
>> As for combining with BrowserPluginMsg_, yes, that is definitely on the
>> roadmap. Ken was working with Fady on this. I think the plan was to land
>> these first...unblock the Site Isolation work, and the migrate webview
>> over.  Is that an okay plan, or do you want me to find a way to combine
>> in this CL?
>>
>
> Well... it kinda depends on how long this will take.
> We're working towards simplifying some of this cross-process GPU stuff, in
> particular we would like to remove the BuffersSwapped messages (and have
> everything go through the SwapCompositorFrame path). It gets harder if we
> keep adding users of them as we try to remove others. Also, looking at the
> big patch, there's definitely a lot of commonality on the renderer side
> between ChildFrameCompositingHelper and BrowserPluginCompositingHelper.
> If it's incremental steps, then sg, but if it's going to linger for a long
> time, then I'd rather we try to reuse rather than copy&paste.
>

Hmm...well it's definitely on the road to combining...but it's hard for me
to predict Ken's timeline with him being out.  I'll look at it tomorrow
morning and see how hard it'd be to combine.


>
>> https://codereview.chromium.org/100473010/diff/90001/
>> content/common/frame_messages.h#newcode113
>> content/common/frame_messages.h:113:
>>
>> On 2013/12/16 23:16:32, nasko wrote:
>>
>>> nit: just one empty line
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/cross_process_frame_connector.h
>> File content/browser/frame_host/cross_process_frame_connector.h (right):
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/cross_process_frame_connector.h#newcode44
>> content/browser/frame_host/cross_process_frame_connector.h:44: //
>> RenderWidgetHostViewChildFrame. This RenderWidgetHostViewChildFrame
>> needs
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> nit: needs to
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/cross_process_frame_connector.h#newcode46
>> content/browser/frame_host/cross_process_frame_connector.h:46: // B2's
>> data with A1's. CrossProcessFrameConnector bridges between B2 to A2
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> bridges between B2's RWHVChildFrame and A2
>>>
>>
>>  (or am I mistaken?)
>>>
>>
>> Good catch. Fixed.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/cross_process_frame_connector.h#newcode47
>> content/browser/frame_host/cross_process_frame_connector.h:47: // to
>> allow for this communiation. (Note B1 is only mentioned for
>> completeness.
>> On 2013/12/17 00:56:52, nasko wrote:
>>
>>> nit: communication
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/cross_process_frame_connector.h#newcode57
>> content/browser/frame_host/cross_process_frame_connector.h:57: explicit
>> CrossProcessFrameConnector(
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> Since you've spelled out the example so nicely above, can you add a
>>>
>> comment here
>>
>>> saying this expects RFH A2 from the example?
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/cross_process_frame_connector.h#newcode89
>> content/browser/frame_host/cross_process_frame_connector.h:89:
>> RenderFrameHostImpl* frame_proxy_in_parent_renderer_;
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> This name is much clearer.
>>>
>>
>> yay.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_frame_host_impl.cc
>> File content/browser/frame_host/render_frame_host_impl.cc (right):
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_frame_host_impl.cc#newcode73
>> content/browser/frame_host/render_frame_host_impl.cc:73:
>> cross_process_frame_connector_ = cross_process_frame_connector;
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> This is simple and can be inlined in the .h file, with the name
>>> set_cross_process_frame_connector.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_frame_host_impl.h
>> File content/browser/frame_host/render_frame_host_impl.h (right):
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_frame_host_impl.h#newcode55
>> content/browser/frame_host/render_frame_host_impl.h:55: void
>> SetCrossProcessFrameConnector(
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> It feels odd to me to have no comments on the public contract.  We
>>>
>> should at
>>
>>> least tell people to look at the member declaration for an
>>>
>> explanation, but
>>
>>> perhaps we can say something about when this should be called.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_frame_host_impl.h#newcode93
>> content/browser/frame_host/render_frame_host_impl.h:93: //   (b) has a
>> child frame that
>> On 2013/12/17 00:56:52, nasko wrote:
>>
>>> This doesn't end properly, nor does it read coherently if you continue
>>>
>> with next
>>
>>> line.
>>>
>>
>> Yep...sorry...I hadn't gotten this far yet.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_widget_host_view_child_frame.cc
>> File content/browser/frame_host/render_widget_host_view_child_frame.cc
>> (right):
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_widget_host_view_child_
>> frame.cc#newcode27
>> content/browser/frame_host/render_widget_host_view_child_frame.cc:27:
>> frame_connector_ = frame_connector;
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> Can be inlined in the .h file, unix_hacker_style.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_widget_host_view_child_
>> frame.cc#newcode48
>> content/browser/frame_host/render_widget_host_view_child_frame.cc:48:
>> void RenderWidgetHostViewChildFrame::Focus() {
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> Do we need this (and other empty methods) in the .cc file?  You have
>>>
>> {} in the
>>
>>> .h file for other functions.
>>>
>> Went the other direction to match the other RWHVs
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_widget_host_view_child_
>> frame.cc#newcode83
>> content/browser/frame_host/render_widget_host_view_child_frame.cc:83:
>> return 0;
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> 0 -> NULL?
>>> (throughout the class)
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_widget_host_view_child_frame.h
>> File content/browser/frame_host/render_widget_host_view_child_frame.h
>> (right):
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/browser/frame_host/render_widget_host_view_child_
>> frame.h#newcode34
>> content/browser/frame_host/render_widget_host_view_child_frame.h:34:
>> void SetCrossProcessFrameConnector(CrossProcessFrameConnector*);
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> Needs a parameter name.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/common/frame_messages.h
>> File content/common/frame_messages.h (right):
>>
>> https://codereview.chromium.org/100473010/diff/170001/
>> content/common/frame_messages.h#newcode24
>> content/common/frame_messages.h:24: // When HW accelerated buffers are
>> swapped in an out of process child frame
>> On 2013/12/17 01:09:31, creis wrote:
>>
>>> nit: out-of-process
>>>
>>
>> Done.
>>
>> https://codereview.chromium.org/100473010/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[awong, 2013-12-19 02:07:49]

Okay, time for review again.

Dropping @piman since he's out until 1/7.

[Fady Samuel, 2013-12-19 21:35:00]

browser_plugin LGTM.

https://codereview.chromium.org/100473010/diff/310001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_guest_unittest.cc
(left):

https://codereview.chromium.org/100473010/diff/310001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_guest_unittest.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
What happened to these tests?

https://codereview.chromium.org/100473010/diff/310001/content/common/browser_...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/100473010/diff/310001/content/common/browser_...
content/common/browser_plugin/browser_plugin_messages.h:368: // because
BrowserPlugins implement custom a second leverl of routing logic,
s/leverl/level

[awong, 2013-12-19 21:45:35]

@sadrul: Fady said to ping you. Can you look at the changes related to
RenderWidgetHostView*?

Thanks!

[Charlie Reis, 2013-12-20 00:24:21]

CrossProcessFrameConnector LGTM, though I defer to others on the RWHV-related
stuff.  Thanks for clarifying things!

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:55: // This function is
called when this is a swapped out RenderFrameHost that
"when this is a swapped out RenderFrameHost:" That's a temporary state.  Does
this need to be called with NULL if this RenderFrameHost becomes active again?

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:57: //
|cross_process_frame_connector| allows the non-swapped-out
nit: non-swapped-out -> active

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:106: // the same site
instance as this frame's parent.
nit: SiteInstance

[nasko, 2013-12-20 17:21:04]

A drive-by review with mostly nits.

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_RENDER_WIDGET_HOST_VIEW_GUEST_H_
The guard should be updated to reflect the new path.

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.h:35: // The webview
plugin is currently a special case for out-of-process rendered
nit: It should be either <webview> or BrowserPlugin, not "webview plugin"

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_me...
content/common/frame_messages.h:23: // renderer, the message is forwarded to the
top level frame renderer to
Is it really the top level or the parent? I thought short term it was sending to
parent process.

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_me...
content/common/frame_messages.h:28:
IPC_MESSAGE_ROUTED1(FrameMsg_CompositorFrameSwapped,
nit: It will be nice to have a comment on this IPC, at least to describe how it
differs from the above.

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
File content/common/frame_param.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
content/common/frame_param.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
nit: (c) is no longer required.

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
File content/common/frame_param_macros.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
content/common/frame_param_macros.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
content/common/frame_param_macros.h:8: // the RenderFrame infrastructure.
nit: Maybe a TODO for easy searching later on?

https://codereview.chromium.org/100473010/diff/350001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/100473010/diff/350001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:192:
params.producing_host_id = last_host_id_;
Is this intentional that use last_host_id_ here and it used to be
mailbox.route_id? In all the cases above it is just moving the same params into
the IPC struct, but here it differs.

[awong, 2013-12-20 23:53:36]

Commetns adressed.

https://codereview.chromium.org/100473010/diff/310001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_guest_unittest.cc
(left):

https://codereview.chromium.org/100473010/diff/310001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_guest_unittest.cc:1: //
Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/12/19 21:35:01, Fady Samuel wrote:
> What happened to these tests?

This is a file move. I'll see if I can fix in the revision history.  It's in
frame_host.

https://codereview.chromium.org/100473010/diff/310001/content/common/browser_...
File content/common/browser_plugin/browser_plugin_messages.h (right):

https://codereview.chromium.org/100473010/diff/310001/content/common/browser_...
content/common/browser_plugin/browser_plugin_messages.h:368: // because
BrowserPlugins implement custom a second leverl of routing logic,
On 2013/12/19 21:35:01, Fady Samuel wrote:
> s/leverl/level

Done.

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_guest.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_RENDER_WIDGET_HOST_VIEW_GUEST_H_
On 2013/12/20 17:21:05, nasko wrote:
> The guard should be updated to reflect the new path.

Done.

https://codereview.chromium.org/100473010/diff/350001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_guest.h:35: // The webview
plugin is currently a special case for out-of-process rendered
On 2013/12/20 17:21:05, nasko wrote:
> nit: It should be either <webview> or BrowserPlugin, not "webview plugin"

Done.

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_me...
content/common/frame_messages.h:23: // renderer, the message is forwarded to the
top level frame renderer to
On 2013/12/20 17:21:05, nasko wrote:
> Is it really the top level or the parent? I thought short term it was sending
to
> parent process.

Erp...I think this is a naming collosion. "frame renderer" here I think means
"compositing renderer" not "renderer process".

Reworde3d.

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_me...
content/common/frame_messages.h:28:
IPC_MESSAGE_ROUTED1(FrameMsg_CompositorFrameSwapped,
On 2013/12/20 17:21:05, nasko wrote:
> nit: It will be nice to have a comment on this IPC, at least to describe how
it
> differs from the above.

Done.

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
File content/common/frame_param.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
content/common/frame_param.h:1: // Copyright (c) 2013 The Chromium Authors. All
rights reserved.
On 2013/12/20 17:21:05, nasko wrote:
> nit: (c) is no longer required.

Done.

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
File content/common/frame_param_macros.h (right):

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
content/common/frame_param_macros.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/12/20 17:21:05, nasko wrote:
> nit: 2013

Done.

https://codereview.chromium.org/100473010/diff/350001/content/common/frame_pa...
content/common/frame_param_macros.h:8: // the RenderFrame infrastructure.
On 2013/12/20 17:21:05, nasko wrote:
> nit: Maybe a TODO for easy searching later on?

Good idea. Also created a bug. Done.

https://codereview.chromium.org/100473010/diff/350001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin_compositing_helper.cc
(right):

https://codereview.chromium.org/100473010/diff/350001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin_compositing_helper.cc:192:
params.producing_host_id = last_host_id_;
On 2013/12/20 17:21:05, nasko wrote:
> Is this intentional that use last_host_id_ here and it used to be
> mailbox.route_id? In all the cases above it is just moving the same params
into
> the IPC struct, but here it differs.

Whoa. Nice catch!

No, that's a mistake. Fixed.

[nasko, 2013-12-21 00:39:38]

LGTM with just one nit.

https://codereview.chromium.org/100473010/diff/430001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/100473010/diff/430001/content/common/frame_me...
content/common/frame_messages.h:28: // This is used in the non-ubercompe HW
accelerated compositing path.
nit: Extra e in ubercomp.

https://codereview.chromium.org/100473010/diff/430001/content/common/frame_me...
content/common/frame_messages.h:37: // This is used in the ubercomp compositing
path.
Kudos on add the extra clarification on which path is using which message!

[commit-bot: I haz the power, 2014-01-02 21:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/100473010/520001

[awong, 2014-01-02 21:47:42]

On 2014/01/02 21:43:57, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/kenrb%40chromium.org/100473010/520001

FYI, committing now TBR=sadrul.

This change has been reviewed by 3 people now, so it should be solid enough to
check in. Waiting longer costs more in productivity blocked than the safety
gained by following the word of the review procedure.

If there are further concerns, I will fix in followup CLs.

[commit-bot: I haz the power, 2014-01-02 21:49:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/100473010/640001

[commit-bot: I haz the power, 2014-01-02 22:07:37]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-03 00:11:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/100473010/830001

[commit-bot: I haz the power, 2014-01-03 01:02:09]

Retried try job too often on linux_rel for step(s) chromedriver2_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-03 02:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/100473010/830001

[commit-bot: I haz the power, 2014-01-03 03:12:22]

Retried try job too often on linux_rel for step(s) chromedriver2_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-03 03:16:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/100473010/830001

[commit-bot: I haz the power, 2014-01-03 03:57:39]

Retried try job too often on linux_rel for step(s) chromedriver2_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[sadrul, 2014-01-03 15:22:09]

Sorry about the delay. LGTM

https://codereview.chromium.org/100473010/diff/310001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/100473010/diff/310001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:30:
RenderWidgetHostViewChildFrame(RenderWidgetHost* widget);
explicit

https://codereview.chromium.org/100473010/diff/830001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/100473010/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:30:
RenderWidgetHostViewChildFrame(RenderWidgetHost* widget);
explicit?

[awong, 2014-01-03 19:47:06]

Thanks for the review!

Explicitly added.

On 2014/01/03 15:22:09, sadrul wrote:
> Sorry about the delay. LGTM
> 
>
https://codereview.chromium.org/100473010/diff/310001/content/browser/frame_h...
> File content/browser/frame_host/render_widget_host_view_child_frame.h (right):
> 
>
https://codereview.chromium.org/100473010/diff/310001/content/browser/frame_h...
> content/browser/frame_host/render_widget_host_view_child_frame.h:30:
> RenderWidgetHostViewChildFrame(RenderWidgetHost* widget);
> explicit
> 
>
https://codereview.chromium.org/100473010/diff/830001/content/browser/frame_h...
> File content/browser/frame_host/render_widget_host_view_child_frame.h (right):
> 
>
https://codereview.chromium.org/100473010/diff/830001/content/browser/frame_h...
> content/browser/frame_host/render_widget_host_view_child_frame.h:30:
> RenderWidgetHostViewChildFrame(RenderWidgetHost* widget);
> explicit?

[awong, 2014-01-03 19:54:32]

https://codereview.chromium.org/100473010/diff/310001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/100473010/diff/310001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:30:
RenderWidgetHostViewChildFrame(RenderWidgetHost* widget);
On 2014/01/03 15:22:10, sadrul wrote:
> explicit

Done.

https://codereview.chromium.org/100473010/diff/830001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.h (right):

https://codereview.chromium.org/100473010/diff/830001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.h:30:
RenderWidgetHostViewChildFrame(RenderWidgetHost* widget);
On 2014/01/03 15:22:10, sadrul wrote:
> explicit?

Done.

[commit-bot: I haz the power, 2014-01-03 19:54:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kenrb@chromium.org/100473010/1200001

[commit-bot: I haz the power, 2014-01-03 22:21:57]

Change committed as 242942