Make content::FeaturePolicy implement WebFeaturePolicy, and use it in blink code

third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp

Index: third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp b/third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp
index 8f3a147dbdcbab0d1e2c8b615833f4d794ccbebd..f03be985815fffb89853e5e140a7ac066e88f3f9 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ConditionalFeatures.cpp
@@ -10,8 +10,9 @@
 #include "bindings/core/v8/V8Navigator.h"
 #include "bindings/core/v8/V8Window.h"
 #include "core/dom/ExecutionContext.h"
-#include "core/frame/LocalFrame.h"
+#include "core/frame/Frame.h"
 #include "core/origin_trials/OriginTrials.h"
+#include "public/platform/Platform.h"
 
 namespace blink {
 
@@ -141,22 +142,36 @@ void installPendingConditionalFeature(const String& feature,
   (*s_installPendingConditionalFeatureFunction)(feature, scriptState);
 }
 
-bool isFeatureEnabledInFrame(const FeaturePolicy::Feature& feature,
-                             const LocalFrame* frame) {
-  // If there is no frame, or if feature policy is disabled, use defaults.
-  bool enabledByDefault =
-      (feature.defaultPolicy == FeaturePolicy::FeatureDefault::EnableForAll ||
-       (feature.defaultPolicy == FeaturePolicy::FeatureDefault::EnableForSelf &&
-        !frame->isCrossOriginSubframe()));
-  if (!RuntimeEnabledFeatures::featurePolicyEnabled() || !frame)
-    return enabledByDefault;
-  FeaturePolicy* featurePolicy = frame->securityContext()->getFeaturePolicy();
+bool isFeatureEnabledInFrame(WebFeaturePolicyFeature feature,
+                             const Frame* frame) {
+  SecurityOrigin* origin = nullptr;
+  if (frame)
+    origin = frame->securityContext()->getSecurityOrigin();
+
+  // TODO: Remove this check when FP ships. This sets the static policy for
+  // fullscreen and vibrate to be allowed at the top-level, but not in cross-
+  // origin content. With FP enabled, this logic is centralized in the
+  // FeaturePolicy class.
+  if (!RuntimeEnabledFeatures::featurePolicyEnabled() || !frame) {

[raymes, 2017/02/13 04:45:22]

Currently the only callsites I see for this check that FP is enabled prior to
calling this function. Maybe some of these changes could be put into an
orthogonal CL?

[iclelland, 2017/02/23 20:04:12]

Agreed; most of this should be removed; when feature policy is disabled, this
shouldn't be called at all. That will simplify a lot of the code, including the
awkward edge-case handling below.

+    bool isSameOriginSubframe =
+        frame &&
+        (frame->isMainFrame() ||
+         origin->canAccess(
+             frame->tree().top()->securityContext()->getSecurityOrigin()));
+    return (isSameOriginSubframe ||

[raymes, 2017/02/13 04:45:22]

This might be a bit simpler to read as:
if (isSameOriginSubframe) {
  return feature != WebFeaturePolicyFeature::x && 
         feature != WebFeaturePolicyFeature::y;
}
  

[iclelland, 2017/02/23 20:04:12]

Done.

+            !(feature == WebFeaturePolicyFeature::Fullscreen ||

[raymes, 2017/02/13 04:45:22]

Does this mean fullscreen won't work at all in a cross-origin iframe? Shouldn't
it work if the attribute is provided? Same with payments?

[iclelland, 2017/02/23 20:04:12]

That edge case actually isn't ever hit -- it was a rough "is this page cross
origin" check for the features which are disabled by default in cross-origin
frames (like vibrate), to handle the case when FeaturePolicy was not enabled.
Fullscreen won't run this code, though, in that case; if FP is disabled, it just
walks up the frame tree looking for 'allowfullscreen' attributes on frame
owners. Same with payments. This is really just for vibrate. (And then vibrate
went ahead and used the runtime feature check, so this whole block is unused
now.)

+              feature == WebFeaturePolicyFeature::Geolocation ||

[raymes, 2017/02/13 04:45:22]

Should geolocation be here?

[iclelland, 2017/02/23 20:04:12]

It was just all of the features that had a default defined in the spec of
["self"]. 

+              feature == WebFeaturePolicyFeature::Payment ||
+              feature == WebFeaturePolicyFeature::Vibrate));
+  }
+  WebFeaturePolicy* featurePolicy =
+      frame->securityContext()->getFeaturePolicy();
   // The policy should always be initialized before checking it to ensure we
   // properly inherit the parent policy.
   DCHECK(featurePolicy);
 
   // Otherwise, check policy.
-  return featurePolicy->isFeatureEnabled(feature);
+  return Platform::current()->isFeatureEnabledByPolicy(featurePolicy, feature);
 }
 
 }  // namespace blink