Record HTMLPluginElement::requestObject result.

third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
index 52e341805564d0756a265dadfbee14252fec77e1..d926dc1c35d2adde3ee9f76cf06cd4cb1a8c73c5 100644
--- a/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp
@@ -45,6 +45,7 @@
 #include "core/page/Page.h"
 #include "core/page/scrolling/ScrollingCoordinator.h"
 #include "core/plugins/PluginView.h"
+#include "platform/Histogram.h"
 #include "platform/Logging.h"
 #include "platform/MIMETypeFromURL.h"
 #include "platform/MIMETypeRegistry.h"
@@ -57,6 +58,18 @@ namespace blink {
 
 using namespace HTMLNames;
 
+namespace {
+
+// Used for histograms, do not change the order.
+enum PluginRequestObjectResult {
+    PluginRequestObjectResultFailure = 0,
+    PluginRequestObjectResultSuccess = 1,
+    // Keep at the end.
+    PluginRequestObjectResultMax
+};
+
+} // anonymous namespace
+
 HTMLPlugInElement::HTMLPlugInElement(const QualifiedName& tagName, Document& doc, bool createdByParser, PreferPlugInsForImagesOption preferPlugInsForImagesOption)
     : HTMLFrameOwnerElement(tagName, doc)
     , m_isDelayingLoadEvent(false)
@@ -446,15 +459,17 @@ bool HTMLPlugInElement::wouldLoadAsNetscapePlugin(const String& url, const Strin
 
 bool HTMLPlugInElement::requestObject(const String& url, const String& mimeType, const Vector<String>& paramNames, const Vector<String>& paramValues)

[pdr., 2016/08/09 17:45:45]

I don't think gotos are inherently bad but we rarely use them in blink (outside
of performance-critical code) so I'd be worried they will cause refactoring bugs
in the future. Can you use a private function to avoid using goto?
E.g., PluginRequestObjectResult HTMLPluginElement::requestObjectInternal(...)

This will also let you avoid the conversions to/from enum/bool since you can
write:
bool HTMLPlugInElement::requestObject(...)
{
    PluginRequestObjectResult result = requestObjectInternal(...);

    DEFINE_STATIC_LOCAL(EnumerationHistogram, resultHistogram,
("Plugin.RequestObjectResult", PluginRequestObjectResultMax));
    resultHistogram.count(result);

    if (result == PluginRequestObjectResultFailure)
        return false;
    return true;
}

[mlamouri (slow - plz ping), 2016/08/09 18:20:20]

I don't think returning PLuginRequestObjectResult really helps because the
requestObjectInternal() uses load*() methods which return bool so we would end
up doing more conversions.

This said, I applied your comments.

 {
+    bool result = false;
+    KURL completedURL = url.isEmpty() ? KURL() : document().completeURL(url);
+
     if (url.isEmpty() && mimeType.isEmpty())
-        return false;
+        goto reportAndReturn;
 
     if (protocolIsJavaScript(url))
-        return false;
+        goto reportAndReturn;
 
-    KURL completedURL = url.isEmpty() ? KURL() : document().completeURL(url);
     if (!allowedToLoadObject(completedURL, mimeType))
-        return false;
+        goto reportAndReturn;
 
     bool useFallback;
     if (!shouldUsePlugin(completedURL, mimeType, hasFallbackContent(), useFallback)) {
@@ -462,10 +477,17 @@ bool HTMLPlugInElement::requestObject(const String& url, const String& mimeType,
         // loadOrRedirectSubframe will re-use it. Otherwise, it will create a
         // new frame and set it as the LayoutPart's widget, causing what was
         // previously in the widget to be torn down.
-        return loadOrRedirectSubframe(completedURL, getNameAttribute(), true);
+        result = loadOrRedirectSubframe(completedURL, getNameAttribute(), true);
+        goto reportAndReturn;
     }
 
-    return loadPlugin(completedURL, mimeType, paramNames, paramValues, useFallback, true);
+    result = loadPlugin(completedURL, mimeType, paramNames, paramValues, useFallback, true);
+
+reportAndReturn:
+    DEFINE_STATIC_LOCAL(EnumerationHistogram, resultHistogram, ("Plugin.RequestObjectResult", PluginRequestObjectResultMax));
+    resultHistogram.count(result ? PluginRequestObjectResultSuccess : PluginRequestObjectResultFailure);
+
+    return result;
 }
 
 bool HTMLPlugInElement::loadPlugin(const KURL& url, const String& mimeType, const Vector<String>& paramNames, const Vector<String>& paramValues, bool useFallback, bool requireLayoutObject)