Move memory histograms generation to MetricsMemoryDetails.

Move memory histograms generation to MetricsMemoryDetails.

Also moves MetricsMemoryDetails (and MemoryGrowthTracker) to its
own file as well as moving the corresponding browser test.

This allows the base MemoryDetails class to be simplified, as it now
needs to do a lot less. Also, disables logging of these histograms
from a couple places that use the MemoryDetails that didn't need to
log the histograms but were doing it anyways (with TODOs).

BUG=448834
Committed: https://crrev.com/89406d1fbe69fc8a87e30315b17c2a76aac33517
Cr-Commit-Position: refs/heads/master@{#312026}

[Alexei Svitkine (slow), 2015-01-15 20:39:37]

Patchset #2 (id:20001) has been deleted

[Alexei Svitkine (slow), 2015-01-15 20:42:49]

Patchset #2 (id:40001) has been deleted

[Alexei Svitkine (slow), 2015-01-15 20:43:19]

Patchset #2 (id:60001) has been deleted

[Alexei Svitkine (slow), 2015-01-15 20:43:27]

Patchset #1 (id:1) has been deleted

[Alexei Svitkine (slow), 2015-01-15 20:44:36]

Patchset #1 (id:80001) has been deleted

[Alexei Svitkine (slow), 2015-01-15 20:44:46]

Patchset #1 (id:100001) has been deleted

[Alexei Svitkine (slow), 2015-01-15 20:55:42]

asvitkine@chromium.org changed reviewers:
+ isherman@chromium.org, jamescook@chromium.org

[Alexei Svitkine (slow), 2015-01-15 20:55:43]

PTAL

isherman: overall review
jamescook: take a look at the TODO(jamescook)'s I'm addressing

[James Cook, 2015-01-15 21:13:24]

OK, so this looks like it stops reporting memory statistics in two cases:

* When the user sends a feedback report
* When the user navigates to about:memory

I think I preserved those cases when I added the do/don't flag out of paranoia
for maintaining the existing histogram behavior.  I'm OK with removing them -- I
doubt they make much difference in the overall stats results.

If that's OK with you too, LGTM

[Ilya Sherman, 2015-01-16 04:57:36]

Are you 100% sure that there's no need to update the various memory histograms
in the two places that you identified?  I'd recommend double-checking with
OWNERS of the appropriate code, in case they're relying on the histograms being
updated somehow.

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/feedback/...
File
chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.cc
(right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/feedback/...
chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.cc:48:
handler->StartFetch();
Are you sure that updating the histograms is not important here?  I know that
feedback reports include some memory statistics.  Are those based on something
other than the histograms?

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/memory_de...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/memory_de...
chrome/browser/memory_details.cc:8: #include <set>
nit: Are these just fixing pre-existing IWYU violations?

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_memory_details.cc (right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_memory_details.cc:150: }
nit: The placement of this curly brace looks really off.  I think the best thing
to do is just to move the three lines below this one up into the nested scope.

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_memory_details.h (right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_memory_details.h:20: // If 30 minutes have passed
since last UMA record, UpdateSample() computes
Why 30 minutes?  Is it too expensive to compute once per UMA upload?

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_memory_details.h:65: // If it is NULL, nothing is
tracked.
Please update this comment -- there's no longer a SetMemoryGrowthTracker()
method.

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_memory_details_browsertest.cc:15: }
FWIW, this is also defined in base/bind_helpers.h.

[Alexei Svitkine (slow), 2015-01-16 16:06:01]

Patchset #3 (id:150001) has been deleted

[Alexei Svitkine (slow), 2015-01-16 16:07:50]

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/feedback/...
File
chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.cc
(right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/feedback/...
chrome/browser/feedback/system_logs/log_sources/memory_details_log_source.cc:48:
handler->StartFetch();
On 2015/01/16 04:57:36, Ilya Sherman wrote:
> Are you sure that updating the histograms is not important here?  I know that
> feedback reports include some memory statistics.  Are those based on something
> other than the histograms?

Yes, see line 24 in this file - ToLogString() is what is actually being reported
- which comes from the base class and prints the various memory metrics as a
string.

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/memory_de...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/memory_de...
chrome/browser/memory_details.cc:8: #include <set>
On 2015/01/16 04:57:36, Ilya Sherman wrote:
> nit: Are these just fixing pre-existing IWYU violations?

Yep, as suggested by git cl lint.

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_memory_details.cc (right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_memory_details.cc:150: }
On 2015/01/16 04:57:36, Ilya Sherman wrote:
> nit: The placement of this curly brace looks really off.  I think the best
thing
> to do is just to move the three lines below this one up into the nested scope.

Indeed - I guess clang format got confused here. Fixed.

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_memory_details.h (right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_memory_details.h:20: // If 30 minutes have passed
since last UMA record, UpdateSample() computes
On 2015/01/16 04:57:36, Ilya Sherman wrote:
> Why 30 minutes?  Is it too expensive to compute once per UMA upload?

I'm not changing this - this is just moving from the other file.

This class is a helper to support the Memory.RendererShrinkIn30Min and
Memory.RendererGrowthIn30Min histograms.

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_memory_details.h:65: // If it is NULL, nothing is
tracked.
On 2015/01/16 04:57:36, Ilya Sherman wrote:
> Please update this comment -- there's no longer a SetMemoryGrowthTracker()
> method.

Done.

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_memory_details_browsertest.cc (right):

https://codereview.chromium.org/823533004/diff/50016/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_memory_details_browsertest.cc:15: }
On 2015/01/16 04:57:36, Ilya Sherman wrote:
> FWIW, this is also defined in base/bind_helpers.h.

Done.

[Ilya Sherman, 2015-01-17 00:18:43]

LGTM, thanks.

[Alexei Svitkine (slow), 2015-01-17 00:41:56]

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

[Alexei Svitkine (slow), 2015-01-17 00:41:57]

+thestig for owners review for chrome/

[Lei Zhang, 2015-01-17 01:41:53]

lgtm

https://codereview.chromium.org/823533004/diff/170001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_memory_details.h (right):

https://codereview.chromium.org/823533004/diff/170001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_memory_details.h:40: // Will run the provided
task after finished.
nit: task -> callback, "after finished" -> "after finishing" or "when finished"

[Alexei Svitkine (slow), 2015-01-17 06:06:21]

https://codereview.chromium.org/823533004/diff/170001/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_memory_details.h (right):

https://codereview.chromium.org/823533004/diff/170001/chrome/browser/metrics/...
chrome/browser/metrics/metrics_memory_details.h:40: // Will run the provided
task after finished.
On 2015/01/17 01:41:53, Lei Zhang wrote:
> nit: task -> callback, "after finished" -> "after finishing" or "when
finished"

Done.

[Alexei Svitkine (slow), 2015-01-17 06:06:52]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2015-01-17 06:07:56]

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

[commit-bot: I haz the power, 2015-01-17 06:57:37]

Committed patchset #4 (id:190001)

[commit-bot: I haz the power, 2015-01-17 06:58:45]

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