To eliminate trivial duplication of JSC's bindings logic by the V8 bindings, we can move the logic into bindings/generic and share SOP, etc. between JSC and V8.
Created attachment 65951 [details] Patch. Do not review--just bouncing off trybots.
Created attachment 66006 [details] Patch.
Attachment 66006 [details] did not build on qt: Build output: http://queues.webkit.org/results/3881169
Attachment 66006 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3825139
Created attachment 66106 [details] Patch, Qt makefiles.
Comment on attachment 66106 [details] Patch, Qt makefiles. View in context: https://bugs.webkit.org/attachment.cgi?id=66106&action=prettypatch > WebCore/bindings/generic/BindingLocation.h:47 > +template <class Binding> > +void BindingLocation<Binding>::replace(State<Binding>* state, Location* location, const String& urlSpec) Usually we just call String versions of URLs "url". Yes, it's lame. We need to fix that globally at some point. > WebCore/bindings/generic/BindingLocation.h:55 > + KURL url = Binding::Utilities::completeURL(state, urlSpec); > + if (url.isNull()) > + return; In that case, we'd call these "fullURL" > WebCore/bindings/generic/BindingLocation.h:60 > + Binding::Utilities::navigateIfAllowed(state, frame, url, true, true); true/true is hard to understand. Not really the fault of this patch. Just something to notice. > WebCore/bindings/generic/BindingUtilities.h:45 > +template <class Binding> > +class BindingUtilities { > +public: > + static KURL completeURL(State<Binding>*, const String& relativeURL); > + static void navigateIfAllowed(State<Binding>*, Frame*, const KURL&, bool lockHistory, bool lockBackForwardList); > +}; I'm not super cheesed about a general "utilities" class. navigateIfAllowed seems like a method of Frame somehow... I'm not sure what advice to give you here. Maybe completeURL should be a method of the state since we're completing the URL w.r.t. the state? > WebCore/bindings/generic/BindingUtilities.h:65 > + if (!protocolIsJavaScript(url) || state->isSafeScript(frame)) I don't really like the name "isSafeScript". Sorry to nitpick the names. Something like state->canAccess(frame) would be clearer to me. > WebCore/bindings/js/JSDOMBinding.cpp:645 > + JSBindingState state = JSBindingState::create(exec); Why not just have a regular constructor instead of a create method? > WebCore/bindings/js/specialization/JSBindingState.cpp:61 > +bool State<JSBinding>::isSafeScript(Frame* frame) > +{ > + return allowsAccessFromFrame(m_exec, frame); > +} Ah, see. allowsAccessFromFrame would be a since name for isSafeScript. That also explains what direction the access is being tested in. > WebCore/bindings/js/specialization/JSBindingState.h:53 > + static State create(JSC::ExecState* exec) > + { > + return State(exec); > + } Yeah, I'd just make this a constructor. > WebCore/bindings/js/specialization/JSBindingState.h:68 > + explicit State(JSC::ExecState* exec) : m_exec(exec) {} Should be split onto multiple lines. > WebCore/bindings/v8/specialization/V8BindingState.cpp:89 > + return ScriptController::isSafeScript(frame); Yeah, this method should be renamed to match the JSC method.
Adding sam to make sure he's ok with this direction.
Comment on attachment 66106 [details] Patch, Qt makefiles. I'd like to see one more iteration of this patch with the comments above addressed (even if they're minor). This also gives Sam more time to express an opinion.
Created attachment 66172 [details] Patch.
Comment on attachment 66172 [details] Patch. View in context: https://bugs.webkit.org/attachment.cgi?id=66172&action=prettypatch Looks great. Give Sam a day or two to comment before landing the patch though. > WebCore/bindings/generic/GenericBinding.h:35 > +#include "Frame.h" > +#include "FrameLoader.h" These don't seem needed anymore. > WebCore/bindings/generic/GenericBinding.h:46 > -class State {}; > +class State { > +}; This change is correct, but probably not needed. > WebCore/bindings/js/JSLocationCustom.cpp:192 > + JSBindingState state(exec); > + JSBinding::Frame::navigateIfAllowed(&state, frame, url, lockHistory ? JSBinding::Frame::LockedHistory : JSBinding::Frame::UnlockedHistory, lockBackForwardList ? JSBinding::Frame::LockedBackForwardList : JSBinding::Frame::UnlockedBackForwardList); What do you think? Is this better? > WebCore/bindings/js/specialization/JSBindingState.h:51 > + explicit State(JSC::ExecState* exec) > + : m_exec(exec) {} Usually we'll put these braces on their own lines. > WebCore/bindings/v8/V8Utilities.cpp:119 > + return V8Binding::Frame::navigateIfAllowed(V8BindingState::Only(), frame, url, lockHistory ? V8Binding::Frame::LockedHistory : V8Binding::Frame::UnlockedHistory, lockBackForwardList ? V8Binding::Frame::LockedBackForwardList : V8Binding::Frame::UnlockedBackForwardList); I think to make this work, we'd have to put these enums in WebCore, probably in FrameLoader and push them through the loading system. It's probably worth doing, but maybe not in this patch.
I would like to move BindingFrame::completeURL onto State, but that needs to call getFirstFrame in the specialization (State<JSBinding>, etc.) which is blocked by State<GenericBinding>. Should State<GenericBinding> use CRTP and shared functions downcast to the specialization? Or should I nix State<GenericBinding> and make completeURL templatized on State<T>? > > WebCore/bindings/js/JSLocationCustom.cpp:192 > > + JSBindingState state(exec); > > + JSBinding::Frame::navigateIfAllowed(&state, frame, url, lockHistory ? JSBinding::Frame::LockedHistory : JSBinding::Frame::UnlockedHistory, lockBackForwardList ? JSBinding::Frame::LockedBackForwardList : JSBinding::Frame::UnlockedBackForwardList); > What do you think? Is this better? It is ugly. What would you prescribe? FIXME to move the enums into WebCore? Or explain at the call site with variables: bool lockHistory = true; bool lockBackForwardList = true; JSBinding::Frame::NavigateIfAllowed(&state, frame, url, lockHistory, lockBackForwardList);
(In reply to comment #11) > I would like to move BindingFrame::completeURL onto State, but that needs to call getFirstFrame in the specialization (State<JSBinding>, etc.) which is blocked by State<GenericBinding>. Should State<GenericBinding> use CRTP and shared functions downcast to the specialization? Or should I nix State<GenericBinding> and make completeURL templatized on State<T>? Can't we just make completeURL a free function like it currently is? It can be templated on T without having a class to back it up. > > > WebCore/bindings/js/JSLocationCustom.cpp:192 > > > + JSBindingState state(exec); > > > + JSBinding::Frame::navigateIfAllowed(&state, frame, url, lockHistory ? JSBinding::Frame::LockedHistory : JSBinding::Frame::UnlockedHistory, lockBackForwardList ? JSBinding::Frame::LockedBackForwardList : JSBinding::Frame::UnlockedBackForwardList); > > What do you think? Is this better? > > It is ugly. What would you prescribe? FIXME to move the enums into WebCore? Or explain at the call site with variables: > > bool lockHistory = true; > bool lockBackForwardList = true; > JSBinding::Frame::NavigateIfAllowed(&state, frame, url, lockHistory, lockBackForwardList); I think we should go back to what you had before and just pass the literal values and remember that we should do this global cleanup another day.
Created attachment 66253 [details] Patch.
Attachment 66253 [details] did not build on gtk: Build output: http://queues.webkit.org/results/3955026
Comment on attachment 66253 [details] Patch. Will update ChangeLog and fix GTK build.
Attachment 66253 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3916026
Created attachment 66393 [details] Patch.
Created attachment 66525 [details] Patch.
Comment on attachment 66525 [details] Patch. Okiedokes.
Comment on attachment 66525 [details] Patch. Clearing flags on attachment: 66525 Committed r66770: <http://trac.webkit.org/changeset/66770>
All reviewed patches have been landed. Closing bug.
Created attachment 67010 [details] Patch
Created attachment 67127 [details] Patch
Comment on attachment 67127 [details] Patch ok
Comment on attachment 67127 [details] Patch I'll commit this manually.
Created attachment 67152 [details] Patch
Comment on attachment 67152 [details] Patch Clearing flags on attachment: 67152 Committed r67167: <http://trac.webkit.org/changeset/67167>