media: Add UMA to report CDM interface version being used

media/cdm/ppapi/ppapi_cdm_adapter.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/cdm/ppapi/ppapi_cdm_adapter.h"
 
 #include <string.h>
 
 #include "base/macros.h"
 #include "build/build_config.h"
@@ skipping 337 matching lines @@
   // The Pepper plugin will be staticly linked to the CDM, so pass the plugin's
   // CreateCdmInstance() to CdmWrapper.
   CdmWrapper* cdm = CdmWrapper::Create(::CreateCdmInstance, key_system.data(),
                                        key_system.size(), GetCdmHost, this);
 
   const std::string message = "CDM instance for " + key_system +
                               (cdm ? "" : " could not be") + " created.";
   DLOG_TO_CONSOLE(message);
   CDM_DLOG() << message;
 
+  if (cdm) {
+    pp::UMAPrivate uma_interface(this);
+    uma_interface.HistogramEnumeration(
+        "Media.EME.CdmVersion", cdm->GetVersion(),

[ddorwin, 2016/08/22 20:38:15]

Update this name too.

[xhwang, 2016/08/23 00:21:43]

Done.

+        cdm::ContentDecryptionModule::kVersion + 1);

[xhwang, 2016/08/22 17:59:38]

Just FYI: cdm::ContentDecryptionModule::kVersion is 8 for now. cdm->GetVersion()
can return 7 or 8. In the future, cdm::ContentDecryptionModule::kVersion could
grow, e.g. to 9, and cdm->GetVersion() can return some integer that is <= 9.

[ddorwin, 2016/08/22 20:38:15]

What is the +1 doing here? It might be worth commenting this.

Currently, it is set to the max version supported by the adapter/Chrome + 1. Why
is that?

[Ilya Sherman, 2016/08/22 20:53:36]

Could this be recorded as a sparse histogram?

[xhwang, 2016/08/22 21:51:22]

This will be a very limited set. For now, it'll only be 7 and 8. In the future,
it'll be something like 8 and 9, then 9 and 10 etc. Do you suggest using a
sparse histogram for this case?

BTW, I am using the pepper interface to report UMA. It seems like this interface
doesn't support sparse histogram yet :(

[Ilya Sherman, 2016/08/22 22:10:38]

It's okay to use a dense histogram as long as the values are small.  If you
prefer that approach, could you please add an assertion to verify that the
version number is not very large, and a comment to document why that assertion
is in place?

[xhwang, 2016/08/23 00:21:43]

Done.

[xhwang, 2016/08/23 00:21:43]

This part is a bit confusing. The doc says that the "value" should be strictly
less than the "boundary", hence the +1:

https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?rcl=0&l=253

I added a comment here.

+  }
+
   return cdm;
 }
 
 void PpapiCdmAdapter::Initialize(uint32_t promise_id,
                                  const std::string& key_system,
                                  bool allow_distinctive_identifier,
                                  bool allow_persistent_state) {
   PP_DCHECK(!key_system.empty());
   PP_DCHECK(!cdm_);
 
@@ skipping 949 matching lines @@
 }  // namespace media
 
 namespace pp {
 
 // Factory function for your specialization of the Module object.
 Module* CreateModule() {
   return new media::PpapiCdmAdapterModule();
 }
 
 }  // namespace pp