Extract guts of ChromeStabilityMetricsProvider into helper class

Extract guts of ChromeStabilityMetricsProvider into helper class

The iOS port needs to turn down its usage of ChromeStabilityMetricsProvider in
favor of using its own stability metrics provider. To minimize code duplication
between the two providers, this CL extracts the code that will be common
between the two into a shared helper class.

As a necessary dependence, this CL also componentizes
system_memory_stats_recorder*. Those files can be componentized as-is.

BUG=512422
Committed: https://crrev.com/8ae15621b008c8df6a71af4f6a5a3ff28b6131d0
Cr-Commit-Position: refs/heads/master@{#351036}

[blundell, 2015-09-23 20:01:53]

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

[blundell, 2015-09-24 07:16:47]

On 2015/09/23 20:01:54, blundell wrote:

(note that the failures are unrelated)

[blundell, 2015-09-24 07:19:22]

https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
File components/metrics/stability_metrics_helper.cc (right):

https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
components/metrics/stability_metrics_helper.cc:150:
base::RecordAction(base::UserMetricsAction("PageLoad"));
One question: I assume ChromeStabilityMetricsProvider is used only on the UI
thread, so that the change from content::RecordAction() to base::RecordAction()
is safe?

[Alexei Svitkine (slow), 2015-09-24 15:42:38]

I'm wondering, have you considered the inversion the other way?

Instead of having the helper be shared between iOS and other platforms, have the
provider be shared and have platform specific logic in the helper?

Just wondering why this approach is preferable to that.

One reason it's not preferable is it does require having separate provider
registration code (and ifdef'd dependencies) when registering providers.

https://codereview.chromium.org/1366643002/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1366643002/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:137:
->Contains(host->GetID()))
Nit: {}'s

https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
File components/metrics/stability_metrics_helper.cc (right):

https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
components/metrics/stability_metrics_helper.cc:150:
base::RecordAction(base::UserMetricsAction("PageLoad"));
On 2015/09/24 07:19:22, blundell wrote:
> One question: I assume ChromeStabilityMetricsProvider is used only on the UI
> thread, so that the change from content::RecordAction() to
base::RecordAction()
> is safe?

Yes, it's only used on the UI thread.

https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
File components/metrics/stability_metrics_helper.h (right):

https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
components/metrics/stability_metrics_helper.h:13: 
Nit: Remove empty line.

[blundell, 2015-09-24 15:49:32]

On 2015/09/24 15:42:38, Alexei Svitkine (slow) wrote:
> I'm wondering, have you considered the inversion the other way?
> 
> Instead of having the helper be shared between iOS and other platforms, have
the
> provider be shared and have platform specific logic in the helper?

Yep, I went back and forth between the two approaches. The reason that I went
for the shared helper in this case rather than making the provider itself shared
is because the shared code doesn't actually track any metrics on its own -- all
of that tracking is still in the //chrome-level (or //ios-chrome-level, once I
get to implementing the iOS-specific one) class. It seemed misleading to me to
have a StabilityMetricsProvider class in the component that if you just
instantiated it, would not actually track any metrics. IMO, a delegate makes
more sense in the case where the shared class implements the main functionality
and just needs to call out for some embedder-specific information. In this case,
it seems to me that it is the //chrome-level class that is actually doing the
main logic (which is //chrome-specific).

I don't feel super-strongly about it though so if you do, I don't mind switching
around.
 
> 
> Just wondering why this approach is preferable to that.
> 
> One reason it's not preferable is it does require having separate provider
> registration code (and ifdef'd dependencies) when registering providers.

iOS is going to have to move away from using ChromeMetricsProviderClient, so if
I'm understanding you correctly, I don't think that this in particular is an
issue.

> 
>
https://codereview.chromium.org/1366643002/diff/100001/chrome/browser/metrics...
> File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):
> 
>
https://codereview.chromium.org/1366643002/diff/100001/chrome/browser/metrics...
> chrome/browser/metrics/chrome_stability_metrics_provider.cc:137:
> ->Contains(host->GetID()))
> Nit: {}'s
> 
>
https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
> File components/metrics/stability_metrics_helper.cc (right):
> 
>
https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
> components/metrics/stability_metrics_helper.cc:150:
> base::RecordAction(base::UserMetricsAction("PageLoad"));
> On 2015/09/24 07:19:22, blundell wrote:
> > One question: I assume ChromeStabilityMetricsProvider is used only on the UI
> > thread, so that the change from content::RecordAction() to
> base::RecordAction()
> > is safe?
> 
> Yes, it's only used on the UI thread.
> 
>
https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
> File components/metrics/stability_metrics_helper.h (right):
> 
>
https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
> components/metrics/stability_metrics_helper.h:13: 
> Nit: Remove empty line.

[blundell, 2015-09-24 15:55:40]

On 2015/09/24 15:49:32, blundell wrote:
> On 2015/09/24 15:42:38, Alexei Svitkine (slow) wrote:
> > I'm wondering, have you considered the inversion the other way?
> > 
> > Instead of having the helper be shared between iOS and other platforms, have
> the
> > provider be shared and have platform specific logic in the helper?
> 
> Yep, I went back and forth between the two approaches. The reason that I went
> for the shared helper in this case rather than making the provider itself
shared
> is because the shared code doesn't actually track any metrics on its own --
all
> of that tracking is still in the //chrome-level (or //ios-chrome-level, once I
> get to implementing the iOS-specific one) class. It seemed misleading to me to
> have a StabilityMetricsProvider class in the component that if you just
> instantiated it, would not actually track any metrics. IMO, a delegate makes
> more sense in the case where the shared class implements the main
functionality
> and just needs to call out for some embedder-specific information. In this
case,
> it seems to me that it is the //chrome-level class that is actually doing the
> main logic (which is //chrome-specific).
> 
> I don't feel super-strongly about it though so if you do, I don't mind
switching
> around.
>  

(What I mean by "doesn't track metrics" here is "doesn't track events that
result in metrics being generated.")

> > 
> > Just wondering why this approach is preferable to that.
> > 
> > One reason it's not preferable is it does require having separate provider
> > registration code (and ifdef'd dependencies) when registering providers.
> 
> iOS is going to have to move away from using ChromeMetricsProviderClient, so
if
> I'm understanding you correctly, I don't think that this in particular is an
> issue.
> 
> > 
> >
>
https://codereview.chromium.org/1366643002/diff/100001/chrome/browser/metrics...
> > File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/1366643002/diff/100001/chrome/browser/metrics...
> > chrome/browser/metrics/chrome_stability_metrics_provider.cc:137:
> > ->Contains(host->GetID()))
> > Nit: {}'s
> > 
> >
>
https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
> > File components/metrics/stability_metrics_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
> > components/metrics/stability_metrics_helper.cc:150:
> > base::RecordAction(base::UserMetricsAction("PageLoad"));
> > On 2015/09/24 07:19:22, blundell wrote:
> > > One question: I assume ChromeStabilityMetricsProvider is used only on the
UI
> > > thread, so that the change from content::RecordAction() to
> > base::RecordAction()
> > > is safe?
> > 
> > Yes, it's only used on the UI thread.
> > 
> >
>
https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
> > File components/metrics/stability_metrics_helper.h (right):
> > 
> >
>
https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
> > components/metrics/stability_metrics_helper.h:13: 
> > Nit: Remove empty line.

[blundell, 2015-09-24 16:15:31]

Apologies, a rebase snuck into my response to your review.

https://codereview.chromium.org/1366643002/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right):

https://codereview.chromium.org/1366643002/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_stability_metrics_provider.cc:137:
->Contains(host->GetID()))
On 2015/09/24 15:42:38, Alexei Svitkine (slow) wrote:
> Nit: {}'s

Done.

https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
File components/metrics/stability_metrics_helper.h (right):

https://codereview.chromium.org/1366643002/diff/100001/components/metrics/sta...
components/metrics/stability_metrics_helper.h:13: 
On 2015/09/24 15:42:38, Alexei Svitkine (slow) wrote:
> Nit: Remove empty line.

Done.

[Alexei Svitkine (slow), 2015-09-24 16:48:42]

LGTM, thanks for the explanation.

[blundell, 2015-09-25 12:55:52]

blundell@chromium.org changed reviewers:
+ georgesak@chromium.org

[blundell, 2015-09-25 12:55:53]

georgesak@: Could you do an OWNERS review of //chrome/browser/memory? Thanks!

[Georges Khalil, 2015-09-25 13:47:31]

//chrome/browser/memory LGTM

[blundell, 2015-09-25 13:48:19]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2015-09-25 13:48:38]

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

[commit-bot: I haz the power, 2015-09-25 13:50:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-25 13:50:50]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[blundell, 2015-09-28 07:14:58]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-09-28 07:14:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, georgesak@chromium.org
Link to the patchset: https://codereview.chromium.org/1366643002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-28 07:16:30]

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

[commit-bot: I haz the power, 2015-09-28 08:34:27]

Committed patchset #8 (id:140001)

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

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