bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

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

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 13 matching lines @@
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #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 DOMWindow;
+class EventTarget;
+class ExceptionState;
+class Frame;
 class LocalDOMWindow;
-class ExceptionState;
+class Location;
 class Node;
 
 enum SecurityReportingOption {
     DoNotReportSecurityError,
     ReportSecurityError,
 };
 
 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