Record HTMLPluginElement::requestObject result.

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

 /**
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2000 Stefan Schimanski (1Stein@gmx.de)
  * Copyright (C) 2004, 2005, 2006 Apple Computer, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 27 matching lines @@
 #include "core/html/PluginDocument.h"
 #include "core/input/EventHandler.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "core/layout/LayoutImage.h"
 #include "core/layout/LayoutPart.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/loader/MixedContentChecker.h"
 #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"
 #include "platform/Widget.h"
 #include "platform/network/ResourceRequest.h"
 #include "platform/plugins/PluginData.h"
 #include "public/platform/WebURLRequest.h"
 
 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)
     // m_needsWidgetUpdate(!createdByParser) allows HTMLObjectElement to delay
     // widget updates until after all children are parsed. For HTMLEmbedElement
     // this delay is unnecessary, but it is simpler to make both classes share
     // the same codepath in this class.
     , m_needsWidgetUpdate(!createdByParser)
     , m_shouldPreferPlugInsForImages(preferPlugInsForImagesOption == ShouldPreferPlugInsForImages)
 {
@@ skipping 367 matching lines @@
 // that <object> uses depending on <param> values.
 bool HTMLPlugInElement::wouldLoadAsNetscapePlugin(const String& url, const String& serviceType)
 {
     ASSERT(document().frame());
     KURL completedURL;
     if (!url.isEmpty())
         completedURL = document().completeURL(url);
     return document().frame()->loader().client()->getObjectContentType(completedURL, serviceType, shouldPreferPlugInsForImages()) == ObjectContentNetscapePlugin;
 }
 
 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)) {
         // If the plugin element already contains a subframe,
         // 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)
 {
     if (!allowedToLoadPlugin(url, mimeType))
         return false;
 
     LocalFrame* frame = document().frame();
     if (!frame->loader().allowPlugins(AboutToInstantiatePlugin))
         return false;
@@ skipping 116 matching lines @@
 
 void HTMLPlugInElement::lazyReattachIfNeeded()
 {
     if (!useFallbackContent() && needsWidgetUpdate() && layoutObject() && !isImageType()) {
         lazyReattachIfAttached();
         setPersistedPluginWidget(nullptr);
     }
 }
 
 } // namespace blink