Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
293393010:
Move initial metrics gathering tasks out of MetricsService. (Closed)
|
Created:
6 years, 7 months ago by blundell Modified:
6 years, 6 months ago CC:
chromium-reviews, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMove initial metrics gathering tasks out of MetricsService.
This CL moves the coordination of initial metrics gathering tasks out of
MetricsService and into ChromeMetricsServiceClient. As part of this move, it
also does the following:
- Moves registration of metrics providers from MetricsService to
ChromeMetricsServiceClient
- Moves registration of prefs for individual metrics providers from
MetricsService to ChromeMetricsServiceClient
- Eliminates MetricsService being a TrackingSynchronizerObserver in favor of
ChromeMetricsServiceClient being a TrackingSynchronizerObserver
BUG=374218, 375776, 376288
TBR=thakis
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275753
Patch Set 1 #Patch Set 2 : Fixes #
Total comments: 5
Patch Set 3 : Rebase #
Total comments: 8
Patch Set 4 : Response to review #
Total comments: 27
Patch Set 5 : Response to review #
Total comments: 6
Patch Set 6 : Response to review #Patch Set 7 : Rebase #
Messages
Total messages: 27 (0 generated)
Mr. Svitkine and Mr. Sherman, I started pulling on one thread and it turned out to be intertangled with several other threads, so I pulled until they all came loose. The CL is larger than I had intended as a result. This CL is dependent on https://codereview.chromium.org/301633006/. Thanks, Dr. Blundell https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_stability_metrics_provider.cc (right): https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_stability_metrics_provider.cc:101: count = pref->GetInteger(prefs::kStabilityChildProcessCrashCount); This code should have been moved here when ChromeStabilityMetricsProvider was created. It's part of this CL because the MetricsLog unittest started crashing due to this pref no longer being registered. https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service_unittest.cc:281: service.InitializeMetricsRecordingState(); Needed due to the client causing a MetricsService codepath that wasn't previously exercised by this test to now be exercised. https://codereview.chromium.org/293393010/diff/20001/chrome/browser/plugins/p... File chrome/browser/plugins/plugin_observer.cc (right): https://codereview.chromium.org/293393010/diff/20001/chrome/browser/plugins/p... chrome/browser/plugins/plugin_observer.cc:459: g_browser_process->plugin_metrics_provider()->LogPluginLoadingError( I assume that plugins are guaranteed to be enabled here. This felt slightly cleaner to me than exposing the client via g_browser_process and having a method on the client that internally forwarded to the plugin metrics provider, but I don't feel strongly about it. https://codereview.chromium.org/293393010/diff/20001/components/metrics/metri... File components/metrics/metrics_provider.h (right): https://codereview.chromium.org/293393010/diff/20001/components/metrics/metri... components/metrics/metrics_provider.h:48: virtual void RecordCurrentState() {} Ideas on naming and commenting for this method?
https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_service.h:234: void RecordProfilerData( Hmm, I'm not super keen on exposing this like so. Probably, the right thing to do here is to have a ProfilerDataProvider which would also have the implementation of MetricsLog::RecordProfilerData(). Since that would be a large-ish change, it should probably be in a separate CL.
On 2014/05/27 20:49:59, Alexei Svitkine wrote: > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > File chrome/browser/metrics/metrics_service.h (right): > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > chrome/browser/metrics/metrics_service.h:234: void RecordProfilerData( > Hmm, I'm not super keen on exposing this like so. > > Probably, the right thing to do here is to have a ProfilerDataProvider which > would also have the implementation of MetricsLog::RecordProfilerData(). Since > that would be a large-ish change, it should probably be in a separate CL. It's not clear to me what you're suggesting as the next action here :).
On 2014/05/28 13:43:56, blundell wrote: > On 2014/05/27 20:49:59, Alexei Svitkine wrote: > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > File chrome/browser/metrics/metrics_service.h (right): > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > chrome/browser/metrics/metrics_service.h:234: void RecordProfilerData( > > Hmm, I'm not super keen on exposing this like so. > > > > Probably, the right thing to do here is to have a ProfilerDataProvider which > > would also have the implementation of MetricsLog::RecordProfilerData(). Since > > that would be a large-ish change, it should probably be in a separate CL. > > It's not clear to me what you're suggesting as the next action here :). I would say to do the ProfileDataProvider in a separate CL that this CL will then depend on.
On 2014/05/28 14:48:57, Alexei Svitkine wrote: > On 2014/05/28 13:43:56, blundell wrote: > > On 2014/05/27 20:49:59, Alexei Svitkine wrote: > > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > > File chrome/browser/metrics/metrics_service.h (right): > > > > > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > > chrome/browser/metrics/metrics_service.h:234: void RecordProfilerData( > > > Hmm, I'm not super keen on exposing this like so. > > > > > > Probably, the right thing to do here is to have a ProfilerDataProvider which > > > would also have the implementation of MetricsLog::RecordProfilerData(). > Since > > > that would be a large-ish change, it should probably be in a separate CL. > > > > It's not clear to me what you're suggesting as the next action here :). > > I would say to do the ProfileDataProvider in a separate CL that this CL will > then depend on. It's not clear to me where the code in MetricsService::ReceivedProfilerData() should go, and how the ProfileDataProvider would get access to the proto that it needs to write into. Could you give some more detail on what you're envisioning here?
On 2014/05/28 14:55:09, blundell wrote: > On 2014/05/28 14:48:57, Alexei Svitkine wrote: > > On 2014/05/28 13:43:56, blundell wrote: > > > On 2014/05/27 20:49:59, Alexei Svitkine wrote: > > > > > > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > > > File chrome/browser/metrics/metrics_service.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > > > chrome/browser/metrics/metrics_service.h:234: void RecordProfilerData( > > > > Hmm, I'm not super keen on exposing this like so. > > > > > > > > Probably, the right thing to do here is to have a ProfilerDataProvider > which > > > > would also have the implementation of MetricsLog::RecordProfilerData(). > > Since > > > > that would be a large-ish change, it should probably be in a separate CL. > > > > > > It's not clear to me what you're suggesting as the next action here :). > > > > I would say to do the ProfileDataProvider in a separate CL that this CL will > > then depend on. > > It's not clear to me where the code in MetricsService::ReceivedProfilerData() > should go, and how the ProfileDataProvider would get access to the proto that it > needs to write into. Could you give some more detail on what you're envisioning > here? Sorry, I meant ProfilerMetricsProvider which would be a MetricsProvider. It would have a ReceivedProfileData() method as well as the RecordProfilerData() code from metrics_log.cc moved to it. ChromeMetricsServiceClient can then call ReceivedProfileData() on it rather than on the MetricsService.
On 2014/05/28 15:07:12, Alexei Svitkine wrote: > On 2014/05/28 14:55:09, blundell wrote: > > On 2014/05/28 14:48:57, Alexei Svitkine wrote: > > > On 2014/05/28 13:43:56, blundell wrote: > > > > On 2014/05/27 20:49:59, Alexei Svitkine wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > > > > File chrome/browser/metrics/metrics_service.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > > > > chrome/browser/metrics/metrics_service.h:234: void RecordProfilerData( > > > > > Hmm, I'm not super keen on exposing this like so. > > > > > > > > > > Probably, the right thing to do here is to have a ProfilerDataProvider > > which > > > > > would also have the implementation of MetricsLog::RecordProfilerData(). > > > Since > > > > > that would be a large-ish change, it should probably be in a separate > CL. > > > > > > > > It's not clear to me what you're suggesting as the next action here :). > > > > > > I would say to do the ProfileDataProvider in a separate CL that this CL will > > > then depend on. > > > > It's not clear to me where the code in MetricsService::ReceivedProfilerData() > > should go, and how the ProfileDataProvider would get access to the proto that > it > > needs to write into. Could you give some more detail on what you're > envisioning > > here? > > Sorry, I meant ProfilerMetricsProvider which would be a MetricsProvider. It > would have a ReceivedProfileData() method as well as the RecordProfilerData() > code from metrics_log.cc moved to it. > > ChromeMetricsServiceClient can then call ReceivedProfileData() on it rather than > on the MetricsService. But where would ChromeMetricsServiceClient get the proto from to pass to the ProfilerMetricsProvider? In addition, MetricsService creates the initial log in ReceivedProfilerData, so it seems like MetricsService would still need some function that's called at that point.
On 2014/05/28 15:09:38, blundell wrote: > On 2014/05/28 15:07:12, Alexei Svitkine wrote: > > On 2014/05/28 14:55:09, blundell wrote: > > > On 2014/05/28 14:48:57, Alexei Svitkine wrote: > > > > On 2014/05/28 13:43:56, blundell wrote: > > > > > On 2014/05/27 20:49:59, Alexei Svitkine wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > > > > > File chrome/browser/metrics/metrics_service.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293393010/diff/20001/chrome/browser/metrics/m... > > > > > > chrome/browser/metrics/metrics_service.h:234: void RecordProfilerData( > > > > > > Hmm, I'm not super keen on exposing this like so. > > > > > > > > > > > > Probably, the right thing to do here is to have a ProfilerDataProvider > > > which > > > > > > would also have the implementation of > MetricsLog::RecordProfilerData(). > > > > Since > > > > > > that would be a large-ish change, it should probably be in a separate > > CL. > > > > > > > > > > It's not clear to me what you're suggesting as the next action here :). > > > > > > > > I would say to do the ProfileDataProvider in a separate CL that this CL > will > > > > then depend on. > > > > > > It's not clear to me where the code in > MetricsService::ReceivedProfilerData() > > > should go, and how the ProfileDataProvider would get access to the proto > that > > it > > > needs to write into. Could you give some more detail on what you're > > envisioning > > > here? > > > > Sorry, I meant ProfilerMetricsProvider which would be a MetricsProvider. It > > would have a ReceivedProfileData() method as well as the RecordProfilerData() > > code from metrics_log.cc moved to it. > > > > ChromeMetricsServiceClient can then call ReceivedProfileData() on it rather > than > > on the MetricsService. > > But where would ChromeMetricsServiceClient get the proto from to pass to the > ProfilerMetricsProvider? In addition, MetricsService creates the initial log in > ReceivedProfilerData, so it seems like MetricsService would still need some > function that's called at that point. The proto is already passed to ProvideGeneralMetrics() on the MetricsProvider. Note that the provider can keep a cached version of a proto as an ivar and swap the profiler data in it with the proto passed to the above function, similar to how OmniboxMetricsProvider does it.
I split the ProfilerMetricsProvider change into to its own CL and shifted out a couple of other "starter" pieces into their own CLs as well, resulting in a leaner, meaner version of this CL. PTAL!
https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:149: #endif // defined(OS_ANDROID) nit: Make it consistent - either have // defined(...) comments for all the blocks, or for none of them (or for all of them with bodies >1 line). https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:264: #endif Nit: #else NOTREACHED(): https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_services_manager.cc (right): https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_services_manager.cc:64: GetMetricsService(); Nit: Instead, create a private GetMetricsServiceClient() method and make GetMetricsService() simply call GetMetricsServiceClient()->metrics_service().
https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:149: #endif // defined(OS_ANDROID) On 2014/06/02 13:54:03, Alexei Svitkine wrote: > nit: Make it consistent - either have // defined(...) comments for all the > blocks, or for none of them (or for all of them with bodies >1 line). Done. https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:149: #endif // defined(OS_ANDROID) On 2014/06/02 13:54:03, Alexei Svitkine wrote: > nit: Make it consistent - either have // defined(...) comments for all the > blocks, or for none of them (or for all of them with bodies >1 line). Done. https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:264: #endif On 2014/06/02 13:54:03, Alexei Svitkine wrote: > Nit: #else > NOTREACHED(): Done. https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_services_manager.cc (right): https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_services_manager.cc:38: new ExtensionsMetricsProvider(GetMetricsStateManager()))); this is already done in ChromeMetricsServiceClient o_o https://codereview.chromium.org/293393010/diff/40001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_services_manager.cc:64: GetMetricsService(); On 2014/06/02 13:54:03, Alexei Svitkine wrote: > Nit: Instead, create a private GetMetricsServiceClient() method and make > GetMetricsService() simply call GetMetricsServiceClient()->metrics_service(). Done.
https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:153: #endif nit: "// defined(ENABLE_PLUGINS)" https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:163: new ExtensionsMetricsProvider(metrics_state_manager_))); Huh. Was this being doubly registered before? https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:169: #endif nit: I'd move this past line 181, so that all of the non-OS-specific bits are all grouped together. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:169: #endif nit: "// defined(OS_ANDROID)" https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:187: #endif nit: "// defined(OS_WIN)" https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:194: #endif nit: "// defined(ENABLE_PLUGINS)" https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:266: #endif nit: trailing comment https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.h (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:12: #include "base/files/file_path.h" nit: Can this be forward-declared? https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:37: public chrome_browser_metrics::TrackingSynchronizerObserver, nit: Hmm, I am really not fond of "chrome_browser_metrics" as a namespace. When did that get added? How would you guys feel about either changing to just "metrics", or removing it entirely? https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:48: // types we'll be using. nit: This doc string is really confusingly worded. I realize that you just copied it over verbatim, but mind updating it to something clearer, while you're here? https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:146: ProfilerMetricsProvider* profiler_metrics_provider_; nit: Please add some documentation, including about lifetime. Ditto for the two below. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_services_manager.h (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_services_manager.h:54: ChromeMetricsServiceClient* GetChromeMetricsServiceClient(); nit: Please add a doc string. Notably, this creates a MetricsService if it doesn't already exist, in addition to creating a ChromeMetricsServiceClient if it doesn't exist. (In fact, that's a little bit spooky. Is it safe for OnPluginLoadingError() to assume that the client already exists? If not, then this is a behavioral change.)
Thanks, PTAL! The CLs on which this is dependent have now all landed, so this will be ready to go once it gets LGTM'd. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:153: #endif On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: "// defined(ENABLE_PLUGINS)" Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:163: new ExtensionsMetricsProvider(metrics_state_manager_))); Yep. On 2014/06/02 21:52:37, Ilya Sherman wrote: > Huh. Was this being doubly registered before? https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:169: #endif On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: "// defined(OS_ANDROID)" Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:169: #endif On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: I'd move this past line 181, so that all of the non-OS-specific bits are > all grouped together. Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:187: #endif On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: "// defined(OS_WIN)" Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:194: #endif On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: "// defined(ENABLE_PLUGINS)" Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:266: #endif On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: trailing comment Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.h (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:12: #include "base/files/file_path.h" On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: Can this be forward-declared? Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:37: public chrome_browser_metrics::TrackingSynchronizerObserver, I'd rather leave this for a separate CL to reduce noise in this CL if that's OK with you. On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: Hmm, I am really not fond of "chrome_browser_metrics" as a namespace. When > did that get added? How would you guys feel about either changing to just > "metrics", or removing it entirely? https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:48: // types we'll be using. On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: This doc string is really confusingly worded. I realize that you just > copied it over verbatim, but mind updating it to something clearer, while you're > here? Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:146: ProfilerMetricsProvider* profiler_metrics_provider_; On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: Please add some documentation, including about lifetime. Ditto for the two > below. Done. https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_services_manager.h (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_services_manager.h:54: ChromeMetricsServiceClient* GetChromeMetricsServiceClient(); On 2014/06/02 21:52:37, Ilya Sherman wrote: > nit: Please add a doc string. Notably, this creates a MetricsService if it > doesn't already exist, in addition to creating a ChromeMetricsServiceClient if > it doesn't exist. (In fact, that's a little bit spooky. Is it safe for > OnPluginLoadingError() to assume that the client already exists? If not, then > this is a behavioral change.) Doc added. There's no behavioral change since the code that is in GetChromeMetricsServiceClient() used to be in GetMetricsService().
LGTM % comment, thanks! https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:267: void ChromeMetricsServiceClient::LogPluginLoadingError( Please define methods in the same order they're declared in the .h file.
Thanks, Colin. I'm still worried about the potential behavioral change around the call to GetChromeMetricsServiceClient(). Otherwise, LG! https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_services_manager.h (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_services_manager.h:54: ChromeMetricsServiceClient* GetChromeMetricsServiceClient(); On 2014/06/06 13:06:29, blundell wrote: > On 2014/06/02 21:52:37, Ilya Sherman wrote: > > nit: Please add a doc string. Notably, this creates a MetricsService if it > > doesn't already exist, in addition to creating a ChromeMetricsServiceClient if > > it doesn't exist. (In fact, that's a little bit spooky. Is it safe for > > OnPluginLoadingError() to assume that the client already exists? If not, then > > this is a behavioral change.) > > Doc added. There's no behavioral change since the code that is in > GetChromeMetricsServiceClient() used to be in GetMetricsService(). There is a potential behavioral change: OnPluginLoadingError() now also calls this method. Have you verified that OnPluginLoadingError() can only be called after the MetricsService is constructed? If so, perhaps it would make sense to just access the metrics_service_client_ directly from that method, rather than calling a method that might have the side-effect of creating the client if it didn't already exist. https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:167: nit: I was suggesting omitting this blank line as well, since there's not much reason to separate the ExtensionMetricsProvider from the rest. https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.h (right): https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:150: // MetricsService. Outlives this instance. Hmm, I think this is actually destroyed as part of the destructor, since |this| owns the MetricsService instance -- right?
Nevermind, I understand what you were saying now. LGTM with my other two nitty comments addressed. Thanks, Colin! https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_services_manager.h (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_services_manager.h:54: ChromeMetricsServiceClient* GetChromeMetricsServiceClient(); On 2014/06/06 19:04:18, Ilya Sherman wrote: > On 2014/06/06 13:06:29, blundell wrote: > > On 2014/06/02 21:52:37, Ilya Sherman wrote: > > > nit: Please add a doc string. Notably, this creates a MetricsService if it > > > doesn't already exist, in addition to creating a ChromeMetricsServiceClient > if > > > it doesn't exist. (In fact, that's a little bit spooky. Is it safe for > > > OnPluginLoadingError() to assume that the client already exists? If not, > then > > > this is a behavioral change.) > > > > Doc added. There's no behavioral change since the code that is in > > GetChromeMetricsServiceClient() used to be in GetMetricsService(). > > There is a potential behavioral change: OnPluginLoadingError() now also calls > this method. Have you verified that OnPluginLoadingError() can only be called > after the MetricsService is constructed? If so, perhaps it would make sense to > just access the metrics_service_client_ directly from that method, rather than > calling a method that might have the side-effect of creating the client if it > didn't already exist. Oh, actually, nvm -- I understand your point now: OnPluginLoadingError() used to call GetMetricsService(), which would have had the same side-effect. Alright, fair enough. I'm still not super comfortable with the notion that calling OnPluginLoadingError() might have the side effect of constructing a MetricsService(), but since that's consistent with what we had before, I guess we can worry about it in a separate CL, or just leave things in their slightly precarious current state.
https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... File chrome/browser/metrics/metrics_services_manager.h (right): https://codereview.chromium.org/293393010/diff/80001/chrome/browser/metrics/m... chrome/browser/metrics/metrics_services_manager.h:54: ChromeMetricsServiceClient* GetChromeMetricsServiceClient(); On 2014/06/06 19:04:18, Ilya Sherman wrote: > On 2014/06/06 13:06:29, blundell wrote: > > On 2014/06/02 21:52:37, Ilya Sherman wrote: > > > nit: Please add a doc string. Notably, this creates a MetricsService if it > > > doesn't already exist, in addition to creating a ChromeMetricsServiceClient > if > > > it doesn't exist. (In fact, that's a little bit spooky. Is it safe for > > > OnPluginLoadingError() to assume that the client already exists? If not, > then > > > this is a behavioral change.) > > > > Doc added. There's no behavioral change since the code that is in > > GetChromeMetricsServiceClient() used to be in GetMetricsService(). > > There is a potential behavioral change: OnPluginLoadingError() now also calls > this method. Have you verified that OnPluginLoadingError() can only be called > after the MetricsService is constructed? If so, perhaps it would make sense to > just access the metrics_service_client_ directly from that method, rather than > calling a method that might have the side-effect of creating the client if it > didn't already exist. It used to be that it was called through g_browser_process->metrics_service(), which was a method that also would create the MetricsService if it didn't exist. So I don't see how the behavior could change here...
https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:167: On 2014/06/06 19:04:18, Ilya Sherman wrote: > nit: I was suggesting omitting this blank line as well, since there's not much > reason to separate the ExtensionMetricsProvider from the rest. Done. https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.cc:267: void ChromeMetricsServiceClient::LogPluginLoadingError( On 2014/06/06 13:23:18, Alexei Svitkine wrote: > Please define methods in the same order they're declared in the .h file. Done. https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... File chrome/browser/metrics/chrome_metrics_service_client.h (right): https://codereview.chromium.org/293393010/diff/90001/chrome/browser/metrics/c... chrome/browser/metrics/chrome_metrics_service_client.h:150: // MetricsService. Outlives this instance. On 2014/06/06 19:04:18, Ilya Sherman wrote: > Hmm, I think this is actually destroyed as part of the destructor, since |this| > owns the MetricsService instance -- right? Done.
TBR=thakis for //chrome/browser/browser_prefs.cc
The CQ bit was checked by blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/293393010/130001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/14293) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/17405)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
The CQ bit was checked by blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/293393010/150001
Message was sent while issue was closed.
Change committed as 275753 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
