Create MetricsPrivateDelegate for metricsPrivate behavior

chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h

Index: chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h
diff --git a/chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h b/chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h
new file mode 100644
index 0000000000000000000000000000000000000000..a3183a4997f15b066875eee00aa2bba73aaaf66b
--- /dev/null
+++ b/chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h
@@ -0,0 +1,26 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_API_METRICS_PRIVATE_CHROME_METRICS_PRIVATE_DELEGATE_H_
+#define CHROME_BROWSER_EXTENSIONS_API_METRICS_PRIVATE_CHROME_METRICS_PRIVATE_DELEGATE_H_
+
+#include "base/macros.h"
+#include "extensions/browser/api/metrics_private/metrics_private_delegate.h"
+
+namespace extensions {
+
+class ChromeMetricsPrivateDelegate : public MetricsPrivateDelegate {
+ public:
+  ChromeMetricsPrivateDelegate() {}
+  ~ChromeMetricsPrivateDelegate() override {}
+
+  bool IsCrashReportingEnabled() override;

[Ilya Sherman, 2016/09/23 22:39:59]

I'm honestly somewhat uncomfortable allowing all clients of the metricsPrivate
API to query this state -- it's access controlled for a reason, which is that
it's rarely correct to branch code based on whether crash reporting is enabled. 
Do you know where this API is currently used, and whether it would be possible
to remove this functionality from the metricsPrivate API?  If there's only one
or two clients -- which I hope is the case -- it might be more appropriate to
create specific APIs for those clients to use, such that those APIs are more
tightly ACL'ed.

[michaelpg, 2016/09/23 23:48:15]

Does this CL change that, from an API standpoint? I guess friending
ChromeMetricsPrivateDelegate means that anyone can create this to call into the
MetricsServiceAccessor.

What's the motivation for ACLing this in the first place?
ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled() is mostly a
wrapper around the "user_experience_metrics.reporting_enabled" local state pref
(or "user_experience_metrics_crash.reporting_enabled" for Android), which
anything could use AFAICT. 
 
AFAICT it's used as a proxy for "is metrics reporting enabled?". I see quite a
few uses in google3, including https://goto.google.com/ztrpk which looks fun.

Use of this function in Chrome seems limited to google_now:
https://cs.chromium.org/chromium/src/chrome/browser/resources/google_now/util...

[Ilya Sherman, 2016/09/27 01:33:13]

This CL doesn't change the status quo dramatically, but it drew my attention to
this potential issue that I wasn't previously aware of.
 
It's true that code could just read the pref, but it's discouraged.  The reason
that we ACL this code is that many times, people try to branch based on the UMA
enabled state for invalid reasons -- typically, for performance optimizations. 
This results in UMA-enabled users getting a worse (read: slower) browser, which
then creates an incentive for people to opt out of metrics reporting, not due to
privacy concerns (which we understand and respect), but simply to get a better
browser.

So, we create this very promising-looking API which is a wrapper around the
pref, and then ACL it to make sure that we review code that uses it, as a
sanity-check that it's an appropriate use.  It's not a perfect system, but it
mostly works.  And it would be great to have some similar sanity-check
interposed for extensions that want to look at this state.
Hmm... not sure what to make of that use :/  FWIW, the corresponding design doc
[
https://docs.google.com/document/d/1edsT3LeN7qL_bSdnp6pudRsjE2TT1fFmViTByjYuN...
] makes it seem probably reasonable?

[Rahul Chaturvedi, 2016/09/27 18:41:57]

I am a bit confused here. This is a private API. Any users of this API are
Google apps and extensions which are going to go through a security review.
Secondly, we're moving this API from location (a) in code, to location (b). Does
this warrant a change in the API behavior itself?


I understand that this API is not behaving ideally, but then we should file a
bug on that and address that in another CL. I don't see that falling in the
scope of this CL.

[Ilya Sherman, 2016/09/27 20:58:29]

A security review has nothing to do with the issue that I'm raising.
I agree that it is likely not in the scope of this CL.  However, this seemed
like an opportune moment to ask extensions experts to think about solutions to
the issue that I raised.  And, it's very possible that the solution to the issue
that I raised would also affect how this CL is implemented -- thusly do I raise
the concern.

+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ChromeMetricsPrivateDelegate);
+};
+
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_METRICS_PRIVATE_CHROME_METRICS_PRIVATE_DELEGATE_H_