Plugin Power Saver: Collect size histogram on Flash Plugins users click.

content/renderer/pepper/pepper_plugin_instance_impl.cc

Index: content/renderer/pepper/pepper_plugin_instance_impl.cc
diff --git a/content/renderer/pepper/pepper_plugin_instance_impl.cc b/content/renderer/pepper/pepper_plugin_instance_impl.cc
index 4efdbb6458c958fbda35c459dc4fe58886c3ee59..9e620b47f48d3632c4bf9f2441ac84c900f51a28 100644
--- a/content/renderer/pepper/pepper_plugin_instance_impl.cc
+++ b/content/renderer/pepper/pepper_plugin_instance_impl.cc
@@ -12,6 +12,7 @@
 #include "base/memory/linked_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "base/metrics/sparse_histogram.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_offset_string_conversions.h"
@@ -186,6 +187,13 @@ namespace content {
 
 namespace {
 
+static const int kInfiniteRatio = 99999;
+
+#define UMA_HISTOGRAM_ASPECT_RATIO(name, width, height) \
+    UMA_HISTOGRAM_SPARSE_SLOWLY( \

[groby-ooo-7-16, 2014/11/07 21:57:44]

I think  we need cutoffs - beyond 3:1 or 4:1 it's becoming really uninteresting
to sample more details. Also, the effective granularity for aspect ratios < 1 is
worse than the other way around.

So, how about this for the value:

// flip ratio to always be >1
if (height > width) 
  swapped = true;
  std::swap(width, height)
}

// Clamp ratio at 400, 401 is anything > 4:1
if (height)
  ratio = min(((width * 100) / height), 400)
else
  ratio = 401;

// Flag swapped values
if (swapped) 
  ratio = -ratio;

Or, if you want to be concise:
ratio = (min(width,height) ? min(max(width, height) / min(width,height), 400) :
401) * ((width < height) ? -1 : 1); // I'd rather you're not concise and spell
this out in the recording function :)

[tommycli, 2014/11/07 22:46:37]

I used the same aspect ratio encoding as here:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

I was planning to consolidate the two macros in a followup CL.

[groby-ooo-7-16, 2014/11/07 23:02:20]

These are video aspect ratios - they're rarely flipped (i.e. <1), so they don't
need to tackle that problem.  Essentially, they're expected to be 4:3/16:9
pretty much all the time. Hm.

We _expect_ that to be the case as well, so we could indeed re-use that macro.
If we're outside that range, the data will just be slightly less useful. Since
it promotes re-use, let's re-use.  

[tommycli, 2014/11/07 23:14:10]

Done.

+        name, \
+        (height) ? ((width) * 100) / (height) : kInfiniteRatio);
+
 // Check PP_TextInput_Type and ui::TextInputType are kept in sync.
 COMPILE_ASSERT(int(ui::TEXT_INPUT_TYPE_NONE) == int(PP_TEXTINPUT_TYPE_NONE),
                mismatching_enums);
@@ -391,6 +399,14 @@ void InitLatencyInfo(ui::LatencyInfo* new_latency,
   }
 }
 
+enum PluginFlashInstanceSize {
+  INSTANCE_SIZE_1_1 = 0,
+  INSTANCE_SIZE_5_5 = 1,
+  INSTANCE_SIZE_10_10 = 2,
+  INSTANCE_SIZE_LARGE = 3,
+  INSTANCE_SIZE_NUM_ITEMS
+};
+
 // How the throttled power saver is unthrottled, if ever.
 // These numeric values are used in UMA logs; do not change them.
 enum PowerSaverUnthrottleMethod {
@@ -400,8 +416,36 @@ enum PowerSaverUnthrottleMethod {
   UNTHROTTLE_METHOD_NUM_ITEMS
 };
 
+const char kFlashClickSizeAspectRatioHistogram[] =
+    "Plugin.FlashClickSizeAspectRatio";

[groby-ooo-7-16, 2014/11/07 21:57:44]

Plugin.Flash.ClickSize.AspectRatio ? (And the same for the other metrics). We
seem to usually treat names as a hierarchy of namespaces, so this seems a better
name. 

[tommycli, 2014/11/07 22:46:37]

Done.

+const char kFlashClickSizeHeightHistogram[] = "Plugin.FlashClickSizeHeight";
+const char kFlashClickSizeWidthHistogram[] = "Plugin.FlashClickSizeWidth";
+const char kFlashInstanceSizeHistogram[] = "Plugin.FlashInstanceSize";
 const char kPowerSaverUnthrottleHistogram[] = "Plugin.PowerSaverUnthrottle";
 
+void RecordFlashSizeMetric(int width, int height) {
+  if (width <= 1 && height <= 1) {
+    UMA_HISTOGRAM_ENUMERATION(kFlashInstanceSizeHistogram, INSTANCE_SIZE_1_1,
+                              INSTANCE_SIZE_NUM_ITEMS);
+  } else if (width <= 5 && height <= 5) {
+    UMA_HISTOGRAM_ENUMERATION(kFlashInstanceSizeHistogram, INSTANCE_SIZE_5_5,
+                              INSTANCE_SIZE_NUM_ITEMS);
+  } else if (width <= 10 && height <= 10) {
+    UMA_HISTOGRAM_ENUMERATION(kFlashInstanceSizeHistogram, INSTANCE_SIZE_10_10,
+                              INSTANCE_SIZE_NUM_ITEMS);
+  } else {
+    UMA_HISTOGRAM_ENUMERATION(kFlashInstanceSizeHistogram, INSTANCE_SIZE_LARGE,
+                              INSTANCE_SIZE_NUM_ITEMS);
+  }
+}
+
+void RecordFlashClickSizeMetric(int width, int height) {
+  UMA_HISTOGRAM_COUNTS(kFlashClickSizeHeightHistogram, height);

[groby-ooo-7-16, 2014/11/07 21:57:44]

I believe these are exponential histograms. We want a linear one for all of
them. Something like

  base::HistogramBase* histogram =
        base::LinearHistogram::FactoryGet(
            kFlashClickSizeHeightHistogram
            200, // minimum height
            400, // maximum height
            50,  // number of buckets.
            base::HistogramBase::kUmaTargetedHistogramFlag);
    histogram->Add(height);

[tommycli, 2014/11/07 22:46:37]

Done.

I followed this example:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Are they 'doing it wrong' then?

[groby-ooo-7-16, 2014/11/07 23:02:20]

They're not "doing it wrong", but they might be interested in different data. If
you look at their width/height metrics, you'll clearly see the exponential
bucketing, which might serve the needs they have.

We're interested in getting a fairly exact reading of the cutoff point, so
having things vanish in large buckets would not be helpful. (Also, their range
is bigger than ours - we want a narrow sample around the cutoff point, they
probably want an overview)

[tommycli, 2014/11/07 23:14:10]

Done. Good point.

+  UMA_HISTOGRAM_COUNTS(kFlashClickSizeWidthHistogram, width);
+  UMA_HISTOGRAM_ASPECT_RATIO(kFlashClickSizeAspectRatioHistogram, width,
+                             height);
+}
+
 void RecordUnthrottleMethodMetric(PowerSaverUnthrottleMethod method) {
   UMA_HISTOGRAM_ENUMERATION(kPowerSaverUnthrottleHistogram, method,
                             UNTHROTTLE_METHOD_NUM_ITEMS);
@@ -511,6 +555,7 @@ PepperPluginInstanceImpl::PepperPluginInstanceImpl(
       layer_bound_to_fullscreen_(false),
       layer_is_hardware_(false),
       plugin_url_(plugin_url),
+      has_been_clicked_(false),
       power_saver_enabled_(false),
       is_peripheral_content_(false),
       plugin_throttled_(false),
@@ -606,11 +651,6 @@ PepperPluginInstanceImpl::PepperPluginInstanceImpl(
   if (GetContentClient()->renderer() &&  // NULL in unit tests.
       GetContentClient()->renderer()->IsExternalPepperPlugin(module->name()))
     external_document_load_ = true;
-
-  if (IsFlashPlugin(module_.get())) {
-    RenderThread::Get()->RecordAction(
-        base::UserMetricsAction("Flash.PluginInstanceCreated"));
-  }
 }
 
 PepperPluginInstanceImpl::~PepperPluginInstanceImpl() {
@@ -871,10 +911,16 @@ bool PepperPluginInstanceImpl::Initialize(
   if (!render_frame_)
     return false;
 
+  blink::WebRect bounds = container_->element().boundsInViewportSpace();
+  if (IsFlashPlugin(module_.get())) {

[groby-ooo-7-16, 2014/11/07 21:57:44]

Curious: Why did this move? To do all recording at the same time?

[tommycli, 2014/11/07 22:46:38]

Yes. And it had to be here because you can only get the bounds after the
constructor finishes.

+    RenderThread::Get()->RecordAction(
+        base::UserMetricsAction("Flash.PluginInstanceCreated"));
+    RecordFlashSizeMetric(bounds.width, bounds.height);
+  }
+
   PluginPowerSaverHelper* power_saver_helper =
       render_frame_->plugin_power_saver_helper();
   GURL content_origin = plugin_url_.GetOrigin();
-  blink::WebRect bounds = container_->element().boundsInViewportSpace();
 
   bool cross_origin = false;
   is_peripheral_content_ =
@@ -1158,6 +1204,13 @@ bool PepperPluginInstanceImpl::HandleInputEvent(
     WebCursorInfo* cursor_info) {
   TRACE_EVENT0("ppapi", "PepperPluginInstanceImpl::HandleInputEvent");
 
+  if (event.type == blink::WebInputEvent::MouseDown && !has_been_clicked_ &&
+      IsFlashPlugin(module_.get())) {
+    has_been_clicked_ = true;
+    blink::WebRect bounds = container_->element().boundsInViewportSpace();
+    RecordFlashClickSizeMetric(bounds.width, bounds.height);
+  }
+
   if (event.type == blink::WebInputEvent::MouseUp && is_peripheral_content_) {
     is_peripheral_content_ = false;
     power_saver_enabled_ = false;