Initial Implementation of Iframe Attribute for Feature Policy (Part 2)

third_party/WebKit/Source/core/frame/FrameOwner.h

Index: third_party/WebKit/Source/core/frame/FrameOwner.h
diff --git a/third_party/WebKit/Source/core/frame/FrameOwner.h b/third_party/WebKit/Source/core/frame/FrameOwner.h
index 6a2a91401b5b39045c7bc04ffdacf2f0d4258622..e4ac63b1c64a9e7368458bfb6f2743504a682c67 100644
--- a/third_party/WebKit/Source/core/frame/FrameOwner.h
+++ b/third_party/WebKit/Source/core/frame/FrameOwner.h
@@ -9,6 +9,7 @@
 #include "core/dom/SandboxFlags.h"
 #include "platform/heap/Handle.h"
 #include "platform/scroll/ScrollTypes.h"
+#include "public/platform/WebFeaturePolicy.h"
 #include "public/platform/WebVector.h"
 #include "public/platform/modules/permissions/permission.mojom-blink.h"
 
@@ -51,6 +52,8 @@ class CORE_EXPORT FrameOwner : public GarbageCollectedMixin {
   virtual AtomicString csp() const = 0;
   virtual const WebVector<mojom::blink::PermissionName>& delegatedPermissions()
       const = 0;
+  virtual const WebVector<WebFeaturePolicyFeature>& allowedFeatureNames()

[iclelland, 2017/02/16 15:26:33]

I feel like we should rename this to allowedFeatures everywhere, since it's a
vector of features, and not just strings. Would that make sense?

[lunalu1, 2017/02/16 20:16:15]

Done.

+      const = 0;

[iclelland, 2017/02/16 15:26:33]

I'm curious why this is a pure virtual method, when the exact same default
implementation needs to be used in two if its three subclasses.

Is there a reason we can't use the default implementation here, and then not
have to re-define it in DummyFrameOwner and HTMLFrameOwnerElement?

[lunalu1, 2017/02/16 20:16:15]

We can use a default definition here, that just means we will have to copy the
m_allowedFeatures to here to return?
Currently virtual method is used for other attributes like allowfullscreen() and
delegatedPermissions(), which is why I followed the same pattern. 

[iclelland, 2017/02/16 20:33:40]

I'm just wondering whether the pattern is there for a good reason. I thought
that we could move this method:

  const WebVector<WebFeaturePolicyFeature>& allowedFeatureNames() const {
    DEFINE_STATIC_LOCAL(WebVector<WebFeaturePolicyFeature>, features, ());
    return features;
  }

into FrameOwner, and then it would be the default for things like
HTMLEmbedElement, HTMLObjectElement, HTMLFrameElement, etc. The version with
m_allowedFeatures would just be in HTMLIframeElement and RemoteFrameOwner. But
maybe there's a good reason we can't do that.

[iclelland, 2017/02/16 20:49:27]

I suppose this might be for memory savings -- if the compiler can avoid
generating a vtable for this class when all methods are pure virtual. Following
the existing pattern is fine here, I think. If we want to revisit, we can move
all of these related methods in the same CL and measure any memory regression.

[lunalu1, 2017/02/16 21:33:46]

According to Google Style Guide
(https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance), a
subclass with more than one base classes is only allowed if at most one of the
base class has an implementation. All other base classes must be pure interface
classes. 
An example in our case would be HTMLFrameOwnerElement. It has 2 base classes,
HTMLElement and FrameOwner. FrameOwner is the pure interface class here.

 };
 
 // TODO(dcheng): This class is an internal implementation detail of provisional
@@ -89,6 +92,11 @@ class CORE_EXPORT DummyFrameOwner
                         ());
     return permissions;
   }
+  const WebVector<WebFeaturePolicyFeature>& allowedFeatureNames()
+      const override {
+    DEFINE_STATIC_LOCAL(WebVector<WebFeaturePolicyFeature>, features, ());
+    return features;
+  }
 
  private:
   // Intentionally private to prevent redundant checks when the type is