Bug 44891

Summary: [V8] Share bindings logic for location.replace between V8 and JSC
Product: WebKit Reporter: Dominic Cooney <dominicc>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dglazkov, gns, sam, tkent, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 45200    
Bug Blocks:    
Attachments:
Description Flags
Patch. Do not review--just bouncing off trybots.
none
Patch.
none
Patch, Qt makefiles.
none
Patch.
none
Patch.
none
Patch.
none
Patch.
none
Patch
none
Patch
none
Patch none

Description Dominic Cooney 2010-08-30 13:58:33 PDT
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.
Comment 1 Dominic Cooney 2010-08-30 14:02:07 PDT
Created attachment 65951 [details]
Patch. Do not review--just bouncing off trybots.
Comment 2 Dominic Cooney 2010-08-30 20:29:39 PDT
Created attachment 66006 [details]
Patch.
Comment 3 Early Warning System Bot 2010-08-30 20:37:30 PDT
Attachment 66006 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3881169
Comment 4 WebKit Review Bot 2010-08-30 20:39:10 PDT
Attachment 66006 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3825139
Comment 5 Dominic Cooney 2010-08-31 14:24:57 PDT
Created attachment 66106 [details]
Patch, Qt makefiles.
Comment 6 Adam Barth 2010-08-31 18:15:25 PDT
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.
Comment 7 Adam Barth 2010-08-31 18:15:51 PDT
Adding sam to make sure he's ok with this direction.
Comment 8 Adam Barth 2010-08-31 18:16:32 PDT
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.
Comment 9 Dominic Cooney 2010-09-01 00:11:00 PDT
Created attachment 66172 [details]
Patch.
Comment 10 Adam Barth 2010-09-01 00:24:25 PDT
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.
Comment 11 Dominic Cooney 2010-09-01 00:40:39 PDT
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);
Comment 12 Adam Barth 2010-09-01 00:46:18 PDT
(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.
Comment 13 Dominic Cooney 2010-09-01 13:24:25 PDT
Created attachment 66253 [details]
Patch.
Comment 14 WebKit Review Bot 2010-09-01 14:11:30 PDT
Attachment 66253 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3955026
Comment 15 Dominic Cooney 2010-09-01 15:42:49 PDT
Comment on attachment 66253 [details]
Patch.

Will update ChangeLog and fix GTK build.
Comment 16 WebKit Review Bot 2010-09-01 16:01:47 PDT
Attachment 66253 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3916026
Comment 17 Dominic Cooney 2010-09-02 12:09:26 PDT
Created attachment 66393 [details]
Patch.
Comment 18 Dominic Cooney 2010-09-03 11:50:45 PDT
Created attachment 66525 [details]
Patch.
Comment 19 Adam Barth 2010-09-03 13:46:23 PDT
Comment on attachment 66525 [details]
Patch.

Okiedokes.
Comment 20 WebKit Commit Bot 2010-09-03 14:12:15 PDT
Comment on attachment 66525 [details]
Patch.

Clearing flags on attachment: 66525

Committed r66770: <http://trac.webkit.org/changeset/66770>
Comment 21 WebKit Commit Bot 2010-09-03 14:12:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Dominic Cooney 2010-09-09 02:19:58 PDT
Created attachment 67010 [details]
Patch
Comment 23 Dominic Cooney 2010-09-09 17:53:23 PDT
Created attachment 67127 [details]
Patch
Comment 24 Kent Tamura 2010-09-09 18:01:42 PDT
Comment on attachment 67127 [details]
Patch

ok
Comment 25 Kent Tamura 2010-09-09 22:00:48 PDT
Comment on attachment 67127 [details]
Patch

I'll commit this manually.
Comment 26 Dominic Cooney 2010-09-09 22:07:48 PDT
Created attachment 67152 [details]
Patch
Comment 27 Kent Tamura 2010-09-09 22:10:33 PDT
Comment on attachment 67152 [details]
Patch

Clearing flags on attachment: 67152

Committed r67167: <http://trac.webkit.org/changeset/67167>
Comment 28 Kent Tamura 2010-09-09 22:10:44 PDT
All reviewed patches have been landed.  Closing bug.