Report disabled plugin in CreatePlugin.

Report disabled plugin in CreatePlugin.

Currently we only get stats about missing plugins (including unregistered plugin and missing plugin file). It is also interesting to know how many plugin creation failed because it is disabled. User may forget that he/she disabled a particular plugin and he/she doesn't get an info bar in this case, which could cause confusion.

In this CL:

- Rename MissingPluginReporter to PluginUMAReporter.
- Add ReportPluginDisabled function.
- Call ReportPluginDisabled function if the plugin is disabled in CreatePlugin.
- Refactor PluginUMATest to remove duplicate code.

BUG=226107
TEST=Unittests pass.

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

[xhwang, 2013-04-03 18:50:45]

PTAL

https://codereview.chromium.org/13414007/diff/1/chrome/renderer/plugins/plugi...
File chrome/renderer/plugins/plugin_uma_unittest.cc (right):

https://codereview.chromium.org/13414007/diff/1/chrome/renderer/plugins/plugi...
chrome/renderer/plugins/plugin_uma_unittest.cc:36: EXPECT_CALL(*sender_mock_,
SendPluginUMA(PluginUMAReporter::DISABLED_PLUGIN,
DISABLED_PLUGIN is tested here!

[Bernhard Bauer, 2013-04-03 19:08:42]

LGTM, with a suggestion for more cleanup in the test (depending on how you feel
about yak shaving):

https://codereview.chromium.org/13414007/diff/1/chrome/renderer/plugins/plugi...
File chrome/renderer/plugins/plugin_uma_unittest.cc (right):

https://codereview.chromium.org/13414007/diff/1/chrome/renderer/plugins/plugi...
chrome/renderer/plugins/plugin_uma_unittest.cc:49:
ExpectUMAReport(PluginUMAReporter::WINDOWS_MEDIA_PLAYER,
Thanks for this cleanup! We could probably go a bit further though and directly
test GetPluginType, as that's the interesting part. The rest is just testing
that ReportPlugin{Disabled,Missing} calls SendPluginUMA, which is IMO trivial.
That way, we could also get rid of the mocks here, which are somewhat confusing
to me.

[xhwang, 2013-04-04 17:28:38]

unittest updated. PTAL!

Sorry for the rebase noise in chrome_content_renderer_client.cc.

https://codereview.chromium.org/13414007/diff/1/chrome/renderer/plugins/plugi...
File chrome/renderer/plugins/plugin_uma_unittest.cc (right):

https://codereview.chromium.org/13414007/diff/1/chrome/renderer/plugins/plugi...
chrome/renderer/plugins/plugin_uma_unittest.cc:49:
ExpectUMAReport(PluginUMAReporter::WINDOWS_MEDIA_PLAYER,
On 2013/04/03 19:08:42, Bernhard Bauer wrote:
> Thanks for this cleanup! We could probably go a bit further though and
directly
> test GetPluginType, as that's the interesting part. The rest is just testing
> that ReportPlugin{Disabled,Missing} calls SendPluginUMA, which is IMO trivial.
> That way, we could also get rid of the mocks here, which are somewhat
confusing
> to me.

Done.

[ddorwin, 2013-04-04 19:09:50]

LGTM % nits. I'll defer to bauerb on the test changes.
I also have some suggestions for possible future improvements.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:514: 
We could do a histogram of values based on this |status_value|. That would cover
everything. This would be the inverse (list of status for each type) of what we
currently have (type by status).
Also, ReportPluginMissing() is used in another place, and I'm not sure what the
implication of such a change would be.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:519: orig_mime_type ==
content::kBrowserPluginMimeType) {
I wonder if kBrowserPluginMimeType is polluting stats since it is unhandled
falls into the OTHER category. Maybe we should handle kBrowserPluginMimeType
explicitly in plugin_uma.cc.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
File chrome/renderer/plugins/plugin_uma.cc (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
chrome/renderer/plugins/plugin_uma.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
same

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
File chrome/renderer/plugins/plugin_uma.h (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
chrome/renderer/plugins/plugin_uma.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
leave, right?

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
File chrome/renderer/plugins/plugin_uma_unittest.cc (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
chrome/renderer/plugins/plugin_uma_unittest.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
same

[Bernhard Bauer, 2013-04-04 21:14:31]

On 2013/04/04 17:28:38, xhwang wrote:
> unittest updated. PTAL!
> 
> Sorry for the rebase noise in chrome_content_renderer_client.cc.

FYI, you can do the following:

1. Sync
2. Upload
3. Make the review changes
4. Upload

That way, the reviewer can look at the diff between the patch sets created in
steps 2 and 4, which only contains your changes.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:514: 
On 2013/04/04 19:09:50, ddorwin wrote:
> We could do a histogram of values based on this |status_value|. That would
cover
> everything. This would be the inverse (list of status for each type) of what
we
> currently have (type by status).
> Also, ReportPluginMissing() is used in another place, and I'm not sure what
the
> implication of such a change would be.

The other place should be fine. What happens is that in theory, a plug-in could
go missing between the time we create a click-to-play placeholder for it and the
time the user clicks on it, so we deal with that, and also record the plug-in as
missing in that case.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
File chrome/renderer/plugins/plugin_uma_unittest.cc (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
chrome/renderer/plugins/plugin_uma_unittest.cc:9: void
ExpectPluginType(PluginUMAReporter::PluginType expected_plugin_type,
This should be in an anonymous namespace.

Of course, that means that you can't forward-declare this method as a friend of
PluginUMAReporter, but we usually don't friend individual methods anyway. The
standard way to do this would be to create a test fixture class (PluginUMATest)
that derives from ::testing::Test, define a protected method there, then friend
the test fixture in PluginUMAReporter, and make the test cases below use the
test fixture by using TEST_F instead of TEST.

[xhwang, 2013-04-04 23:10:33]

comment mostly resolved. PTAL

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:514: 
On 2013/04/04 21:14:31, Bernhard Bauer wrote:
> On 2013/04/04 19:09:50, ddorwin wrote:
> > We could do a histogram of values based on this |status_value|. That would
> cover
> > everything. This would be the inverse (list of status for each type) of what
> we
> > currently have (type by status).
> > Also, ReportPluginMissing() is used in another place, and I'm not sure what
> the
> > implication of such a change would be.
> 
> The other place should be fine. What happens is that in theory, a plug-in
could
> go missing between the time we create a click-to-play placeholder for it and
the
> time the user clicks on it, so we deal with that, and also record the plug-in
as
> missing in that case.

That's a good suggestion. But probably belongs to another CL.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:519: orig_mime_type ==
content::kBrowserPluginMimeType) {
On 2013/04/04 19:09:50, ddorwin wrote:
> I wonder if kBrowserPluginMimeType is polluting stats since it is unhandled
> falls into the OTHER category. Maybe we should handle kBrowserPluginMimeType
> explicitly in plugin_uma.cc.

sg, but that'll belong to another CL.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
File chrome/renderer/plugins/plugin_uma.cc (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
chrome/renderer/plugins/plugin_uma.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/04/04 19:09:50, ddorwin wrote:
> same

Done.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
File chrome/renderer/plugins/plugin_uma.h (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
chrome/renderer/plugins/plugin_uma.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/04/04 19:09:50, ddorwin wrote:
> leave, right?

Done.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
File chrome/renderer/plugins/plugin_uma_unittest.cc (right):

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
chrome/renderer/plugins/plugin_uma_unittest.cc:1: // Copyright (c) 2013 The
Chromium Authors. All rights reserved.
On 2013/04/04 19:09:50, ddorwin wrote:
> same

Done.

https://codereview.chromium.org/13414007/diff/13001/chrome/renderer/plugins/p...
chrome/renderer/plugins/plugin_uma_unittest.cc:9: void
ExpectPluginType(PluginUMAReporter::PluginType expected_plugin_type,
On 2013/04/04 21:14:31, Bernhard Bauer wrote:
> This should be in an anonymous namespace.
> 
> Of course, that means that you can't forward-declare this method as a friend
of
> PluginUMAReporter, but we usually don't friend individual methods anyway. The
> standard way to do this would be to create a test fixture class
(PluginUMATest)
> that derives from ::testing::Test, define a protected method there, then
friend
> the test fixture in PluginUMAReporter, and make the test cases below use the
> test fixture by using TEST_F instead of TEST.

Done.

[ddorwin, 2013-04-04 23:56:47]

Thanks. My issues are addressed.

https://codereview.chromium.org/13414007/diff/23001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/13414007/diff/23001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:525: // TODO(xhwang):
Distinguish between kNotFound and kBrowserPluginMimeType.
I don't think that's quite right. Either way, if you're doing this in the other
CL, you don't need this. (You would just have to modify this file to remove the
TODO later.)

[xhwang, 2013-04-05 01:00:24]

https://codereview.chromium.org/13414007/diff/23001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/13414007/diff/23001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:525: // TODO(xhwang):
Distinguish between kNotFound and kBrowserPluginMimeType.
On 2013/04/04 23:56:47, ddorwin wrote:
> I don't think that's quite right. Either way, if you're doing this in the
other
> CL, you don't need this. (You would just have to modify this file to remove
the
> TODO later.)

Done.

[commit-bot: I haz the power, 2013-04-05 01:00:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/13414007/40001

[commit-bot: I haz the power, 2013-04-05 01:00:44]

Presubmit check for 13414007-40001 failed and returned exit status 1.

INFO:root:Found 5 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/renderer/chrome_content_renderer_client.cc

[xhwang, 2013-04-05 01:13:46]

jam@: could you please do an owners review on
chrome/renderer/chrome_content_renderer_client.cc

[jam, 2013-04-05 17:58:08]

lgtm

[commit-bot: I haz the power, 2013-04-05 18:04:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/13414007/40001

[commit-bot: I haz the power, 2013-04-05 23:54:17]

Change committed as 192652