[Webkit-unassigned] [Bug 44891] [V8] Share bindings logic for location.replace between V8 and JSC

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44891





--- Comment #6 from Adam Barth <abarth at webkit.org>  2010-08-31 18:15:25 PST ---
(From update of attachment 66106)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.