bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h

Index: third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h
diff --git a/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h b/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h
index f8fbe354caff51b25d157eae0ebc90d5296f1076..68b14f87be782d1b9f6040f14f30c6242bcebd47 100644
--- a/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h
+++ b/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h
@@ -31,16 +31,18 @@
 #ifndef BindingSecurity_h
 #define BindingSecurity_h
 
-// FIXME: The LocalFrame include should not be necessary, clients should be including it where they use it.
 #include "core/CoreExport.h"
-#include "core/frame/LocalFrame.h"
 #include "wtf/Allocator.h"
 #include <v8.h>
 
 namespace blink {
 
-class LocalDOMWindow;
+class DOMWindow;
+class EventTarget;
 class ExceptionState;
+class Frame;
+class LocalDOMWindow;
+class Location;
 class Node;
 
 enum SecurityReportingOption {
@@ -51,15 +53,26 @@ enum SecurityReportingOption {
 class BindingSecurity {

[haraken, 2015/11/16 11:34:20]

Add CORE_EXPORT to the class (and remove CORE_EXPORT from each method).

[Yuki, 2015/11/20 12:27:52]

Done.

     STATIC_ONLY(BindingSecurity);
 public:
+    // Check the access to the receiver.

[haraken, 2015/11/16 11:34:20]

// Check if the receiver is allowed to access the
DOMWindow/EventTarget/Location.

[dcheng, 2015/11/17 01:56:59]

Can we clarify the comments to define what "receiver" means in this context?

[Yuki, 2015/11/20 12:27:52]

I rewrote the comment in a little bit different way.

[Yuki, 2015/11/20 12:27:52]

Done.

+    // DOMWindow
+    CORE_EXPORT static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const DOMWindow* target, ExceptionState&);

[jochen (gone - plz use gerrit), 2015/11/16 14:16:58]

should we encode what the method does in the name instead of in a comment?

shouldAllowAccessFromWindowTo() and shouldAllowWindowToGetReturnValue() or
something?

[Yuki, 2015/11/20 12:27:52]

I agree that we'd better to encode what the method does in the name, however,
this simple overloading makes the generated code much simpler.

    {{cpp_type}} impl = V8T::toImpl(v8Object);
    if (!shouldAllowAccessTo(accessingWindow, impl)) {
        return;  // NOT ALLOWED.
    }
    impl->doSomething();

where there is no need to change the name of shouldAllowAccessToXXX() depending
on the type of |impl|.

In this case, I'd prefer the simple overloading with the same name.  With this
CL, we can unify [CheckSecurity=Window] and [CheckSecurity=Frame] extended
attributes.

What do you guys think?

+    CORE_EXPORT static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const DOMWindow* target, SecurityReportingOption);
+    // EventTarget (as the parent of DOMWindow)
+    CORE_EXPORT static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const EventTarget* target, ExceptionState&);  // NOLINT(readability/parameter_name)

[haraken, 2015/11/16 11:34:20]

I'm just curious but how much is the EventTarget* version used? Hopefully we can
remove it...

[Yuki, 2015/11/20 12:27:52]

The generated V8EventTarget.cpp needs the EventTarget* version.  The DOM
attributes and operations of EventTarget, such as {add,remove}EventListener,
dispatchEvent, etc., need to check the origin because the instance can be an
instance of DOMWindow.  So we need this version.

+    // Location
+    CORE_EXPORT static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const Location* target, ExceptionState&);
+    CORE_EXPORT static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const Location* target, SecurityReportingOption);
+
     // Check the access to the return value.

[dcheng, 2015/11/17 01:56:59]

and "access to the return value". I'm not super familiar with bindings, so I'm
not actually sure what the difference between the receiver and return value
methods is.

[Yuki, 2015/11/20 12:27:52]

Done.

-    static bool shouldAllowAccessToNode(v8::Isolate*, LocalDOMWindow* accessingWindow, Node*, SecurityReportingOption);
-    static bool shouldAllowAccessToNode(v8::Isolate*, LocalDOMWindow* accessingWindow, Node*, ExceptionState&);
+    // Node
+    CORE_EXPORT static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const Node* target, ExceptionState&);
+    CORE_EXPORT static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const Node* target, SecurityReportingOption);
 
-    // Check the access to the receiver.
-    CORE_EXPORT static bool shouldAllowAccessToFrame(v8::Isolate*, LocalDOMWindow* accessingWindow, Frame*, SecurityReportingOption = ReportSecurityError);
-    CORE_EXPORT static bool shouldAllowAccessToFrame(v8::Isolate*, LocalDOMWindow* accessingWindow, Frame*, ExceptionState&);
+    // Check the access to the frame rather than to a DOM object.

[haraken, 2015/11/16 11:34:20]

DOM object => DOMWindow/EventTarget/Location

[Yuki, 2015/11/20 12:27:52]

Done.

+    // You should check the access to the DOM object as long as it's possible.

[dcheng, 2015/11/17 01:56:59]

Nit: reword this as "Prefer to use the previous overloads instead of falling
back to using Frame*."

[Yuki, 2015/11/20 12:27:52]

Done.

+    CORE_EXPORT static bool shouldAllowAccessToFrame(v8::Isolate*, const LocalDOMWindow* accessingWindow, const Frame*, SecurityReportingOption);

[haraken, 2015/11/16 11:34:20]

const Frame* target

[dcheng, 2015/11/17 01:56:59]

This is a 'receiver' method, right? Should it be grouped with the other receiver
methods?

[Yuki, 2015/11/20 12:27:52]

Done.

[Yuki, 2015/11/20 12:27:52]

Done.

 };
 
-}
+} // namespace blink
 
 #endif