Add UMA stats for predicted process counts with out-of-process iframes.

Add UMA stats for predicted process counts with out-of-process iframes.

Requires adding the committed URL to each FrameTreeNode and exposing the
ShouldUseProcessPerSite function outside content.

BUG=248299
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206286

[Charlie Reis, 2013-06-11 01:03:20]

Nasko, can you take a first look over this?

[nasko, 2013-06-11 17:48:32]

As we discussed, let's add the number of BrowsingInstances to the data, so we
have better understanding of how SiteInstances are distributed.

[Charlie Reis, 2013-06-11 19:11:40]

On 2013/06/11 17:48:32, nasko wrote:
> As we discussed, let's add the number of BrowsingInstances to the data, so we
> have better understanding of how SiteInstances are distributed.

Good idea.  Fixed.

[nasko, 2013-06-11 20:16:37]

Couple of nits and a question.

https://codereview.chromium.org/16599016/diff/11001/chrome/browser/memory_det...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/16599016/diff/11001/chrome/browser/memory_det...
chrome/browser/memory_details.cc:406: iter != sites_in_tab.end(); ++iter) {
nit: Shouldn't the increment part of the loop be on a new line? The same pattern
is in use below.

https://codereview.chromium.org/16599016/diff/11001/chrome/browser/memory_det...
chrome/browser/memory_details.cc:408: if (iter->is_empty())
Is "about:blank" an actual empty GURL? I thought we always commit "about:blank"
and it is an explicit GURL.

https://codereview.chromium.org/16599016/diff/11001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/16599016/diff/11001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:2972: if (node)
nit: node shouldn't be NULL, right?

[Charlie Reis, 2013-06-11 21:36:39]

https://codereview.chromium.org/16599016/diff/11001/chrome/browser/memory_det...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/16599016/diff/11001/chrome/browser/memory_det...
chrome/browser/memory_details.cc:406: iter != sites_in_tab.end(); ++iter) {
On 2013/06/11 20:16:37, nasko wrote:
> nit: Shouldn't the increment part of the loop be on a new line? The same
pattern
> is in use below.

No, this looks like a common pattern.  You only need to do one
parameter-per-line for things like function declarations.  It's fine to combine
the test and iterate parts of a for loop on the same line if there's space.

https://codereview.chromium.org/16599016/diff/11001/chrome/browser/memory_det...
chrome/browser/memory_details.cc:408: if (iter->is_empty())
On 2013/06/11 20:16:37, nasko wrote:
> Is "about:blank" an actual empty GURL? I thought we always commit
"about:blank"
> and it is an explicit GURL.

It comes from GetSiteForURL, which has the comment "// URLs with no host should
have an empty site."  Granted, that does mean that this code will ignore any URL
without a host, but I'm not sure there are others that matter here.  I've
updated the comment.

https://codereview.chromium.org/16599016/diff/11001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/16599016/diff/11001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:2972: if (node)
On 2013/06/11 20:16:37, nasko wrote:
> nit: node shouldn't be NULL, right?

I thought not, but I was wrong.  I added a DCHECK and it fails for pages with
about:blank iframes.  I filed http://crbug.com/248699 for it.

I'll keep the "if" here for now.  This shouldn't be much of an issue for the
stats, because we skip about:blank pages when counting anyway (since they don't
get a process of their own).

[nasko, 2013-06-11 21:46:39]

lgtm

[Charlie Reis, 2013-06-11 21:51:55]

Darin, can you review for owners approval?

[darin (slow to review), 2013-06-12 04:40:48]

LGTM

https://codereview.chromium.org/16599016/diff/21001/chrome/browser/memory_det...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/16599016/diff/21001/chrome/browser/memory_det...
chrome/browser/memory_details.cc:378: void
MemoryDetails::CollectSiteInfo(WebContents* contents) {
nit: It is perhaps expanding the scope of this class beyond "memory details" to
add collection of site info.  Would it work for this to be separated into some
independent class / file?

https://codereview.chromium.org/16599016/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16599016/diff/21001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1544: if
(WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
By the way, perhaps it would be better to perform this test before asking the
ContentBrowserClient?  It seems like this second check could override what the
ContentBrowserClient indicates, which could be confusing.  Might as well not
even bother consulting ContentBrowserClient in the cases where we are going to
ignore what it has to say.

https://codereview.chromium.org/16599016/diff/21001/content/browser/web_conte...
File content/browser/web_contents/frame_tree_node.h (right):

https://codereview.chromium.org/16599016/diff/21001/content/browser/web_conte...
content/browser/web_contents/frame_tree_node.h:65: // Temporarily track the
current frame's last committed URL, so we can
nit: Maybe leave off "Temporarily" or add a TODO?

[Charlie Reis, 2013-06-12 18:59:16]

Thanks, PTAL.

https://codereview.chromium.org/16599016/diff/21001/chrome/browser/memory_det...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/16599016/diff/21001/chrome/browser/memory_det...
chrome/browser/memory_details.cc:378: void
MemoryDetails::CollectSiteInfo(WebContents* contents) {
On 2013/06/12 04:40:48, darin wrote:
> nit: It is perhaps expanding the scope of this class beyond "memory details"
to
> add collection of site info.  Would it work for this to be separated into some
> independent class / file?

It's kind of intermediate data to report process counts similar to what this
file is doing, and I'd like to keep it tied in with the timing and thread hops
we're doing here to collect the data.  However, I agree that site data is a bit
different, so I moved it out to another class called from here.

https://codereview.chromium.org/16599016/diff/21001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16599016/diff/21001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1544: if
(WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
On 2013/06/12 04:40:48, darin wrote:
> By the way, perhaps it would be better to perform this test before asking the
> ContentBrowserClient?  It seems like this second check could override what the
> ContentBrowserClient indicates, which could be confusing.  Might as well not
> even bother consulting ContentBrowserClient in the cases where we are going to
> ignore what it has to say.

Good point.  I'll throw together a small separate CL to fix that.

https://codereview.chromium.org/16599016/diff/21001/content/browser/web_conte...
File content/browser/web_contents/frame_tree_node.h (right):

https://codereview.chromium.org/16599016/diff/21001/content/browser/web_conte...
content/browser/web_contents/frame_tree_node.h:65: // Temporarily track the
current frame's last committed URL, so we can
On 2013/06/12 04:40:48, darin wrote:
> nit: Maybe leave off "Temporarily" or add a TODO?

Done.

[darin (slow to review), 2013-06-12 19:27:51]

LGTM

[commit-bot: I haz the power, 2013-06-12 20:39:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/16599016/33001

[commit-bot: I haz the power, 2013-06-12 23:10:57]

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

[commit-bot: I haz the power, 2013-06-12 23:13:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/16599016/33001

[commit-bot: I haz the power, 2013-06-13 02:56:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/16599016/33001

[commit-bot: I haz the power, 2013-06-13 04:57:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-13 23:26:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/16599016/33001

[commit-bot: I haz the power, 2013-06-14 03:38:58]

Change committed as 206286