Bug 41392

Summary: [V8] Move window.open into generic bindings
Product: WebKit Reporter: Dominic Cooney <dominicc>
Component: WebCore JavaScriptAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Moves window.open into the generic bindings.
none
Moves window.open into the generic bindings.
none
Patch
none
Patch none

Description Dominic Cooney 2010-06-29 23:42:56 PDT
Like bug 33201 which moved createWindow into the generic bindings, logic for open can be moved into the generic bindings too.
Comment 1 Dominic Cooney 2010-07-01 21:49:34 PDT
Created attachment 60331 [details]
Moves window.open into the generic bindings.
Comment 2 WebKit Review Bot 2010-07-01 21:53:29 PDT
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.
Comment 3 Dominic Cooney 2010-07-01 21:59:46 PDT
Created attachment 60333 [details]
Moves window.open into the generic bindings.
Comment 4 Adam Barth 2010-07-02 13:53:11 PDT
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"
Comment 5 Dominic Cooney 2010-08-08 23:37:54 PDT
Created attachment 63861 [details]
Patch
Comment 6 Dominic Cooney 2010-08-09 02:18:23 PDT
Created attachment 63873 [details]
Patch
Comment 7 Adam Barth 2010-08-09 10:22:50 PDT
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.
Comment 8 Eric Seidel (no email) 2010-08-09 10:50:50 PDT
Comment on attachment 63873 [details]
Patch

Clearing flags on attachment: 63873

Committed r64991: <http://trac.webkit.org/changeset/64991>
Comment 9 Eric Seidel (no email) 2010-08-09 10:50:55 PDT
All reviewed patches have been landed.  Closing bug.