Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13601002:
Report UMA for Shockwave Flash and Widevine CDM. (Closed)
|
Created:
7 years, 8 months ago by xhwang Modified:
7 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReport UMA for Shockwave Flash and Widevine CDM.
We have tons of (a huge number!) missing plugins reported every day. The current enumerated ones only cover <1% of them. I'd like to add more emums in the list to cover more so that we have a better knowledge what's going on.
We can add more if needed.
I'll update the histograms.xml after this CL.
TBR=jam@chromium.org
BUG=226107
TEST=Tested missing WidevineCDM and Flash.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192751
Patch Set 1 #
Total comments: 17
Patch Set 2 : resolve comments #
Total comments: 2
Patch Set 3 : Updated flash mimetypes and extensions. #
Total comments: 7
Patch Set 4 : comment resolved #Patch Set 5 : Rebase and updated unittest. #
Messages
Total messages: 25 (0 generated)
PTAL https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( For flash, do we also want to get plugin type from extensions? https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.h:26: OTHER = 5, This is hacky. I don't want to touch the current histogram values but now it looks really odd. Thoughts?
https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( On 2013/04/03 23:36:55, xhwang wrote: > For flash, do we also want to get plugin type from extensions? +ilja
I think we would want to distinguish between ppapi and npapi Flash. Adding yzshen if he knows if this is done somewhere else already.
LGTM A general comment on the CL: The claim that plug-ins in the current enumeration are <1% of missing plug-in reports is...interesting. In our stats for plug-in *usage*, Flash alone makes up >90%, followed by the other plug-ins for a total very close to 100%. It would be interesting to find out where the skew is coming from. My guess is that it's pathological cases: For example, there is a Skype extension that injects a plug-in into almost every page, so if we fail to load that plug-in, it would be reported as missing for almost every page load. On 2013/04/03 23:52:55, ilja wrote: > I think we would want to distinguish between ppapi and npapi Flash. Well, this is called for the case where Flash is missing, so we don't have either. Likewise, when Flash is disabled we fall back to another enabled version, so we'd only report it as disabled if all versions of Flash are disabled. https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.h:26: OTHER = 5, On 2013/04/03 23:36:55, xhwang wrote: > This is hacky. I don't want to touch the current histogram values but now it > looks really odd. Thoughts? Haha, guess we should have initially defined OTHER as 0 ;-) Seriously though, I don't think it makes a huge difference, except that it appears in the middle of the definition instead of at the beginning or the end. The whole point of enums is to abstract away the actual values. On that note, you could remove the individual values here; they don't really add anything but noise IMO.
> I think we would want to distinguish between ppapi and npapi Flash. Adding > yzshen if he knows if this is done somewhere else already. As Bernhard said, when it is missing, we don't have either. And when it is not missing, we have Plugin.FlashUsage to know how many people are running PPAPI Flash / NPAPI Flash. Xiaohan: Could you please confirm whether plugins blocked by content settings "block" and "click-to-play" are counted as "missing"? If yes, we probably want to distinguish between "expected missing because of content settings" and "unexpected missing because of some mysterious failure".
https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( On 2013/04/03 23:36:55, xhwang wrote: > For flash, do we also want to get plugin type from extensions? I think we could add the extensions for Flash here, right? It is more consistent that way.
https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( On 2013/04/04 17:38:11, yzshen1 wrote: > On 2013/04/03 23:36:55, xhwang wrote: > > For flash, do we also want to get plugin type from extensions? > > I think we could add the extensions for Flash here, right? It is more consistent > that way. SGTM. Also, should we differentiate "OTHER" mime type from "UNRECOGNIZED_EXTENSION"? Those are very different buckets. https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.h:28: WIDEVINE_CDM = 7, If you're trying to reduce "other", I suggest also adding kBrowserPluginMimeType per my comments in the other CL.
https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:59: MissingPluginReporter::OTHER); You need to use your new PLUGIN_TYPE_MAX here, right? https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( On 2013/04/04 19:52:24, ddorwin wrote: > On 2013/04/04 17:38:11, yzshen1 wrote: > > On 2013/04/03 23:36:55, xhwang wrote: > > > For flash, do we also want to get plugin type from extensions? > > > > I think we could add the extensions for Flash here, right? It is more > consistent > > that way. > > SGTM. Also, should we differentiate "OTHER" mime type from > "UNRECOGNIZED_EXTENSION"? Those are very different buckets. Even better, (since the meaning of OTHER is/would change) we could deprecate the middle-of-the-list OTHER and replace it with OTHER_MIME_TYPE=254 and OTHER_FILE_EXTENSION=255.
On 2013/04/04 17:38:01, yzshen1 wrote: > > I think we would want to distinguish between ppapi and npapi Flash. Adding > > yzshen if he knows if this is done somewhere else already. > > As Bernhard said, when it is missing, we don't have either. And when it is not > missing, we have Plugin.FlashUsage to know how many people are running PPAPI > Flash / NPAPI Flash. > > > Xiaohan: Could you please confirm whether plugins blocked by content settings > "block" and "click-to-play" are counted as "missing"? If yes, we probably want > to distinguish between "expected missing because of content settings" and > "unexpected missing because of some mysterious failure". No, we report it as missing iff we cannot find a plug-in for the required MIME type/file extension all, and likewise as disabled iff the plug-in we choose to use is disabled.
I'll update the tests after I land 13414007 and rebase. PTAL https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:59: MissingPluginReporter::OTHER); On 2013/04/04 20:08:26, ddorwin wrote: > You need to use your new PLUGIN_TYPE_MAX here, right? Done. https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( On 2013/04/04 17:38:11, yzshen1 wrote: > On 2013/04/03 23:36:55, xhwang wrote: > > For flash, do we also want to get plugin type from extensions? > > I think we could add the extensions for Flash here, right? It is more consistent > that way. Done. Do we want to add more, e.g. fla et al? https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( On 2013/04/04 20:08:26, ddorwin wrote: > On 2013/04/04 19:52:24, ddorwin wrote: > > On 2013/04/04 17:38:11, yzshen1 wrote: > > > On 2013/04/03 23:36:55, xhwang wrote: > > > > For flash, do we also want to get plugin type from extensions? > > > > > > I think we could add the extensions for Flash here, right? It is more > > consistent > > > that way. > > > > SGTM. Also, should we differentiate "OTHER" mime type from > > "UNRECOGNIZED_EXTENSION"? Those are very different buckets. > > Even better, (since the meaning of OTHER is/would change) we could deprecate the > middle-of-the-list OTHER and replace it with OTHER_MIME_TYPE=254 and > OTHER_FILE_EXTENSION=255. Done. Use 240/241 in case we want to add more unsupported types. But this really doesn't matter as the limit is MAX_INT. https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.h:26: OTHER = 5, On 2013/04/04 07:01:56, Bernhard Bauer wrote: > On 2013/04/03 23:36:55, xhwang wrote: > > This is hacky. I don't want to touch the current histogram values but now it > > looks really odd. Thoughts? > > Haha, guess we should have initially defined OTHER as 0 ;-) > > Seriously though, I don't think it makes a huge difference, except that it > appears in the middle of the definition instead of at the beginning or the end. > The whole point of enums is to abstract away the actual values. Agreed. I don't want to change the existing histogram/counts in case some one/system is monitoring/depending-on them. > > On that note, you could remove the individual values here; they don't really add > anything but noise IMO. Do you mean to remove entries like REALPLAYER because few users are using them? For now I'd like to add entries instead of removing any before we have a good understanding what plugin types are missing. https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.h:28: WIDEVINE_CDM = 7, On 2013/04/04 19:52:24, ddorwin wrote: > If you're trying to reduce "other", I suggest also adding kBrowserPluginMimeType > per my comments in the other CL. Done.
LG except the open extension question. https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( On 2013/04/05 00:58:18, xhwang wrote: > On 2013/04/04 17:38:11, yzshen1 wrote: > > On 2013/04/03 23:36:55, xhwang wrote: > > > For flash, do we also want to get plugin type from extensions? > > > > I think we could add the extensions for Flash here, right? It is more > consistent > > that way. > > Done. Do we want to add more, e.g. fla et al? I coud be wrong, but it looks like we only support two: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro... https://codereview.chromium.org/13601002/diff/10001/chrome/renderer/plugins/p... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/10001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.h:28: OTHER = 5, // This is obsolete and replaced by incomplete sentence
https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.cc:120: MissingPluginReporter::PluginType MissingPluginReporter::SrcToPluginType( On 2013/04/05 04:21:37, ddorwin wrote: > On 2013/04/05 00:58:18, xhwang wrote: > > On 2013/04/04 17:38:11, yzshen1 wrote: > > > On 2013/04/03 23:36:55, xhwang wrote: > > > > For flash, do we also want to get plugin type from extensions? > > > > > > I think we could add the extensions for Flash here, right? It is more > > consistent > > > that way. > > > > Done. Do we want to add more, e.g. fla et al? > > I coud be wrong, but it looks like we only support two: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro... Done. https://codereview.chromium.org/13601002/diff/10001/chrome/renderer/plugins/p... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/10001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.h:28: OTHER = 5, // This is obsolete and replaced by On 2013/04/05 04:21:37, ddorwin wrote: > incomplete sentence Done.
LGTM, assuming the large gap doesn't cause problems. https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.cc:22: const char kFlashPluginSwfMimeType[] = "application/x-shockwave-flash"; Are we allowed to include webkit/plugins/plugin_constants.h? https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.cc:24: const char kWidevineCDMType[] = "application/x-ppapi-widevine-cdm"; Should we include widevine_cdm_common.h? https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.h:36: PLUGIN_TYPE_MAX On second thought, any idea if each slot consumes some type of resource or if we will end up with 231 empty values in the histogram? Otherwise, what's the point of the max value?
https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/1/chrome/renderer/plugins/plugi... chrome/renderer/plugins/plugin_uma.h:26: OTHER = 5, On 2013/04/05 00:58:18, xhwang wrote: > On 2013/04/04 07:01:56, Bernhard Bauer wrote: > > On 2013/04/03 23:36:55, xhwang wrote: > > > This is hacky. I don't want to touch the current histogram values but now it > > > looks really odd. Thoughts? > > > > Haha, guess we should have initially defined OTHER as 0 ;-) > > > > Seriously though, I don't think it makes a huge difference, except that it > > appears in the middle of the definition instead of at the beginning or the > end. > > The whole point of enums is to abstract away the actual values. > > Agreed. I don't want to change the existing histogram/counts in case some > one/system is monitoring/depending-on them. > > > > On that note, you could remove the individual values here; they don't really > add > > anything but noise IMO. > > Do you mean to remove entries like REALPLAYER because few users are using them? > For now I'd like to add entries instead of removing any before we have a good > understanding what plugin types are missing. No, I meant there is no need to write out the actual numerical values here if they're increasing by one anyway. https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.cc:22: const char kFlashPluginSwfMimeType[] = "application/x-shockwave-flash"; On 2013/04/05 05:32:17, ddorwin wrote: > Are we allowed to include webkit/plugins/plugin_constants.h? Yes.
LGTM Thanks!
https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... File chrome/renderer/plugins/plugin_uma.cc (right): https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.cc:22: const char kFlashPluginSwfMimeType[] = "application/x-shockwave-flash"; On 2013/04/05 05:32:17, ddorwin wrote: > Are we allowed to include webkit/plugins/plugin_constants.h? Done. https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.cc:24: const char kWidevineCDMType[] = "application/x-ppapi-widevine-cdm"; On 2013/04/05 05:32:17, ddorwin wrote: > Should we include widevine_cdm_common.h? Done. https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... File chrome/renderer/plugins/plugin_uma.h (right): https://codereview.chromium.org/13601002/diff/16001/chrome/renderer/plugins/p... chrome/renderer/plugins/plugin_uma.h:36: PLUGIN_TYPE_MAX On 2013/04/05 05:32:17, ddorwin wrote: > On second thought, any idea if each slot consumes some type of resource or if we > will end up with 231 empty values in the histogram? Otherwise, what's the point > of the max value? Moved UNSUPPORTED_* after OTHER to avoid the gap.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/13601002/26001
Presubmit check for 13601002-26001 failed and returned exit status 1.
INFO:root:Found 4 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/DEPS
jam@: could you please do an OWNERS review on the DEPS file?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/13601002/26001
Retried try job too often on ios_dbg_simulator for step(s) base_unittests, compile, content_unittests, crypto_unittests, googleurl_unittests, media_unittests, net_unittests, sql_unittests, ui_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/13601002/26001
Message was sent while issue was closed.
Change committed as 192751 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
