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

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef FrameOwner_h
 #define FrameOwner_h
 
 #include "core/CoreExport.h"
 #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"
 
 namespace blink {
 
 class Frame;
 
 // Oilpan: all FrameOwner instances are GCed objects. FrameOwner additionally
 // derives from GarbageCollectedMixin so that Member<FrameOwner> references can
 // be kept (e.g., Frame::m_owner.)
@@ skipping 22 matching lines @@
   // https://html.spec.whatwg.org/multipage/browsers.html#browsing-context-container
   virtual AtomicString browsingContextContainerName() const = 0;
   virtual ScrollbarMode scrollingMode() const = 0;
   virtual int marginWidth() const = 0;
   virtual int marginHeight() const = 0;
   virtual bool allowFullscreen() const = 0;
   virtual bool allowPaymentRequest() const = 0;
   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
 // frames. Move this into WebLocalFrameImpl.cpp and remove existing dependencies
 // on it.
 class CORE_EXPORT DummyFrameOwner
     : public GarbageCollectedFinalized<DummyFrameOwner>,
       public FrameOwner {
   USING_GARBAGE_COLLECTED_MIXIN(DummyFrameOwner);
 
@@ skipping 18 matching lines @@
   int marginHeight() const override { return -1; }
   bool allowFullscreen() const override { return false; }
   bool allowPaymentRequest() const override { return false; }
   AtomicString csp() const override { return nullAtom; }
   const WebVector<mojom::blink::PermissionName>& delegatedPermissions()
       const override {
     DEFINE_STATIC_LOCAL(WebVector<mojom::blink::PermissionName>, permissions,
                         ());
     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
   // already DummyFrameOwner.
   bool isLocal() const override { return false; }
   bool isRemote() const override { return false; }
 };
 
 }  // namespace blink
 
 #endif  // FrameOwner_h