Force all experiment enabled checks to use ExperimentalFeatures

third_party/WebKit/Source/core/experiments/Experiments.cpp

 // Copyright 2015 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 "core/experiments/Experiments.h"
 
 #include "core/dom/ElementTraversal.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/html/HTMLHeadElement.h"
 #include "core/html/HTMLMetaElement.h"
@@ skipping 10 matching lines @@
 {
     return executionContext->securityOrigin()->toString();
 }
 
 String getDisabledMessage(const String& apiName)
 {
     // TODO(chasej): Update message with URL to experiments site, when live
     return "The '" + apiName + "' API is currently enabled in limited experiments. Please see [Chrome experiments website URL] for information on enabling this experiment on your site.";
 }
 
-bool hasValidAPIKey(ExecutionContext* executionContext, const String& apiName, String& errorMessage)
+bool hasValidAPIKey(ExecutionContext* executionContext, const String& apiName, String* errorMessage)
 {
     bool foundAnyKey = false;
     String origin = getCurrentOrigin(executionContext);
 
     // When in a document, the API key is provided in a meta tag
     if (executionContext->isDocument()) {
         HTMLHeadElement* head = toDocument(executionContext)->head();
         for (HTMLMetaElement* metaElement = head ? Traversal<HTMLMetaElement>::firstChild(*head) : 0; metaElement; metaElement = Traversal<HTMLMetaElement>::nextSibling(*metaElement)) {
             if (equalIgnoringCase(metaElement->name(), kExperimentsMetaName)) {
                 foundAnyKey = true;
                 if (equalIgnoringCase(metaElement->content(), apiName))
                     return true;
             }
         }
     }
-    if (foundAnyKey) {
-        errorMessage = "The provided key(s) are not valid for the '" + apiName + "' API.";
-    } else {
-        errorMessage = getDisabledMessage(apiName);
+
+    if (errorMessage) {

[iclelland, 2016/01/08 15:11:28]

Would the code be cleaner if you did the same thing that
ExecutionContext::isSecureContext() does here -- if null, then reassign the
errorMessage pointer to a new string, and let it get destroyed when the method
returns? Then all of the "if(errorMessage)" blocks, and the ternary below, could
be eliminated.

[chasej, 2016/01/08 17:37:06]

I did consider that the code is less clean with the extra "if (errormessage)"
checks. I think it's a reasonable trade-off, to avoid the extra allocation for a
string that would never be used. Of course, I haven't measured, so the benefit
could be negligible.

[iclelland, 2016/01/08 17:50:09]

Well, if you get to the point where you're calling isSecureContext(), it's going
to allocate that string anyway, so the only potential performance hit is in the
error case.

+        if (foundAnyKey) {
+            *errorMessage = "The provided key(s) are not valid for the '" + apiName + "' API.";
+        } else {
+            *errorMessage = getDisabledMessage(apiName);
+        }
     }
     return false;
 }
 
 } // namespace
 
-bool Experiments::isApiEnabled(ExecutionContext* executionContext, const String& apiName, String& errorMessage)
+// static
+bool Experiments::isApiEnabled(ExecutionContext* executionContext, const String& apiName, String* errorMessage)
 {
     if (!RuntimeEnabledFeatures::experimentalFrameworkEnabled()) {
-        errorMessage = "Experimental Framework is not enabled.";
+        if (errorMessage) {
+            *errorMessage = "Experimental Framework is not enabled.";
+        }
         return false;
     }
 
     if (!executionContext) {
         ASSERT_NOT_REACHED();
         return false;
     }
 
     // Experiments are only enabled for secure origins
-    if (!executionContext->isSecureContext(errorMessage)) {
+    bool isSecure = errorMessage
+        ? executionContext->isSecureContext(*errorMessage)
+        : executionContext->isSecureContext();
+    if (!isSecure) {
         return false;
     }
 
     return hasValidAPIKey(executionContext, apiName, errorMessage);
 }
 
+// static
 DOMException* Experiments::createApiDisabledException(const String& apiName)
 {
     return DOMException::create(NotSupportedError, getDisabledMessage(apiName));
 }
 
 } // namespace blink