[webkit-reviews] review denied: [Bug 32326] Refactor some security code out of V8 bindings : [Attachment 44544] Fixes issues from the style-queue bot

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Charles Reis <creis at chromium.org>'s
request for review:
Bug 32326: Refactor some security code out of V8 bindings
https://bugs.webkit.org/show_bug.cgi?id=32326

Attachment 44544: Fixes issues from the style-queue bot
https://bugs.webkit.org/attachment.cgi?id=44544&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
Thanks for fixing the style nits.  At a high level, this looks great. I have a
few questions:

+ virtual DOMWindow* getActiveWindow() = 0;

Does this need to be virtual, or can we do link-time dispatch here?

+ // Implement BindingsSecurity methods that depend on V8-specific code.

Don't we want to implement this in a V8BindingsSecurity.cpp file instead of
V8Proxy?  V8Proxy is an all-sing, all-dance dumping ground.

+ class V8State : public BindingsState {

I'd call this V8BindingsState to mirror the DOMWindow / V8DOMWindow name
convention.

In general, it's slightly unclear to me whether we actually want a
V8BindingsState object that derives from BindingsState or whether we want to
have a single class where some of the methods are implemented by the different
bindings, like ScriptController.  The advantage of the latter is that we'll get

compile-time dispatch instead of runtime dispatch.

Another option is to have a shared BindingsStateBase and V8 implement a
subclass BindingsState that the shared code refers to.	That way, the bindings
can override whatever methods they need without incurring a virtual method
call.

A final option is to use template namespaces.  These shared classes would be

template<classname Bindings>
class ...

and then refer to Bindings::State.  Then we can have V8Binding::State derive
from GenericBinding::State.  That's more code, but it might make it easier to
support things like re-using this code for ObjCBindings::State, etc.

Thoughts?