Bug 41392 – [V8] Move window.open into generic bindings

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

[Dominic Cooney]

Like bug 33201 which moved createWindow into the generic bindings, logic for open can be moved into the generic bindings too.

[Dominic Cooney]

Created attachment 60331 [details]
Moves window.open into the generic bindings.

[WebKit Review Bot]

Attachment 60331 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/generic/BindingDOMWindow.h:167:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/bindings/generic/BindingDOMWindow.h:192:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/bindings/v8/V8Utilities.cpp:41:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominic Cooney]

Created attachment 60333 [details]
Moves window.open into the generic bindings.

[Adam Barth]

Comment on attachment 60333 [details]
Moves window.open into the generic bindings.

Sorry, need to run.  I got about half way through.

WebCore/bindings/generic/BindingDOMWindow.h:150
 +      Frame* enteredFrame = state->getFirstFrame();
Why not call this firstFrame?

WebCore/bindings/generic/BindingDOMWindow.h:154
 +      Frame* callingFrame = state->getActiveFrame();
Why not call this activeFrame?

WebCore/bindings/generic/BindingDOMWindow.h:193
 +              && (!protocolIsJavaScript(completedUrl) || ScriptController::isSafeScript(frame))) {
isSafeScript is a terrible name.  I don't think this is correct.  You need to call something on BindingsSecurity and pass in the state.

WebCore/bindings/generic/BindingSecurity.h:150
 +      Frame* callingFrame = state->getActiveFrame();
again, should be "activeFrame"

[Dominic Cooney]

Created attachment 63861 [details]
Patch

[Dominic Cooney]

Created attachment 63873 [details]
Patch

[Adam Barth]

Comment on attachment 63873 [details]
Patch

This looks great.  Now that we have more of this fleshed out, we should talk to some JSC folks and see if they want to use some of it.

[Eric Seidel (no email)]

Comment on attachment 63873 [details]
Patch

Clearing flags on attachment: 63873

Committed r64991: <http://trac.webkit.org/changeset/64991>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.