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..33487acf448d0cfb5459cc166f735c66cfd17eb0 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,38 @@ 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) {
+        if (foundAnyKey) {
+            *errorMessage = "The provided key(s) are not valid for the '" + apiName + "' API.";
+        } else {
+            *errorMessage = getDisabledMessage(apiName);
+        }
     }
     return false;
 }
 
 } // namespace
 
+// static
 bool Experiments::isApiEnabled(ExecutionContext* executionContext, const String& apiName, String& errorMessage)
 {
+    return isApiEnabledImpl(executionContext, apiName, &errorMessage);
+}
+
+// static
+bool Experiments::isApiEnabled(ExecutionContext* executionContext, const String& apiName)

[iclelland, 2015/12/23 03:56:12]

This method isn't used anywhere outside of the ExperimentalFeatures class -- do
we need to maintain this version? Could we get away with just the impl here, 
and make it public to call it directly from ExperimentalFeatures::*impl? I think
that would simplify those methods a bit.

Alternately, if we're committed to *not* having regular code call the raw
Experiments::isApiEnabled methods, then we could leave the impl private, and
make blink::ExperimentalFeatures a friend; that would make sure that someone has
to do something really obvious if they're going to bypass the
ExperimentalFeatures::* methods.

[chasej, 2015/12/23 15:58:24]

At this point, I don't believe there should be any regular code calling the raw
isApiEnabled() methods. Other than unit tests, all the regular callers will need
the RuntimeEnabled check included in the generated xxxEnabled methods.  The
public isApiEnabled() methods can be removed. I agree with keeping the impl
private, to avoid inadvertent bypassing of ExperimentalFeatures::*.

If not for the fact that ExperimentalFeatures is generated, I would say that
Experiments::isApiEnabledImpl should be part of that class.  To support code
generation and unit tests, we could use the friend approach as you suggested. 
The other option is to make Experiments a base class for ExperimentalFeatures
(subject to renaming).  I believe that achieves the same goal of making it
obvious if someone was to bypass ExperimentalFeatures::*.

What are your thoughts on defining ExperimentalFeatures as friend vs making
Experiments a base class?

+{
+    return isApiEnabledImpl(executionContext, apiName, nullptr);
+}
+
+// static
+bool Experiments::isApiEnabledImpl(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 +85,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));