Force all experiment enabled checks to use ExperimentalFeatures

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

Index: third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp
diff --git a/third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp b/third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp
index d6138e20ce6558daa95aba12a92f1384547dd012..bb032f9edc98ed77f3eedef9b1bb5ae5601fc6ef 100644
--- a/third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp
+++ b/third_party/WebKit/Source/core/experiments/ExperimentsTest.cpp
@@ -24,6 +24,8 @@ const char* kFrobulateAPIName = "Frobulate";
 const char* kFrobulateEnabledOrigin = "https://www.example.com";
 const char* kFrobulateEnabledOriginUnsecure = "http://www.example.com";
 
+} // namespace
+
 class ExperimentsTest : public ::testing::Test {
 protected:
     ExperimentsTest()
@@ -83,13 +85,18 @@ protected:
         head->appendChild(meta.release());
     }
 
-    bool isApiEnabled(const String& origin, const String& apiName, const char* apiKeyValue, String& errorMessage)
+    bool isApiEnabled(const String& origin, const String& apiName, const char* apiKeyValue, String* errorMessage)
     {
         setPageOrigin(origin);
         addApiKey(apiKeyValue);
         return Experiments::isApiEnabled(executionContext(), apiName, errorMessage);
     }
 
+    bool isApiEnabledWithoutErrorMessage(const String& origin, const String& apiName, const char* apiKeyValue)
+    {
+        return isApiEnabled(origin, apiName, apiKeyValue, nullptr);
+    }
+
 private:
     OwnPtr<DummyPageHolder> m_page;
     RefPtrWillBePersistent<HTMLDocument> m_document;
@@ -102,11 +109,20 @@ TEST_F(ExperimentsTest, EnabledNonExistingAPI)
     bool isNonExistingApiEnabled = isApiEnabled(kFrobulateEnabledOrigin,
         kNonExistingAPIName,
         kFrobulateAPIName /* Use existing api name as the key value */,
-        errorMessage);
+        &errorMessage);
     EXPECT_FALSE(isNonExistingApiEnabled);
     EXPECT_EQ(("The provided key(s) are not valid for the 'This API does not exist' API."), errorMessage);

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

Maybe make this a kExpectedErrorMessage constant at the top of the file, since
it's very closely tied to kNonExistingAPIName.

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

True, they are closely related.  Given the expected error message is only used
once, does it improve the readability of the test to define a constant? Is there
any consensus on style for expected values in tests (that are used only once)?

[iclelland, 2016/01/08 17:59:38]

I think it makes it a little clearer where the magic string value comes from,
but I don't know about any definitive style arguments here. I'm really fine
either way.

 }
 
+TEST_F(ExperimentsTest, EnabledNonExistingAPIWithoutErrorMessage)
+{
+    bool isNonExistingApiEnabled = isApiEnabledWithoutErrorMessage(
+        kFrobulateEnabledOrigin,
+        kNonExistingAPIName,
+        kFrobulateAPIName /* Use existing api name as the key value */);
+    EXPECT_FALSE(isNonExistingApiEnabled);
+}
+
 // The API should be enabled if a valid key for the origin is provided
 TEST_F(ExperimentsTest, EnabledSecureRegisteredOrigin)
 {
@@ -114,11 +130,20 @@ TEST_F(ExperimentsTest, EnabledSecureRegisteredOrigin)
     bool isOriginEnabled = isApiEnabled(kFrobulateEnabledOrigin,
         kFrobulateAPIName,
         kFrobulateAPIName /* Use just the api name as the key value */,
-        errorMessage);
+        &errorMessage);
     EXPECT_TRUE(isOriginEnabled);
     EXPECT_TRUE(errorMessage.isEmpty());
 }
 
+TEST_F(ExperimentsTest, EnabledSecureRegisteredOriginWithoutErrorMessage)
+{
+    bool isOriginEnabled = isApiEnabledWithoutErrorMessage(
+        kFrobulateEnabledOrigin,
+        kFrobulateAPIName,
+        kFrobulateAPIName /* Use just the api name as the key value */);
+    EXPECT_TRUE(isOriginEnabled);
+}
+
 // The API should not be enabled if the origin is unsecure, even if a valid
 // key for the origin is provided
 TEST_F(ExperimentsTest, EnabledNonSecureRegisteredOrigin)
@@ -127,7 +152,7 @@ TEST_F(ExperimentsTest, EnabledNonSecureRegisteredOrigin)
     bool isOriginEnabled = isApiEnabled(kFrobulateEnabledOriginUnsecure,
         kFrobulateAPIName,
         kFrobulateAPIName /* Use just the api name as the key value */,
-        errorMessage);
+        &errorMessage);
     EXPECT_FALSE(isOriginEnabled);
     EXPECT_TRUE(errorMessage.contains("secure origin")) << "Message should indicate only secure origins are allowed, was: " << errorMessage;
 }
@@ -140,5 +165,4 @@ TEST_F(ExperimentsTest, DisabledException)
     EXPECT_TRUE(disabledException->message().contains(kNonExistingAPIName)) << "Message should contain the API name, was: " << disabledException->message();
 }
 
-} // namespace
 } // namespace blink