[DevTools] Move DevToolsAgent to RenderFrameImpl.

[DevTools] Move DevToolsAgent to RenderFrameImpl.

BUG=451004
Committed: https://crrev.com/358ba325dad3cc11d1f330f652e655f38c12249b
Cr-Commit-Position: refs/heads/master@{#322382}

[dgozman, 2015-03-24 13:50:32]

dgozman@chromium.org changed reviewers:
+ creis@chromium.org, nasko@chromium.org, pfeldman@chromium.org

[dgozman, 2015-03-24 13:50:33]

Hi,

Could you please take a look? This patch makes DevTools for cross-process
subframe work. It still fails for some cross-process navigations, and does not
have overlay, but I will address those in follow-ups.

I didn't find a way to:
- check whether RenderFrameImpl is a local frame root at initialization time;
- get to RenderFrameImpl for local frame root from the child.
Should we add this functionality? I've added TODOs for now.

Thanks,
Dmitry

[dgozman, 2015-03-24 13:54:30]

I'm also interested in how to test this new path. It would be ideal to run all
inspector layout tests when the target is cross-process iframe. Do you already
support that?

[pfeldman, 2015-03-24 13:54:33]

lgtm

[nasko, 2015-03-24 16:17:07]

My main concern is that currently the code assumes local frame root in a bunch
of places.

https://codereview.chromium.org/1023783004/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1023783004/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:608: if (proxy_routing_id ==
MSG_ROUTING_NONE)
While this might work, it is a very fragile way to write the code. I'm looking
at the right way to expose the "local frame root" concept and I think we should
land a CL that gives such method prior to this CL.

[Charlie Reis, 2015-03-24 17:25:13]

[+site-isolation-reviews]

Great.  Just a few nits, and I agree with Nasko's comment.  We'll see if we can
get something added for this.

https://codereview.chromium.org/1023783004/diff/1/content/renderer/devtools/d...
File content/renderer/devtools/devtools_agent.h (right):

https://codereview.chromium.org/1023783004/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_agent.h:22: // DevToolsAgent belongs to the
inspectable RenderView and provides Glue's
Let's update this comment.

https://codereview.chromium.org/1023783004/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_agent.h:47: // RenderView::Observer
implementation.
nit: Update comment.

https://codereview.chromium.org/1023783004/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1023783004/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:608: if (proxy_routing_id ==
MSG_ROUTING_NONE)
On 2015/03/24 16:17:07, nasko wrote:
> While this might work, it is a very fragile way to write the code. I'm looking
> at the right way to expose the "local frame root" concept and I think we
should
> land a CL that gives such method prior to this CL.

+1

For comparison, we have a IsCrossProcessSubframe in RenderFrameHostImpl.  I'm ok
with adding something similar, like a IsLocalFrameRoot here or in WebFrame.

https://codereview.chromium.org/1023783004/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1023783004/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:859: // The devtools agent for this frame,
only created for local frame roots.
nit: period or semicolon, not comma.

[dgozman, 2015-03-25 16:25:50]

Pleas take another look.

https://codereview.chromium.org/1023783004/diff/1/content/renderer/devtools/d...
File content/renderer/devtools/devtools_agent.h (right):

https://codereview.chromium.org/1023783004/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_agent.h:22: // DevToolsAgent belongs to the
inspectable RenderView and provides Glue's
On 2015/03/24 17:25:13, Charlie Reis wrote:
> Let's update this comment.

Done.

https://codereview.chromium.org/1023783004/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_agent.h:47: // RenderView::Observer
implementation.
On 2015/03/24 17:25:13, Charlie Reis wrote:
> nit: Update comment.

Done.

https://codereview.chromium.org/1023783004/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1023783004/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.h:859: // The devtools agent for this frame,
only created for local frame roots.
On 2015/03/24 17:25:13, Charlie Reis wrote:
> nit: period or semicolon, not comma.

Done.

https://codereview.chromium.org/1023783004/diff/20001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1023783004/diff/20001/content/renderer/render...
content/renderer/render_view_impl.cc:758: main_render_frame_->Initialize();
This causes crash for me. See comment above. Any ideas?

[dgozman, 2015-03-25 17:35:05]

PTAL, this is ready now.

[nasko, 2015-03-25 22:26:05]

The CL on its own looks ok, but I'm not sure whether the goal is to just move
the code or to also make DevTools work with cross-process subframes. As it is,
it doesn't accomplish the latter part I think.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/devtoo...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/devtoo...
content/renderer/devtools/devtools_agent.cc:261: // We only want to register
once per render view.
nit: Is this comment still valid? In my mind there can be multiple
DevToolsAgents now in a single process for the same page.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:736: is_local_root_ = !frame_->parent() ||
frame_->parent()->isWebRemoteFrame();
I'd put this initialization line as the first thing in this method.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3231: // TODO(dgozman): we should check
DevToolsAgent of the local frame root.
Why not include this check in this CL? I'm worried about this working
inconsistently based on the configuration of the page.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.h:744: // in the same process.
This comment is not quite correct. It is a local frame, which is rendered in a
process different than its parent. The subframes don't need to be all local,
they can be mix of remote and local.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.h:854: // The devtools agent for this frame;
only created for local frame roots.
nit: the frame is a local root, not that the local frame is root.

[dgozman, 2015-03-26 05:41:28]

On 2015/03/25 22:26:05, nasko wrote:
> The CL on its own looks ok, but I'm not sure whether the goal is to just move
> the code or to also make DevTools work with cross-process subframes. As it is,
> it doesn't accomplish the latter part I think.

Why do you think it doesn't work with cross-process subframes? Do you see some
subtle problems?
Because it generally works. I was not able to run the tests, but as far as I can
confirm manually, nothing is broken except for expected things.

[nasko, 2015-03-26 06:15:03]

On 2015/03/26 05:41:28, dgozman wrote:
> On 2015/03/25 22:26:05, nasko wrote:
> > The CL on its own looks ok, but I'm not sure whether the goal is to just
move
> > the code or to also make DevTools work with cross-process subframes. As it
is,
> > it doesn't accomplish the latter part I think.
> 
> Why do you think it doesn't work with cross-process subframes? Do you see some
> subtle problems?
> Because it generally works. I was not able to run the tests, but as far as I
can
> confirm manually, nothing is broken except for expected things.

GetWebDevToolsAgent in render_view_impl.cc is not going to work for the process
hosting the subframe, so I'm suspicious that the rendering/compositor will get
hooked up properly. Also, RenderFrameImpl::didFinishResourceLoad relies on the
fact that there is a dev tools agent, where it should refer to the local root
frame for it, right?

If it works generally, I think it is worth proceeding with the CL, but let's
note these items to ensure we don't lose track of them.

[pfeldman, 2015-03-26 06:30:12]

@dgozman: could you share the design doc with this thread? knoing the
expectations for the v1 would help.

[nasko, 2015-03-26 06:35:46]

On 2015/03/26 06:30:12, pfeldman_ooo_mar_31 wrote:
> @dgozman: could you share the design doc with this thread? knoing the
> expectations for the v1 would help.

I do see you've explicitly called out no overlays, so I assume this makes the
code in RenderViewImpl fine. Until the chat today, I wasn't sure what that
meant, but I do now. Thanks!

LGTM

[dgozman, 2015-03-26 13:25:49]

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/devtoo...
File content/renderer/devtools/devtools_agent.cc (right):

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/devtoo...
content/renderer/devtools/devtools_agent.cc:261: // We only want to register
once per render view.
On 2015/03/25 22:26:04, nasko wrote:
> nit: Is this comment still valid? In my mind there can be multiple
> DevToolsAgents now in a single process for the same page.

Yes. This is for DevTools frontend. DCHECK above ensures this is only called for
main frame.

Changed the comment to not mention render view.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:736: is_local_root_ = !frame_->parent() ||
frame_->parent()->isWebRemoteFrame();
On 2015/03/25 22:26:04, nasko wrote:
> I'd put this initialization line as the first thing in this method.

Done.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3231: // TODO(dgozman): we should check
DevToolsAgent of the local frame root.
On 2015/03/25 22:26:04, nasko wrote:
> Why not include this check in this CL? I'm worried about this working
> inconsistently based on the configuration of the page.

Done.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.h:744: // in the same process.
On 2015/03/25 22:26:04, nasko wrote:
> This comment is not quite correct. It is a local frame, which is rendered in a
> process different than its parent. The subframes don't need to be all local,
> they can be mix of remote and local.

Updated comment. I also renamed this to |is_main_or_local_root_| to be aligned
with blink's counterpart, which does not count main frame as a local root.

https://codereview.chromium.org/1023783004/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.h:854: // The devtools agent for this frame;
only created for local frame roots.
On 2015/03/25 22:26:04, nasko wrote:
> nit: the frame is a local root, not that the local frame is root.

Done.

[dgozman, 2015-03-26 13:26:02]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2015-03-26 13:26:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1023783004/#ps80001
(title: "Fixed more comments")

[commit-bot: I haz the power, 2015-03-26 13:26:14]

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

[dgozman, 2015-03-26 13:40:13]

The CQ bit was unchecked by dgozman@chromium.org

[dgozman, 2015-03-26 14:03:23]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2015-03-26 14:03:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1023783004/#ps100001
(title: "is_main_or_local_root_ -> is_local_root_")

[commit-bot: I haz the power, 2015-03-26 14:03:38]

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

[commit-bot: I haz the power, 2015-03-26 15:05:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-26 15:06:36]

Patchset 6 (id:??) landed as
https://crrev.com/358ba325dad3cc11d1f330f652e655f38c12249b
Cr-Commit-Position: refs/heads/master@{#322382}