Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2331343012:
Create MetricsPrivateDelegate for metricsPrivate behavior (Closed)
DescriptionCreate MetricsPrivateDelegate for metricsPrivate behavior
This creates a MetricsPrivateDelegate for metricsPrivate functionality
implemented by the embedder. This helps deal with the way MetricsServiceAccessor
uses inheritance to restrict access to static methods to friends of subclasses.
(MetricServiceAccessor is "pretty much just a hack around limitations in the
OWNERS system": crbug.com/374199#c7)
Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002).
BUG=647397
Committed: https://crrev.com/49dc7dbfbdf93c439e4d3497442ef2dde9cea330
Cr-Commit-Position: refs/heads/master@{#423442}
Patch Set 1 #
Total comments: 8
Patch Set 2 : ExtensionsAPIClient owns MetricsPrivateDelegate instance #
Total comments: 17
Patch Set 3 : remove ShellMetricsPrivateDelegate #
Total comments: 3
Patch Set 4 : nit & rebase #Messages
Total messages: 42 (13 generated)
michaelpg@chromium.org changed reviewers: + steel@chromium.org
steel: please take a preliminary look.
Looks all good except for creating a context keyed API. https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/metrics_private/metrics_private_api.h (right): https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/metrics_private/metrics_private_api.h:202: class MetricsPrivateAPI : public BrowserContextKeyedAPI { Is this needed? Our delegate isn't context specific but global. Could we instead have [Chrome|Shell]ExtensionsApiClient keep the pointer to the delegate instead and return that to us when we ask for it? https://codereview.chromium.org/2331343012/diff/1/components/metrics/metrics_... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/1/components/metrics/metrics_... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android Did you mean to change this comment?
https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/metrics_private/metrics_private_api.h (right): https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/metrics_private/metrics_private_api.h:202: class MetricsPrivateAPI : public BrowserContextKeyedAPI { On 2016/09/16 23:11:04, Rahul Chaturvedi wrote: > Is this needed? Our delegate isn't context specific but global. > Could we instead have [Chrome|Shell]ExtensionsApiClient keep the pointer to the > delegate instead and return that to us when we ask for it? Good point. We could make the ExtensionAPIClient impl keep the delegate as a static or member variable... or just give ownership to the caller. I wonder if this would make more sense as a static function, with different implementation files for different platforms. Is there a reason to prefer the ExtensionApiClient interface over just creating a one-off function? https://codereview.chromium.org/2331343012/diff/1/components/metrics/metrics_... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/1/components/metrics/metrics_... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android On 2016/09/16 23:11:04, Rahul Chaturvedi wrote: > Did you mean to change this comment? yes, it's outdated -- it was fixed for Chrome OS in https://codereview.chromium.org/1411863002. The referenced method, ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled(), only has special cases for Android now (and in tests).
https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/metrics_private/metrics_private_api.h (right): https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/metrics_private/metrics_private_api.h:202: class MetricsPrivateAPI : public BrowserContextKeyedAPI { On 2016/09/16 23:51:56, michaelpg wrote: > On 2016/09/16 23:11:04, Rahul Chaturvedi wrote: > > Is this needed? Our delegate isn't context specific but global. > > Could we instead have [Chrome|Shell]ExtensionsApiClient keep the pointer to > the > > delegate instead and return that to us when we ask for it? > > Good point. We could make the ExtensionAPIClient impl keep the delegate as a > static or member variable... or just give ownership to the caller. > > I wonder if this would make more sense as a static function, with different > implementation files for different platforms. Is there a reason to prefer the > ExtensionApiClient interface over just creating a one-off function? ExtensionsAPIClient::Get() returns the the correct subclass of ExtensionAPIClient for our build ([Chrome|Shell]ExtensionsApiClient). If we use a static function, how would we automatically get the correct version of our function? https://codereview.chromium.org/2331343012/diff/1/components/metrics/metrics_... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/1/components/metrics/metrics_... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android On 2016/09/16 23:51:56, michaelpg wrote: > On 2016/09/16 23:11:04, Rahul Chaturvedi wrote: > > Did you mean to change this comment? > > yes, it's outdated -- it was fixed for Chrome OS in > https://codereview.chromium.org/1411863002. The referenced method, > ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled(), only has > special cases for Android now (and in tests). Acknowledged.
https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/metrics_private/metrics_private_api.h (right): https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/metrics_private/metrics_private_api.h:202: class MetricsPrivateAPI : public BrowserContextKeyedAPI { On 2016/09/19 18:11:09, Rahul Chaturvedi wrote: > On 2016/09/16 23:51:56, michaelpg wrote: > > On 2016/09/16 23:11:04, Rahul Chaturvedi wrote: > > > Is this needed? Our delegate isn't context specific but global. > > > Could we instead have [Chrome|Shell]ExtensionsApiClient keep the pointer to > > the > > > delegate instead and return that to us when we ask for it? > > > > Good point. We could make the ExtensionAPIClient impl keep the delegate as a > > static or member variable... or just give ownership to the caller. > > > > I wonder if this would make more sense as a static function, with different > > implementation files for different platforms. Is there a reason to prefer the > > ExtensionApiClient interface over just creating a one-off function? > > ExtensionsAPIClient::Get() returns the the correct subclass of > ExtensionAPIClient for our build ([Chrome|Shell]ExtensionsApiClient). If we use > a static function, how would we automatically get the correct version of our > function? We do this a lot for platform-specific functionality: foo.h is implemented in different files (foo_impl_win.cc, foo_impl_chromeos.cc). The implementation is conditionally included at compile-time, as opposed to using sub-classing and virtual methods. But the examples I've seen are mostly *platform* specific (memory/file management, set as default browser, etc.). MetricsPrivateDelegate's functionality is *embedder* specific, which I guess we don't use that pattern for (idk why). Since ExtensionsAPIClient already exists, I can just continue using that. It just seems weird to own a pointer to what is essentially static functionality.
https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/metrics_private/metrics_private_api.h (right): https://codereview.chromium.org/2331343012/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/metrics_private/metrics_private_api.h:202: class MetricsPrivateAPI : public BrowserContextKeyedAPI { On 2016/09/19 18:36:32, michaelpg wrote: > On 2016/09/19 18:11:09, Rahul Chaturvedi wrote: > > On 2016/09/16 23:51:56, michaelpg wrote: > > > On 2016/09/16 23:11:04, Rahul Chaturvedi wrote: > > > > Is this needed? Our delegate isn't context specific but global. > > > > Could we instead have [Chrome|Shell]ExtensionsApiClient keep the pointer > to > > > the > > > > delegate instead and return that to us when we ask for it? > > > > > > Good point. We could make the ExtensionAPIClient impl keep the delegate as a > > > static or member variable... or just give ownership to the caller. > > > > > > I wonder if this would make more sense as a static function, with different > > > implementation files for different platforms. Is there a reason to prefer > the > > > ExtensionApiClient interface over just creating a one-off function? > > > > ExtensionsAPIClient::Get() returns the the correct subclass of > > ExtensionAPIClient for our build ([Chrome|Shell]ExtensionsApiClient). If we > use > > a static function, how would we automatically get the correct version of our > > function? > > We do this a lot for platform-specific functionality: foo.h is implemented in > different files (foo_impl_win.cc, foo_impl_chromeos.cc). The implementation is > conditionally included at compile-time, as opposed to using sub-classing and > virtual methods. > > But the examples I've seen are mostly *platform* specific (memory/file > management, set as default browser, etc.). MetricsPrivateDelegate's > functionality is *embedder* specific, which I guess we don't use that pattern > for (idk why). Since ExtensionsAPIClient already exists, I can just continue > using that. It just seems weird to own a pointer to what is essentially static > functionality. So in 'this' particular situation, a few reasons to pick this pattern over conditional compilation are, (a) xxx_platform.cc files are automatically only compiled for their platform, we don't have any such functionality for app_shell. We'd have to add separate build rules for them, which I would rather avoid. In principle, I prefer to avoid adding build rules since it can very quickly slippery slope into massive build files with enough conditions and sub-conditions to make one's head hurt. (b) Adding this pattern would be more consistent with the way we do delegate functionality in other places. :) I realize that neither of these are super-compelling reasons, but I don't see many compelling reasons to go the other way either.
ptal at the changes before I send out for review.
lgtm
Description was changed from ========== Create MetricsPrivateAPI and MetricsPrivateDelegate This creates a MetricsPrivateDelegate accessible via the MetricsPrivateAPI BrowserContextKeyedAPI. This helps deal with the way MetricsServiceAccessor uses inheritance to restrict access to static methods to friends of subclasses. (MetricServiceAccessor is "pretty much just a hack around limitations in the OWNERS system": crbug.comc/374199#c7) Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002). BUG=647397 ========== to ========== Create MetricsPrivateDelegate for metricsPrivate behavior This creates a MetricsPrivateDelegate for metricsPrivate functionality implemented by the embedder. This helps deal with the way MetricsServiceAccessor uses inheritance to restrict access to static methods to friends of subclasses. (MetricServiceAccessor is "pretty much just a hack around limitations in the OWNERS system": crbug.comc/374199#c7) Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002). BUG=647397 ==========
michaelpg@chromium.org changed reviewers: + finnur@chromium.org, isherman@chromium.org
michaelpg@chromium.org changed required reviewers: + finnur@chromium.org, isherman@chromium.org
PTAL, or let me know if you think there are more appropriate OWNERs (extensions seems kinda random).
steel@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
+devlin for owners review.
michaelpg@chromium.org changed required reviewers: - finnur@chromium.org
michaelpg@chromium.org changed reviewers: - finnur@chromium.org
michaelpg@chromium.org changed required reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h:18: bool IsCrashReportingEnabled() override; 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. https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android Is your CL changing something about the correctness of this method's return value on ChromeOS?
Description was changed from ========== Create MetricsPrivateDelegate for metricsPrivate behavior This creates a MetricsPrivateDelegate for metricsPrivate functionality implemented by the embedder. This helps deal with the way MetricsServiceAccessor uses inheritance to restrict access to static methods to friends of subclasses. (MetricServiceAccessor is "pretty much just a hack around limitations in the OWNERS system": crbug.comc/374199#c7) Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002). BUG=647397 ========== to ========== Create MetricsPrivateDelegate for metricsPrivate behavior This creates a MetricsPrivateDelegate for metricsPrivate functionality implemented by the embedder. This helps deal with the way MetricsServiceAccessor uses inheritance to restrict access to static methods to friends of subclasses. (MetricServiceAccessor is "pretty much just a hack around limitations in the OWNERS system": crbug.com/374199#c7) Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002). BUG=647397 ==========
https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h:18: bool IsCrashReportingEnabled() override; On 2016/09/23 22:39:59, Ilya Sherman wrote: > 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. 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. > 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. 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... https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android On 2016/09/23 22:39:59, Ilya Sherman wrote: > Is your CL changing something about the correctness of this method's return > value on ChromeOS? No. The method seems to have already been corrected on Chrome OS in November, making the comment incorrect: https://bugs.chromium.org/p/chromium/issues/detail?id=532084#c3 If that's correct, would you prefer me to make this comment change in a separate CL?
https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... File extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. When is this used?
https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... File extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/26 16:27:13, Devlin (catching up) wrote: > When is this used? https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse...
https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... File extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/26 18:22:59, Rahul Chaturvedi wrote: > On 2016/09/26 16:27:13, Devlin (catching up) wrote: > > When is this used? > > https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... Right, but the object offers no value there. I don't see the point in adding a shell implementation if it provides no functionality. It would be easier and far less code to just do: is_crash_reporting_enabled = delegate && delegate->IsCrashReportingEnabled(); We have a lot of unnecessary shell delegate implementations already, and it makes finding out where the behavior is fundamentally different very difficult. If we only provide an implementation when it's actually usable, it's far simpler to trace down potential code paths.
https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... File extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/26 18:26:31, Devlin (catching up) wrote: > On 2016/09/26 18:22:59, Rahul Chaturvedi wrote: > > On 2016/09/26 16:27:13, Devlin (catching up) wrote: > > > When is this used? > > > > > https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... > > Right, but the object offers no value there. I don't see the point in adding a > shell implementation if it provides no functionality. It would be easier and > far less code to just do: > is_crash_reporting_enabled = delegate && delegate->IsCrashReportingEnabled(); > > We have a lot of unnecessary shell delegate implementations already, and it > makes finding out where the behavior is fundamentally different very difficult. > If we only provide an implementation when it's actually usable, it's far simpler > to trace down potential code paths. I like that idea, in general. We wouldn't have to change shell_extensions_api_client.* either. rkc^H^H^Hsteel, are you OK with that? There's a tangential discussion in CrOS about how to structure services/dependencies -- main question is how to handle ownership, existence, and fetching of services from different processes. See https://docs.google.com/a/google.com/document/d/16pmp3Vpe1fIzdjakwyEw2Fn2HjX5...
https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... File extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/26 18:26:31, Devlin (catching up) wrote: > On 2016/09/26 18:22:59, Rahul Chaturvedi wrote: > > On 2016/09/26 16:27:13, Devlin (catching up) wrote: > > > When is this used? > > > > > https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... > > Right, but the object offers no value there. I don't see the point in adding a > shell implementation if it provides no functionality. (although I'll note, we do need to add that functionality later.) > It would be easier and > far less code to just do: > is_crash_reporting_enabled = delegate && delegate->IsCrashReportingEnabled(); > > We have a lot of unnecessary shell delegate implementations already, and it > makes finding out where the behavior is fundamentally different very difficult. > If we only provide an implementation when it's actually usable, it's far simpler > to trace down potential code paths.
https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h:18: bool IsCrashReportingEnabled() override; On 2016/09/23 23:48:15, michaelpg wrote: > On 2016/09/23 22:39:59, Ilya Sherman wrote: > > 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. > > Does this CL change that, from an API standpoint? I guess friending > ChromeMetricsPrivateDelegate means that anyone can create this to call into the > MetricsServiceAccessor. 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. > 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. 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. > > 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. > > 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. 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? > Use of this function in Chrome seems limited to google_now: > https://cs.chromium.org/chromium/src/chrome/browser/resources/google_now/util... https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android On 2016/09/23 23:48:15, michaelpg wrote: > On 2016/09/23 22:39:59, Ilya Sherman wrote: > > Is your CL changing something about the correctness of this method's return > > value on ChromeOS? > > No. The method seems to have already been corrected on Chrome OS in November, > making the comment incorrect: > https://bugs.chromium.org/p/chromium/issues/detail?id=532084#c3 > > If that's correct, would you prefer me to make this comment change in a > separate CL? Yes please, and send the code review to gayane@, as she is the most familiar with the details of this code currently =)
https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android On 2016/09/27 01:33:13, Ilya Sherman wrote: > On 2016/09/23 23:48:15, michaelpg wrote: > > On 2016/09/23 22:39:59, Ilya Sherman wrote: > > > Is your CL changing something about the correctness of this method's return > > > value on ChromeOS? > > > > No. The method seems to have already been corrected on Chrome OS in November, > > making the comment incorrect: > > https://bugs.chromium.org/p/chromium/issues/detail?id=532084#c3 > > > > If that's correct, would you prefer me to make this comment change in a > > separate CL? > > Yes please, and send the code review to gayane@, as she is the most familiar > with the details of this code currently =) Looks like gayane@ beat me to it! https://codereview.chromium.org/2351873002
https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h:18: bool IsCrashReportingEnabled() override; On 2016/09/27 01:33:13, Ilya Sherman wrote: > On 2016/09/23 23:48:15, michaelpg wrote: > > On 2016/09/23 22:39:59, Ilya Sherman wrote: > > > 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. > > > > Does this CL change that, from an API standpoint? I guess friending > > ChromeMetricsPrivateDelegate means that anyone can create this to call into > the > > MetricsServiceAccessor. > > 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. > > > 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. > > 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. > > > > 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. > > > > 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. > > 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? > > > Use of this function in Chrome seems limited to google_now: > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/google_now/util... 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. https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... File extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... extensions/shell/browser/api/metrics_private/shell_metrics_private_delegate.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/09/27 00:09:30, michaelpg wrote: > On 2016/09/26 18:26:31, Devlin (catching up) wrote: > > On 2016/09/26 18:22:59, Rahul Chaturvedi wrote: > > > On 2016/09/26 16:27:13, Devlin (catching up) wrote: > > > > When is this used? > > > > > > > > > https://codereview.chromium.org/2331343012/diff/20001/extensions/shell/browse... > > > > Right, but the object offers no value there. I don't see the point in adding > a > > shell implementation if it provides no functionality. > > (although I'll note, we do need to add that functionality later.) > > > It would be easier and > > far less code to just do: > > is_crash_reporting_enabled = delegate && delegate->IsCrashReportingEnabled(); > > > > We have a lot of unnecessary shell delegate implementations already, and it > > makes finding out where the behavior is fundamentally different very > difficult. > > If we only provide an implementation when it's actually usable, it's far > simpler > > to trace down potential code paths. > I like the idea of adding functionality when needed, so sgtm.
https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h:18: bool IsCrashReportingEnabled() override; On 2016/09/27 18:41:57, Rahul Chaturvedi wrote: > On 2016/09/27 01:33:13, Ilya Sherman wrote: > > On 2016/09/23 23:48:15, michaelpg wrote: > > > On 2016/09/23 22:39:59, Ilya Sherman wrote: > > > > 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. > > > > > > Does this CL change that, from an API standpoint? I guess friending > > > ChromeMetricsPrivateDelegate means that anyone can create this to call into > > the > > > MetricsServiceAccessor. > > > > 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. > > > > > 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. > > > > 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. > > > > > > 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. > > > > > > 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. > > > > 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? > > > > > Use of this function in Chrome seems limited to google_now: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/resources/google_now/util... > > 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? A security review has nothing to do with the issue that I'm raising. > 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. 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.
Hokay, talked with steel@ offline, and we agreed that a reasonable solution going forward is to (a) whitelist this metricsPrivate API function separately, and (b) add clear documentation that any new uses of this function should be reviewed by the metrics team. Since that's orthogonal to this CL, it makes sense to implement in a separate CL. So, chrome/browser/metrics/chrome_metrics_service_accessor.h lgtm. https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android On 2016/09/27 16:45:16, michaelpg wrote: > On 2016/09/27 01:33:13, Ilya Sherman wrote: > > On 2016/09/23 23:48:15, michaelpg wrote: > > > On 2016/09/23 22:39:59, Ilya Sherman wrote: > > > > Is your CL changing something about the correctness of this method's > return > > > > value on ChromeOS? > > > > > > No. The method seems to have already been corrected on Chrome OS in > November, > > > making the comment incorrect: > > > https://bugs.chromium.org/p/chromium/issues/detail?id=532084#c3 > > > > > > If that's correct, would you prefer me to make this comment change in a > > > separate CL? > > > > Yes please, and send the code review to gayane@, as she is the most familiar > > with the details of this code currently =) > > Looks like gayane@ beat me to it! https://codereview.chromium.org/2351873002 Great! Please make sure to remember to revert your changes to this file =)
On 2016/09/27 21:16:50, Ilya Sherman wrote: > Hokay, talked with steel@ offline, and we agreed that a reasonable solution > going forward is to (a) whitelist this metricsPrivate API function separately, > and (b) add clear documentation that any new uses of this function should be > reviewed by the metrics team. Since that's orthogonal to this CL, it makes > sense to implement in a separate CL. PTAL: https://codereview.chromium.org/2387793002 > > So, chrome/browser/metrics/chrome_metrics_service_accessor.h lgtm. > > https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... > File components/metrics/metrics_service_accessor.h (right): > > https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... > components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently > does not return the correct value on Android > On 2016/09/27 16:45:16, michaelpg wrote: > > On 2016/09/27 01:33:13, Ilya Sherman wrote: > > > On 2016/09/23 23:48:15, michaelpg wrote: > > > > On 2016/09/23 22:39:59, Ilya Sherman wrote: > > > > > Is your CL changing something about the correctness of this method's > > return > > > > > value on ChromeOS? > > > > > > > > No. The method seems to have already been corrected on Chrome OS in > > November, > > > > making the comment incorrect: > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=532084#c3 > > > > > > > > If that's correct, would you prefer me to make this comment change in a > > > > separate CL? > > > > > > Yes please, and send the code review to gayane@, as she is the most familiar > > > with the details of this code currently =) > > > > Looks like gayane@ beat me to it! https://codereview.chromium.org/2351873002 > > Great! Please make sure to remember to revert your changes to this file =)
okay, here's the agreed-upon patch. Devlin, mind looking this over? https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... File components/metrics/metrics_service_accessor.h (right): https://codereview.chromium.org/2331343012/diff/20001/components/metrics/metr... components/metrics/metrics_service_accessor.h:32: // NOTE: This method currently does not return the correct value on Android On 2016/09/27 21:16:49, Ilya Sherman wrote: > On 2016/09/27 16:45:16, michaelpg wrote: > > On 2016/09/27 01:33:13, Ilya Sherman wrote: > > > On 2016/09/23 23:48:15, michaelpg wrote: > > > > On 2016/09/23 22:39:59, Ilya Sherman wrote: > > > > > Is your CL changing something about the correctness of this method's > > return > > > > > value on ChromeOS? > > > > > > > > No. The method seems to have already been corrected on Chrome OS in > > November, > > > > making the comment incorrect: > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=532084#c3 > > > > > > > > If that's correct, would you prefer me to make this comment change in a > > > > separate CL? > > > > > > Yes please, and send the code review to gayane@, as she is the most familiar > > > with the details of this code currently =) > > > > Looks like gayane@ beat me to it! https://codereview.chromium.org/2351873002 > > Great! Please make sure to remember to revert your changes to this file =) Done.
lgtm https://codereview.chromium.org/2331343012/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h:18: bool IsCrashReportingEnabled() override; nit: // MetricsPrivateDelegate: https://codereview.chromium.org/2331343012/diff/40001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/metrics/test.js (right): https://codereview.chromium.org/2331343012/diff/40001/chrome/test/data/extens... chrome/test/data/extensions/api_test/metrics/test.js:14: chrome.test.assertEq('boolean', typeof enabled); It'd be kind of nice to verify the result is correct, but I suppose that might be out of scope for this CL. But if you wanted to do it anyway... ;)
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from steel@chromium.org, isherman@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2331343012/#ps60001 (title: "nit & rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Create MetricsPrivateDelegate for metricsPrivate behavior This creates a MetricsPrivateDelegate for metricsPrivate functionality implemented by the embedder. This helps deal with the way MetricsServiceAccessor uses inheritance to restrict access to static methods to friends of subclasses. (MetricServiceAccessor is "pretty much just a hack around limitations in the OWNERS system": crbug.com/374199#c7) Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002). BUG=647397 ========== to ========== Create MetricsPrivateDelegate for metricsPrivate behavior This creates a MetricsPrivateDelegate for metricsPrivate functionality implemented by the embedder. This helps deal with the way MetricsServiceAccessor uses inheritance to restrict access to static methods to friends of subclasses. (MetricServiceAccessor is "pretty much just a hack around limitations in the OWNERS system": crbug.com/374199#c7) Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002). BUG=647397 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Create MetricsPrivateDelegate for metricsPrivate behavior This creates a MetricsPrivateDelegate for metricsPrivate functionality implemented by the embedder. This helps deal with the way MetricsServiceAccessor uses inheritance to restrict access to static methods to friends of subclasses. (MetricServiceAccessor is "pretty much just a hack around limitations in the OWNERS system": crbug.com/374199#c7) Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002). BUG=647397 ========== to ========== Create MetricsPrivateDelegate for metricsPrivate behavior This creates a MetricsPrivateDelegate for metricsPrivate functionality implemented by the embedder. This helps deal with the way MetricsServiceAccessor uses inheritance to restrict access to static methods to friends of subclasses. (MetricServiceAccessor is "pretty much just a hack around limitations in the OWNERS system": crbug.com/374199#c7) Precursor to moving chrome.metricsPrivate to //extensions (crrev.com/2349453002). BUG=647397 Committed: https://crrev.com/49dc7dbfbdf93c439e4d3497442ef2dde9cea330 Cr-Commit-Position: refs/heads/master@{#423442} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/49dc7dbfbdf93c439e4d3497442ef2dde9cea330 Cr-Commit-Position: refs/heads/master@{#423442}
Message was sent while issue was closed.
https://codereview.chromium.org/2331343012/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h (right): https://codereview.chromium.org/2331343012/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/metrics_private/chrome_metrics_private_delegate.h:18: bool IsCrashReportingEnabled() override; On 2016/10/01 01:25:14, Devlin wrote: > nit: > // MetricsPrivateDelegate: Done. |
