[webkit-reviews] review granted: [Bug 44891] [V8] Share bindings logic for location.replace between V8 and JSC : [Attachment 66172] Patch.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dominic Cooney
<dominicc at google.com>'s request for review:
Bug 44891: [V8] Share bindings logic for location.replace between V8 and JSC
https://bugs.webkit.org/show_bug.cgi?id=44891

Attachment 66172: Patch.
https://bugs.webkit.org/attachment.cgi?id=66172&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.