[DevTools] Expose DevToolsAgentHost for OOPIF, display it in chrome://inspect.

[DevTools] Expose DevToolsAgentHost for OOPIF, display it in chrome://inspect.

BUG=451004, 464993
Committed: https://crrev.com/ea764811d0f4a41ef8bb9768737bc6f7dde6f27f
Cr-Commit-Position: refs/heads/master@{#319613}

[dgozman, 2015-03-05 14:23:40]

dgozman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[dgozman, 2015-03-05 14:23:41]

Take a look please.

[pfeldman, 2015-03-05 14:57:13]

lgtm

[dgozman, 2015-03-05 15:08:48]

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

[dgozman, 2015-03-05 15:08:49]

@creis, @nasko: take a look at frame_host and public please.

[nasko, 2015-03-05 15:20:48]

Just one minor thing. I'd like to get Charlie's take on the method name, as
those types of names have long lasting effect and we should make sure we get
them right.

https://codereview.chromium.org/976313003/diff/1/content/browser/devtools/ren...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/976313003/diff/1/content/browser/devtools/ren...
content/browser/devtools/render_frame_devtools_agent_host.cc:465: return
render_frame_host_->GetLastCommittedURL();
Why not return this in all cases?

https://codereview.chromium.org/976313003/diff/1/content/browser/devtools/ren...
content/browser/devtools/render_frame_devtools_agent_host.cc:467: return
web_contents->GetVisibleURL();
This is a bit orthogonal, but why do you need the visible URL? In my mind,
DevTools always operates on a document that exists. The visible URL can be an
URL that isn't committed yet, so no real document exist associated with it.

https://codereview.chromium.org/976313003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/976313003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:214: bool IsFrameGroupRoot()
{ return GetRenderWidgetHost(); }
non-trivial method's implementations need to be in the .cc file. Also,
GetRenderWidgetHost() returns a pointer type, use "!!" to explicitly convert to
boolean.

[Charlie Reis, 2015-03-05 20:31:15]

Great, thanks!  We probably don't need IsFrameGroupRoot, plus a few nits. 
Otherwise this looks nice.

(One minor request: it helps to CC site-isolation-reviews@chromium.org on
OOPIF-related CLs, which helps our team keep track of the efforts in progress.)

https://codereview.chromium.org/976313003/diff/1/chrome/browser/devtools/devt...
File chrome/browser/devtools/devtools_target_impl.cc (right):

https://codereview.chromium.org/976313003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_target_impl.cc:67:
web_contents->GetRenderViewHost()->GetMainFrame();
Please use web_contents->GetMainFrame().

https://codereview.chromium.org/976313003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_target_impl.cc:67:
web_contents->GetRenderViewHost()->GetMainFrame();
Please use web_contents->GetMainFrame().

https://codereview.chromium.org/976313003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_target_impl.cc:73:
WebContents::FromRenderViewHost(parent_rvh))->GetId());
Please use WebContents::FromRenderFrameHost(rfh->GetParent()) instead, so that
we don't use RenderViewHost at all here.

https://codereview.chromium.org/976313003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/976313003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:212: // Returns whether this
frame is the root of the frame group. Frame group
This is redundant with IsCrossProcessSubframe, so we should probably just use
that instead.  (IsCrossProcessSubframe returns whether a frame is in a different
process than its parent frame, which is exactly the case that it has its own
widget and is the "root of a new frame group.")

[Charlie Reis, 2015-03-05 20:31:43]

[oops, +site-isolation-reviews@chromium.org for real this time]

[dgozman, 2015-03-06 16:14:14]

Please take another look.

https://codereview.chromium.org/976313003/diff/1/chrome/browser/devtools/devt...
File chrome/browser/devtools/devtools_target_impl.cc (right):

https://codereview.chromium.org/976313003/diff/1/chrome/browser/devtools/devt...
chrome/browser/devtools/devtools_target_impl.cc:67:
web_contents->GetRenderViewHost()->GetMainFrame();
On 2015/03/05 20:31:14, Charlie Reis wrote:
> Please use web_contents->GetMainFrame().

I removed this code. Main frame can never be a cross process subframe.

https://codereview.chromium.org/976313003/diff/1/content/browser/devtools/ren...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/976313003/diff/1/content/browser/devtools/ren...
content/browser/devtools/render_frame_devtools_agent_host.cc:465: return
render_frame_host_->GetLastCommittedURL();
On 2015/03/05 15:20:48, nasko wrote:
> Why not return this in all cases?

Done.

https://codereview.chromium.org/976313003/diff/1/content/browser/devtools/ren...
content/browser/devtools/render_frame_devtools_agent_host.cc:467: return
web_contents->GetVisibleURL();
On 2015/03/05 15:20:48, nasko wrote:
> This is a bit orthogonal, but why do you need the visible URL? In my mind,
> DevTools always operates on a document that exists. The visible URL can be an
> URL that isn't committed yet, so no real document exist associated with it.

You are right, using last committed url.

https://codereview.chromium.org/976313003/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/976313003/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.h:212: // Returns whether this
frame is the root of the frame group. Frame group
On 2015/03/05 20:31:14, Charlie Reis wrote:
> This is redundant with IsCrossProcessSubframe, so we should probably just use
> that instead.  (IsCrossProcessSubframe returns whether a frame is in a
different
> process than its parent frame, which is exactly the case that it has its own
> widget and is the "root of a new frame group.")

Done.

[nasko, 2015-03-06 19:36:18]

This looks really nice! LGTM

[Charlie Reis, 2015-03-06 21:33:50]

Nice!

Are there any tests for opening DevTools?  We should have one for an OOPIF since
it would be easy to miss regressions that sneak in. 
(SitePerProcessBrowserTest.CrossSiteIframe can show you how to create an OOPIF
in a browser test with the --site-per-process flag.)

Having that in this CL would be ideal, but I'm ok with a followup CL for the
test.  Otherwise LGTM with nits.

https://codereview.chromium.org/976313003/diff/40001/content/browser/devtools...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/976313003/diff/40001/content/browser/devtools...
content/browser/devtools/render_frame_devtools_agent_host.cc:94:
DevToolsAgentHost::List* result, RenderFrameHost* host) {
Style nit: Each parameter on its own line if the method and parameters can't all
fit on one line.

https://codereview.chromium.org/976313003/diff/40001/content/browser/devtools...
content/browser/devtools/render_frame_devtools_agent_host.cc:421: // navigation.
Is there a bug number we can cite here?

[dgozman, 2015-03-07 14:13:01]

Added test.

https://codereview.chromium.org/976313003/diff/40001/content/browser/devtools...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/976313003/diff/40001/content/browser/devtools...
content/browser/devtools/render_frame_devtools_agent_host.cc:94:
DevToolsAgentHost::List* result, RenderFrameHost* host) {
On 2015/03/06 21:33:50, Charlie Reis wrote:
> Style nit: Each parameter on its own line if the method and parameters can't
all
> fit on one line.

Done.

https://codereview.chromium.org/976313003/diff/40001/content/browser/devtools...
content/browser/devtools/render_frame_devtools_agent_host.cc:421: // navigation.
On 2015/03/06 21:33:50, Charlie Reis wrote:
> Is there a bug number we can cite here?

Done.

https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
content/browser/devtools/render_frame_devtools_agent_host.cc:476: return
web_contents->GetVisibleURL();
I had to bring this back, since tests expect correct url before load has
finished.

[dgozman, 2015-03-09 12:16:03]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2015-03-09 12:16:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org, creis@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/976313003/#ps80001 (title:
"Disabled test on android")

[commit-bot: I haz the power, 2015-03-09 12:16:19]

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

[commit-bot: I haz the power, 2015-03-09 13:08:04]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-09 13:08:35]

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

[nasko, 2015-03-09 14:14:46]

https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
content/browser/devtools/render_frame_devtools_agent_host.cc:476: return
web_contents->GetVisibleURL();
On 2015/03/07 14:13:01, dgozman wrote:
> I had to bring this back, since tests expect correct url before load has
> finished.

Can you point me to which test is expecting this? Other than the network tab
collecting data for the pending navigation, I don't see any other reason
DevTools should be operating on the pending URL, so I'm curious where my
understanding is lacking.

[dgozman, 2015-03-09 14:16:46]

On 2015/03/09 14:14:46, nasko wrote:
>
https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
> File content/browser/devtools/render_frame_devtools_agent_host.cc (right):
> 
>
https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
> content/browser/devtools/render_frame_devtools_agent_host.cc:476: return
> web_contents->GetVisibleURL();
> On 2015/03/07 14:13:01, dgozman wrote:
> > I had to bring this back, since tests expect correct url before load has
> > finished.
> 
> Can you point me to which test is expecting this? Other than the network tab
> collecting data for the pending navigation, I don't see any other reason
> DevTools should be operating on the pending URL, so I'm curious where my
> understanding is lacking.

Here you go:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_....

We expect DevToolsTarget to report correct URL for just-created page. We can
probably fix that, but it's a separate non-trivial effort.

[nasko, 2015-03-09 14:18:51]

On 2015/03/09 14:16:46, dgozman wrote:
> On 2015/03/09 14:14:46, nasko wrote:
> >
>
https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
> > File content/browser/devtools/render_frame_devtools_agent_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
> > content/browser/devtools/render_frame_devtools_agent_host.cc:476: return
> > web_contents->GetVisibleURL();
> > On 2015/03/07 14:13:01, dgozman wrote:
> > > I had to bring this back, since tests expect correct url before load has
> > > finished.
> > 
> > Can you point me to which test is expecting this? Other than the network tab
> > collecting data for the pending navigation, I don't see any other reason
> > DevTools should be operating on the pending URL, so I'm curious where my
> > understanding is lacking.
> 
> Here you go:
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_....

Thanks!

> We expect DevToolsTarget to report correct URL for just-created page. We can
> probably fix that, but it's a separate non-trivial effort.

I didn't mean that we need to fix anything right now (especially this CL), I
want to fill gaps in my understanding.

[Charlie Reis, 2015-03-09 23:02:38]

Awesome.  Thanks for the test and still LGTM.  A few minor notes below for
future CLs.

https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
File content/browser/devtools/render_frame_devtools_agent_host.cc (right):

https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
content/browser/devtools/render_frame_devtools_agent_host.cc:476: return
web_contents->GetVisibleURL();
On 2015/03/09 14:16:46, dgozman wrote:
> On 2015/03/09 14:14:46, nasko wrote:
> >
>
https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
> > File content/browser/devtools/render_frame_devtools_agent_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/976313003/diff/60001/content/browser/devtools...
> > content/browser/devtools/render_frame_devtools_agent_host.cc:476: return
> > web_contents->GetVisibleURL();
> > On 2015/03/07 14:13:01, dgozman wrote:
> > > I had to bring this back, since tests expect correct url before load has
> > > finished.
> > 
> > Can you point me to which test is expecting this? Other than the network tab
> > collecting data for the pending navigation, I don't see any other reason
> > DevTools should be operating on the pending URL, so I'm curious where my
> > understanding is lacking.
> 
> Here you go:
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_....
> 
> We expect DevToolsTarget to report correct URL for just-created page. We can
> probably fix that, but it's a separate non-trivial effort.

Thanks for the test pointer.  Agreed that we should try to fix that in a future
CL, since tests shouldn't assume that the pending URL is the current one (or
even that it will necessarily commit).

https://codereview.chromium.org/976313003/diff/80001/content/browser/devtools...
File content/browser/devtools/site_per_process_devtools_browsertest.cc (right):

https://codereview.chromium.org/976313003/diff/80001/content/browser/devtools...
content/browser/devtools/site_per_process_devtools_browsertest.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
nit: 2015

https://codereview.chromium.org/976313003/diff/80001/content/browser/devtools...
content/browser/devtools/site_per_process_devtools_browsertest.cc:4: 
nit: Remove one of the two blank lines.