Bug 44891 – [V8] Share bindings logic for location.replace between V8 and JSC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dominic Cooney]

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.

[Dominic Cooney]

Created attachment 65951 [details]
Patch. Do not review--just bouncing off trybots.

[Dominic Cooney]

Created attachment 66006 [details]
Patch.

[Early Warning System Bot]

Attachment 66006 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3881169

[WebKit Review Bot]

Attachment 66006 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3825139

[Dominic Cooney]

Created attachment 66106 [details]
Patch, Qt makefiles.

[Adam Barth]

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.

[Adam Barth]

Adding sam to make sure he's ok with this direction.

[Adam Barth]

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.

[Dominic Cooney]

Created attachment 66172 [details]
Patch.

[Adam Barth]

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.

[Dominic Cooney]

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);

[Adam Barth]

(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.

[Dominic Cooney]

Created attachment 66253 [details]
Patch.

[WebKit Review Bot]

Attachment 66253 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3955026

[Dominic Cooney]

Comment on attachment 66253 [details]
Patch.

Will update ChangeLog and fix GTK build.

[WebKit Review Bot]

Attachment 66253 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3916026

[Dominic Cooney]

Created attachment 66393 [details]
Patch.

[Dominic Cooney]

Created attachment 66525 [details]
Patch.

[Adam Barth]

Comment on attachment 66525 [details]
Patch.

Okiedokes.

[WebKit Commit Bot]

Comment on attachment 66525 [details]
Patch.

Clearing flags on attachment: 66525

Committed r66770: <http://trac.webkit.org/changeset/66770>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dominic Cooney]

Created attachment 67010 [details]
Patch

[Dominic Cooney]

Created attachment 67127 [details]
Patch

[Kent Tamura]

Comment on attachment 67127 [details]
Patch

ok

[Kent Tamura]

Comment on attachment 67127 [details]
Patch

I'll commit this manually.

[Dominic Cooney]

Created attachment 67152 [details]
Patch

[Kent Tamura]

Comment on attachment 67152 [details]
Patch

Clearing flags on attachment: 67152

Committed r67167: <http://trac.webkit.org/changeset/67167>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.