Add UMA stats for pinch-zoom behavior on Android

third_party/WebKit/Source/core/frame/VisualViewport.cpp

Index: third_party/WebKit/Source/core/frame/VisualViewport.cpp
diff --git a/third_party/WebKit/Source/core/frame/VisualViewport.cpp b/third_party/WebKit/Source/core/frame/VisualViewport.cpp
index d6472c65beed2a7c3115a1f23f3a9c20361bebe0..1172fab5091b7af5b5dd26ed62b8a64a37b4cee3 100644
--- a/third_party/WebKit/Source/core/frame/VisualViewport.cpp
+++ b/third_party/WebKit/Source/core/frame/VisualViewport.cpp
@@ -71,12 +71,15 @@ VisualViewport::VisualViewport(FrameHost& owner)
     : m_frameHost(&owner)
     , m_scale(1)
     , m_topControlsAdjustment(0)
+    , m_maxPageScale(-1)
+    , m_trackPinchZoomStatsForPage(false)
 {
     reset();
 }
 
 VisualViewport::~VisualViewport()
 {
+    sendUMAMetrics();

[Rick Byers, 2015/09/28 18:43:58]

Is there already an established pattern for renderer stats sent per page load? 
It seems like it's likely to be fraught with edge cases.  Some small (un-biased)
information loss (like tab discarding) is probably fine here.  But if you
weren't reporting a substantial fraction then I'd worry about the validity of
the data.

Maybe just do a quick manual sanity check that doing 20 page loads (navigating
between real-world pages) reports this exactly 20 times?  

[bokan, 2015/09/29 20:33:20]

I copied this pattern from the Blink useCounter which reports metrics on Page
destruction (VisualViewport is destroyed with the page) and on commit so we
should have the same profile as that. I'd tried this earlier and it seemed to
work as expected but I just double checked and it seems to only report when
following links, not new navigations. I'll investigate a bit more.

[bokan, 2015/10/01 20:58:15]

Right, so this wont report stats for a page if the user navigates away with the
URL bar. Looks like as soon as we navigate to a new URL, the old process is
killed without getting a chance to do any cleanup and a new one is started up.

This means we only get stats for navigation using links and history which seems
unfortunate. Do we have any data on how often users navigate away using the URL
bar vs links? I think we'll lose a pretty significant chunk of page loads this
way (I'd expect ~10-25%).

The only option I can see, since the renderer gets killed pretty ruthlessly, is
something like what
https://docs.google.com/document/u/2/d/1HJsJ5W2H_3qRdqPAUgAEo10AF8gXPTXZLUET4...
is suggesting - periodically passing the data to the Browser and letting it
decide when to upload the metrics. That seems pretty heavy weight for this
though.

Thoughts?

[Rick Byers, 2015/10/02 00:53:22]

Ugh, that's a pain.  I didn't realize UseCounter data was this bad.  Have you
found a bug for that (beyond the page-load-time specific bug 382542)?  Fixing
that seems more important than this specific instance, and maybe there's some
way to fix it in a general way.  If there's not already a bug for that, could
you file one?

Does the same issue apply for cross-origin navigation via links (which I believe
also tears down the old renderer pretty quickly)?  If so then I could imagine
whole classes of appy sites might not get logged at all (eg. when do you ever
leave a facebook.com or gmail.com page in a way that WOULD report data?).

But I agree that it's not worth doing something special case here.  I think you
should either leave it as-is and live with the inaccuracy, or change what you're
looking at.  Eg. what if we just recorded the scale at the end of every pinch
gesture, perhaps that would be good enough?  I guess you still wouldn't know
what fraction of pages get a pinch at all.

 }
 
 DEFINE_TRACE(VisualViewport)
@@ -654,6 +657,55 @@ IntPoint VisualViewport::rootFrameToViewport(const IntPoint& pointInRootFrame) c
     return flooredIntPoint(FloatPoint(rootFrameToViewport(FloatPoint(pointInRootFrame))));
 }
 
+void VisualViewport::startTrackingPinchStats()
+{
+#if !OS(ANDROID)
+    return;

[Rick Byers, 2015/09/28 18:43:58]

Always sucks to add more #if OS(ANDROID (eg. makes testing harder, makes mobile
emulation subtly different than real mobile).  See below.

[bokan, 2015/10/01 20:18:31]

Gone in latest patch.

+#endif
+
+    if (!mainFrame())
+        return;
+
+    Document* document = mainFrame()->document();
+    if (!document)
+        return;
+
+    if (!document->url().protocolIsInHTTPFamily())
+        return;
+
+    const ViewportDescription& desc = mainFrame()->document()->viewportDescription();
+    bool isNonMobile =

[Rick Byers, 2015/09/28 18:43:58]

I thought this logic (along with "am I on Android") was already encapsulated
somewhere else ("shouldEnableDesktopWorkarounds" or some such function somewhere
perhaps)?  It would be nice to use a consistent definition, since the details
are a little subtle.

[bokan, 2015/09/29 20:33:20]

WebViewImpl::shouldDisableDesktopWorkarounds. It's slightly different than what
we agreed on - in that case, the page is "mobile" iff the layout size ==
viewport size OR the author explicitly disabled zoom. That said, I think it
would work as well. Again, Yoav: what do you think?

[bokan, 2015/10/01 20:18:31]

Latest patch replaces this with the shouldDisableDesktopWorkarounds call (I
moved it from WebViewImpl to here). Yoav, does that sound ok?

+        !desc.isSpecifiedByAuthor()
+        || (desc.isMetaViewportType() && desc.maxWidth.type() == blink::Fixed);
+
+    m_trackPinchZoomStatsForPage = isNonMobile;
+}
+
+void VisualViewport::userDidChangeScale()
+{
+    if (!m_trackPinchZoomStatsForPage)
+        return;
+
+    m_maxPageScale = std::max(m_maxPageScale, m_scale);

[Rick Byers, 2015/09/28 18:43:58]

I assume this gets called only at GesturePinchEnd time (not for each scale
change throughout the pinch gesture), right?  Is that the notion of "max" you
want?

[bokan, 2015/09/29 20:33:20]

Good point. I do think capturing the scale only at end is probably the right
thing here but maybe Yoav can confirm.

+}
+
+void VisualViewport::sendUMAMetrics()
+{
+    if (m_trackPinchZoomStatsForPage) {
+        bool didScale = m_maxPageScale > 0;
+
+        Platform::current()->histogramEnumeration("Viewport.DidScalePage", didScale ? 1 : 0, 2);
+
+        if (didScale) {
+            int zoomPercentage = floor(m_maxPageScale * 100);
+            Platform::current()->histogramEnumeration("Viewport.MaxPageScale", zoomPercentage, 501);

[Ilya Sherman, 2015/09/30 04:30:28]

This allocates a histogram with 500 buckets, which is pretty wasteful of memory.
 Is it the case that any page scale between 1 and 500 is possible, or are there
only a few that are actually possible?  In either case, you might want to use a
sparse histogram, which only allocates the used buckets on the client.  If there
are only a few, then you could also use a regular enumerated histogram, but one
where you manually map the page scales onto a much smaller enum.

[bokan, 2015/10/01 20:18:31]

Ok, I mapped the percentages into ~20 buckets comprising 25% ranges - that
should be fine enough for our needs.

+        }
+    }
+
+    m_maxPageScale = -1;
+    m_trackPinchZoomStatsForPage = false;
+}
+
 String VisualViewport::debugName(const GraphicsLayer* graphicsLayer)
 {
     String name;