Reduce plugin_metrics_provider_ usage in MetricsService

Reduce plugin_metrics_provider_ usage in MetricsService

This CL eliminates MetricsService's call to
PluginMetricsProvider::RecordPluginChanges (replaced by a new API on
MetricsProvider). It additionally adds a
MetricsServicesManager::OnPluginLoadingError() API and has the plugin observer
call that API rather than calling MetricsService directly. This change will
enable easily moving MetricsService::LogPluginLoadingError() to
ChromeMetricsServiceClient once the latter is the class that keeps a weak
pointer to the plugin metrics provider.

BUG=375776
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274403

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275404

[blundell, 2014-05-28 19:24:08]

Split out from https://codereview.chromium.org/293393010/ to reduce the size of
that CL

[Alexei Svitkine (slow), 2014-05-29 20:47:43]

Sorry it took me so long to reply here, but I had/have mixed feelings about this
so wanted to take some time to formula my thoughts here.

https://codereview.chromium.org/308433004/diff/1/chrome/browser/browser_proce...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/308433004/diff/1/chrome/browser/browser_proce...
chrome/browser/browser_process.h:108: virtual PluginMetricsProvider*
plugin_metrics_provider() = 0;
I'd really like to hear Ilya's thoughts on this, but I'm not a big fan of
exposing the provider on g_browser_process.

I'd rather we expose MetricsServicesManager and for that have a method
"OnPluginLoadingError()" that the plugins code can call.

The reason I'd prefer the above is that I think we'll need to expose
MetricsServicesManager eventually anyway and it's cleaner to just have that
exposed than a bunch of smaller sub-parts (i.e. individual providers).

Then, PluginMetricsProvider would just be an implementation detail of the
metrics system and things wouldn't need to know about it.

Ilya, WDYT?

https://codereview.chromium.org/308433004/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/308433004/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:1411:
metrics_providers_[i]->RecordCurrentState();
I know this is what the current code does, but I'm really not convinced this is
the right thing to do.

It's pretty dubious to have sync the plugins list at arbitrary times to the
local state. Instead, I'd rather go with something that makes more sense - such
as record periodically whenever plugins change through a timer trigger within
the provider itself.

However, obviously this would be a functional change. If you'd really prefer
that we don't do functional changes in this refactoring, let's stick a TODO to
remove this method with a corresponding crbug. Then we can follow-up on this
later. (If we go this way, I don't mind owning it - you can assign the bug to
me.)

[Alexei Svitkine (slow), 2014-05-29 20:49:03]

https://codereview.chromium.org/308433004/diff/1/chrome/browser/browser_proce...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/308433004/diff/1/chrome/browser/browser_proce...
chrome/browser/browser_process.h:108: virtual PluginMetricsProvider*
plugin_metrics_provider() = 0;
On 2014/05/29 20:47:44, Alexei Svitkine wrote:
> I'd really like to hear Ilya's thoughts on this, but I'm not a big fan of
> exposing the provider on g_browser_process.
> 
> I'd rather we expose MetricsServicesManager and for that have a method
> "OnPluginLoadingError()" that the plugins code can call.
> 
> The reason I'd prefer the above is that I think we'll need to expose
> MetricsServicesManager eventually anyway and it's cleaner to just have that
> exposed than a bunch of smaller sub-parts (i.e. individual providers).
> 
> Then, PluginMetricsProvider would just be an implementation detail of the
> metrics system and things wouldn't need to know about it.
> 
> Ilya, WDYT?

(Just to clarify, my suggestion is to expose MetricsServicesManager and have a
method on that be plumbed through to the client and service.)

[Ilya Sherman, 2014-05-29 21:04:13]

https://codereview.chromium.org/308433004/diff/1/chrome/browser/browser_proce...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/308433004/diff/1/chrome/browser/browser_proce...
chrome/browser/browser_process.h:108: virtual PluginMetricsProvider*
plugin_metrics_provider() = 0;
On 2014/05/29 20:49:03, Alexei Svitkine wrote:
> On 2014/05/29 20:47:44, Alexei Svitkine wrote:
> > I'd really like to hear Ilya's thoughts on this, but I'm not a big fan of
> > exposing the provider on g_browser_process.
> > 
> > I'd rather we expose MetricsServicesManager and for that have a method
> > "OnPluginLoadingError()" that the plugins code can call.
> > 
> > The reason I'd prefer the above is that I think we'll need to expose
> > MetricsServicesManager eventually anyway and it's cleaner to just have that
> > exposed than a bunch of smaller sub-parts (i.e. individual providers).
> > 
> > Then, PluginMetricsProvider would just be an implementation detail of the
> > metrics system and things wouldn't need to know about it.
> > 
> > Ilya, WDYT?
> 
> (Just to clarify, my suggestion is to expose MetricsServicesManager and have a
> method on that be plumbed through to the client and service.)

Honestly, either approach is fine with me.  On the one hand, exposing just the
PluginMetricsProvider exposes a smaller API surface.  On the other hand, if
we're going to need to expose the MetricsServicesManager later on down the line
anyway, then there's not much harm to exposing it sooner rather than later.

[blundell, 2014-05-30 14:42:10]

Thanks, PTAL!

https://codereview.chromium.org/308433004/diff/1/chrome/browser/browser_proce...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/308433004/diff/1/chrome/browser/browser_proce...
chrome/browser/browser_process.h:108: virtual PluginMetricsProvider*
plugin_metrics_provider() = 0;
On 2014/05/29 20:47:44, Alexei Svitkine wrote:
> I'd really like to hear Ilya's thoughts on this, but I'm not a big fan of
> exposing the provider on g_browser_process.
> 
> I'd rather we expose MetricsServicesManager and for that have a method
> "OnPluginLoadingError()" that the plugins code can call.
> 
> The reason I'd prefer the above is that I think we'll need to expose
> MetricsServicesManager eventually anyway and it's cleaner to just have that
> exposed than a bunch of smaller sub-parts (i.e. individual providers).
> 
> Then, PluginMetricsProvider would just be an implementation detail of the
> metrics system and things wouldn't need to know about it.
> 
> Ilya, WDYT?

Done.

https://codereview.chromium.org/308433004/diff/1/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/308433004/diff/1/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:1411:
metrics_providers_[i]->RecordCurrentState();
On 2014/05/29 20:47:44, Alexei Svitkine wrote:
> I know this is what the current code does, but I'm really not convinced this
is
> the right thing to do.
> 
> It's pretty dubious to have sync the plugins list at arbitrary times to the
> local state. Instead, I'd rather go with something that makes more sense -
such
> as record periodically whenever plugins change through a timer trigger within
> the provider itself.
> 
> However, obviously this would be a functional change. If you'd really prefer
> that we don't do functional changes in this refactoring, let's stick a TODO to
> remove this method with a corresponding crbug. Then we can follow-up on this
> later. (If we go this way, I don't mind owning it - you can assign the bug to
> me.)

I think you're better suited to make that change. Bug created, TODO filed.

[Alexei Svitkine (slow), 2014-05-30 15:21:35]

LGTM % nit

Thanks!

https://codereview.chromium.org/308433004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.h (right):

https://codereview.chromium.org/308433004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.h:51: void
LogPluginLoadingError(const base::FilePath& plugin_path);
Nit: I'd name this OnPluginLoadingError(). The fact that it gets logged is an
implementation detail of the metrics system (i.e. we can still keep this name in
the other classes).

[Ilya Sherman, 2014-05-30 23:12:28]

LGTM2.  Please update the CL description, though :)

https://codereview.chromium.org/308433004/diff/20001/components/metrics/metri...
File components/metrics/metrics_provider.h (right):

https://codereview.chromium.org/308433004/diff/20001/components/metrics/metri...
components/metrics/metrics_provider.h:47: // TODO(asvitkine): Remove this
method. crbug.com/379148
nit: Please prepend "http://" to the URL, to make it more likely to be linkified
in editors.

[blundell, 2014-06-01 14:02:16]

https://codereview.chromium.org/308433004/diff/20001/chrome/browser/metrics/m...
File chrome/browser/metrics/metrics_services_manager.h (right):

https://codereview.chromium.org/308433004/diff/20001/chrome/browser/metrics/m...
chrome/browser/metrics/metrics_services_manager.h:51: void
LogPluginLoadingError(const base::FilePath& plugin_path);
On 2014/05/30 15:21:35, Alexei Svitkine wrote:
> Nit: I'd name this OnPluginLoadingError(). The fact that it gets logged is an
> implementation detail of the metrics system (i.e. we can still keep this name
in
> the other classes).

Done.

https://codereview.chromium.org/308433004/diff/20001/components/metrics/metri...
File components/metrics/metrics_provider.h (right):

https://codereview.chromium.org/308433004/diff/20001/components/metrics/metri...
components/metrics/metrics_provider.h:47: // TODO(asvitkine): Remove this
method. crbug.com/379148
On 2014/05/30 23:12:28, Ilya Sherman wrote:
> nit: Please prepend "http://" to the URL, to make it more likely to be
linkified
> in editors.

Done.

[blundell, 2014-06-01 14:08:16]

+jochen@ for *browser_process*. Thanks!

[jochen (gone - plz use gerrit), 2014-06-02 07:03:17]

lgtm

[blundell, 2014-06-02 07:06:33]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-02 07:06:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/308433004/60001

[commit-bot: I haz the power, 2014-06-02 22:33:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-03 01:19:51]

Change committed as 274403

[tzik, 2014-06-03 04:37:37]

A revert of this CL has been created in
https://codereview.chromium.org/314583002/ by tzik@chromium.org.

The reason for reverting is: This CL seems to cause bot failure on CI:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te....

[blundell, 2014-06-04 16:16:20]

Going to try an unchanged reland, following investigation by earthdok@ on the
Asan failures that caused this CL to be reverted:

"I think this is most likely a machine issue after all. I just realized that
browser_tests are sharded across three ASan testers, but only one of them
manifested the timeouts:

http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te...

Since sharding is random, the chance that your CL affected only the tests in the
first shard is negligible. Most likely there's some sort of persistent machine
issue.

Either that, or the specific test affected by your change (if there is one) has
a side-effect that breaks all the subsequent tests running on that machine. But
that is unlikely because the timeouts begin right away, not after a certain
specific test.

Also, note that even those tests that don't time out seem to take a very long
time to run (~45s).

I think you should try relanding it. If the issue reproduces, have someone ssh
to the bot and look at what's going on. I may be able to help if I'm still
here."

[blundell, 2014-06-04 16:16:29]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-04 16:17:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/308433004/80001

[blundell, 2014-06-04 16:19:11]

Additionally, earthdok@ couldn't repro locally and the linux_browser_asan trybot
run that I put through was green.

[commit-bot: I haz the power, 2014-06-04 19:50:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-04 19:53:56]

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

[commit-bot: I haz the power, 2014-06-04 19:53:57]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[blundell, 2014-06-04 20:41:32]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-04 20:42:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/308433004/80001

[commit-bot: I haz the power, 2014-06-04 20:49:00]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-06-04 20:53:50]

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

[commit-bot: I haz the power, 2014-06-04 20:53:50]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[blundell, 2014-06-05 09:35:14]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-05 09:38:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/308433004/80001

[commit-bot: I haz the power, 2014-06-05 09:43:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[blundell, 2014-06-05 09:46:23]

The CQ bit was unchecked by blundell@chromium.org

[blundell, 2014-06-05 09:46:50]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-05 09:47:41]

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

[commit-bot: I haz the power, 2014-06-05 09:47:41]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[blundell, 2014-06-05 09:49:28]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-05 09:50:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/308433004/80001

[commit-bot: I haz the power, 2014-06-05 15:19:52]

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

[commit-bot: I haz the power, 2014-06-05 15:19:52]

Failed to apply patch for chrome/browser/browser_process.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/browser_process.h
  Hunk #1 FAILED at 29.
  Hunk #2 succeeded at 105 (offset 3 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/browser_process.h.rej

Patch:       chrome/browser/browser_process.h
Index: chrome/browser/browser_process.h
diff --git a/chrome/browser/browser_process.h b/chrome/browser/browser_process.h
index
8e46d93b16c2b7c35bc7d782e60cff54be826441..cda9e23d9a77584f3499b99306faa3b347feccfb
100644
--- a/chrome/browser/browser_process.h
+++ b/chrome/browser/browser_process.h
@@ -29,6 +29,7 @@ class IntranetRedirectDetector;
 class IOThread;
 class MediaFileSystemRegistry;
 class MetricsService;
+class MetricsServicesManager;
 class NetworkTimeTracker;
 class NotificationUIManager;
 class PrefRegistrySimple;
@@ -102,6 +103,10 @@ class BrowserProcess {
   // continue shutdown.
   virtual void EndSession() = 0;
 
+  // Gets the manager for the various metrics-related services, constructing it
+  // if necessary.
+  virtual MetricsServicesManager* GetMetricsServicesManager() = 0;
+
   // Services: any of these getters may return NULL
   virtual MetricsService* metrics_service() = 0;
   virtual rappor::RapporService* rappor_service() = 0;

[blundell, 2014-06-05 16:25:20]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-05 16:27:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/308433004/100001

[commit-bot: I haz the power, 2014-06-05 19:18:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  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...)
  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...)

[commit-bot: I haz the power, 2014-06-05 19:24:11]

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

[commit-bot: I haz the power, 2014-06-05 19:24:13]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[blundell, 2014-06-06 07:50:00]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-06-06 07:51:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/blundell@chromium.org/308433004/120001

[commit-bot: I haz the power, 2014-06-06 11:04:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)

[commit-bot: I haz the power, 2014-06-06 11:22:33]

Change committed as 275404