Force all experiment enabled checks to use ExperimentalFeatures

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

Index: third_party/WebKit/Source/core/experiments/Experiments.cpp
diff --git a/third_party/WebKit/Source/core/experiments/Experiments.cpp b/third_party/WebKit/Source/core/experiments/Experiments.cpp
index b3274eef793e9c161bd66d48f8e46db1907d5dd6..011e381d9c648ab5d0cc4636e5749d27e26e3b4b 100644
--- a/third_party/WebKit/Source/core/experiments/Experiments.cpp
+++ b/third_party/WebKit/Source/core/experiments/Experiments.cpp
@@ -28,7 +28,7 @@ String getDisabledMessage(const String& apiName)
     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);
@@ -44,20 +44,26 @@ bool hasValidAPIKey(ExecutionContext* executionContext, const String& apiName, S
             }
         }
     }
-    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;
     }
 
@@ -67,13 +73,17 @@ bool Experiments::isApiEnabled(ExecutionContext* executionContext, const String&
     }
 
     // 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));