Add histogram for number of out-of-process iframes.

Add histogram for number of out-of-process iframes.

BUG=542921
Committed: https://crrev.com/aab1a442b5e4998288db81bd6751ac5e94cc8e7c
Cr-Commit-Position: refs/heads/master@{#360484}

[nasko, 2015-11-17 21:59:30]

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

[nasko, 2015-11-17 21:59:31]

Hey Nick,
Can you review this CL for me? It adds just one histogram - number of
out-of-process iframes.
Thanks in advance!
Nasko

[ncarter (slow), 2015-11-18 17:48:14]

https://codereview.chromium.org/1453193004/diff/20001/chrome/browser/site_det...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1453193004/diff/20001/chrome/browser/site_det...
chrome/browser/site_details.cc:188: num_oopifs +=
i->second.out_of_process_frames.size();
It doesn't look like you ever actually iterate over this set, and I don't see a
need to de-dupe RFH's since each FrameTree is visited exactly once. I think it
could just be an int counter?

https://codereview.chromium.org/1453193004/diff/20001/chrome/browser/site_det...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1453193004/diff/20001/chrome/browser/site_det...
chrome/browser/site_details_browsertest.cc:513:
EXPECT_THAT(details->GetOutOfProcessIframeCount(), EqualsIfSitePerProcess(0));
replace both of these with EXPECT_THAT's with

EXPECT_EQ(details->GetOutOfProcessIframeCount(), 0);

That'll provide some sanity coverage for the behavior in non-oopif.

https://codereview.chromium.org/1453193004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1453193004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:44099: +    The count of all
out-of-process iframes.  Recorded once per UMA ping.
They could be frameset frames, but I'm okay with the simplification, since
"oopif" is well understood and the meaning of "iframe" is unambiguous.

[nasko, 2015-11-18 18:32:58]

https://codereview.chromium.org/1453193004/diff/20001/chrome/browser/site_det...
File chrome/browser/site_details.cc (right):

https://codereview.chromium.org/1453193004/diff/20001/chrome/browser/site_det...
chrome/browser/site_details.cc:188: num_oopifs +=
i->second.out_of_process_frames.size();
On 2015/11/18 17:48:14, ncarter wrote:
> It doesn't look like you ever actually iterate over this set, and I don't see
a
> need to de-dupe RFH's since each FrameTree is visited exactly once. I think it
> could just be an int counter?

Done.

https://codereview.chromium.org/1453193004/diff/20001/chrome/browser/site_det...
File chrome/browser/site_details_browsertest.cc (right):

https://codereview.chromium.org/1453193004/diff/20001/chrome/browser/site_det...
chrome/browser/site_details_browsertest.cc:513:
EXPECT_THAT(details->GetOutOfProcessIframeCount(), EqualsIfSitePerProcess(0));
On 2015/11/18 17:48:14, ncarter wrote:
> replace both of these with EXPECT_THAT's with
> 
> EXPECT_EQ(details->GetOutOfProcessIframeCount(), 0);
> 
> That'll provide some sanity coverage for the behavior in non-oopif.

Done.

https://codereview.chromium.org/1453193004/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1453193004/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:44099: +    The count of all
out-of-process iframes.  Recorded once per UMA ping.
On 2015/11/18 17:48:14, ncarter wrote:
> They could be frameset frames, but I'm okay with the simplification, since
> "oopif" is well understood and the meaning of "iframe" is unambiguous.

I've debated this one, but I think most people think in terms of iframes, so it
will be easier to understand and not be confused.

[ncarter (slow), 2015-11-18 19:02:35]

lgtm

thestig@ and jam@ have recently done site_details.cc reviews.

[ncarter (slow), 2015-11-18 19:02:37]

lgtm

thestig@ and jam@ have recently done site_details.cc reviews.

[nasko, 2015-11-18 19:09:21]

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

[nasko, 2015-11-18 19:09:22]

Hey Lei,
Can you do an OWNERS review for chrome/?
Thanks!
Nasko

[Lei Zhang, 2015-11-18 19:31:45]

lgtm

[nasko, 2015-11-18 22:52:03]

The CQ bit was checked by nasko@chromium.org

[nasko, 2015-11-18 22:52:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1453193004/#ps60001
(title: "Initialize frames count in constructor.")

[commit-bot: I haz the power, 2015-11-18 22:53:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453193004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453193004/60001

[nasko, 2015-11-18 22:53:21]

The CQ bit was unchecked by nasko@chromium.org

[nasko, 2015-11-18 22:54:05]

isherman@ - can you do OWNERS review of tools/metrics?

[nasko, 2015-11-19 00:14:31]

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

[nasko, 2015-11-19 00:14:32]

Adding isherman@ for real this time.

[Ilya Sherman, 2015-11-19 00:55:04]

LGTM

[nasko, 2015-11-19 00:55:35]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-11-19 00:59:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1453193004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1453193004/60001

[commit-bot: I haz the power, 2015-11-19 02:10:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-19 02:12:21]

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