Bug 51527 – Move V8 to WebCore's implementation of showModalDialog

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Move V8 to WebCore's implementation of showModalDialog

[Adam Barth]

Created attachment 77316 [details]
Patch

[anton muhin]

LGTM

> WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:401
> +    if (m_dialogArguments.IsEmpty())

apparently m_dialogArguments can never be empty.  Maybe just ASSERT it here?

> WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:412
> +    v8::Handle<v8::Value> returnValue = m_dialogContext->Global()->Get(v8::String::New("returnValue"));

technically one doesn't need to turn empty handle into undefined here, but that might be just safer.   My only concern is it could hide some exception, but I don't think you've introduced it here.

> WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:430
> +    DOMWindow* activeWindow = state->activeWindow();

minor cosmetic nit: maybe nuke local variable (activeWindow) and use the expression directly (state->activeWindow())?

Same for firstWindow.

Feel free to ignore

(In reply to comment #1)
> Created an attachment (id=77316) [details]
> Patch

[WebKit Commit Bot]

Comment on attachment 77316 [details]
Patch

Clearing flags on attachment: 77316

Committed r74563: <http://trac.webkit.org/changeset/74563>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Comment on attachment 77316 [details]
Patch

Rejecting attachment 77316 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'apply-attachment', '--no-update', '--non-interactive', 77316]" exit_code: 2
Last 500 characters of output:
ng file WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp
Hunk #1 FAILED at 378.
Hunk #2 FAILED at 451.
2 out of 2 hunks FAILED -- saving rejects to file WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp.rej
patching file WebCore/bindings/v8/specialization/V8BindingDOMWindow.h
Hunk #1 FAILED at 31.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/bindings/v8/specialization/V8BindingDOMWindow.h.rej

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7239128

[Adam Barth]

> > WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:401
> > +    if (m_dialogArguments.IsEmpty())
> 
> apparently m_dialogArguments can never be empty.  Maybe just ASSERT it here?

The old code had this test.  I'll write a follow-up patch that removes it.

> > WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:412
> > +    v8::Handle<v8::Value> returnValue = m_dialogContext->Global()->Get(v8::String::New("returnValue"));
> 
> technically one doesn't need to turn empty handle into undefined here, but that might be just safer.   My only concern is it could hide some exception, but I don't think you've introduced it here.

Yeah, the old code did that, and I thought that was unnecessary.  Is undefined different than an empty handle?

> > WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:430
> > +    DOMWindow* activeWindow = state->activeWindow();
> 
> minor cosmetic nit: maybe nuke local variable (activeWindow) and use the expression directly (state->activeWindow())?
> 
> Same for firstWindow.

I'd like to keep this code this way to match the JSC code as close as possible, although I agree it's kind of silly.

[Darin Adler]

(In reply to comment #6)
> I'd like to keep this code this way to match the JSC code as close as possible, although I agree it's kind of silly.

We can change/refine both if practical now.

I’d also like to look over the “generic bindings” machinery to see if there is more we can remove.

[Adam Barth]

> > I'd like to keep this code this way to match the JSC code as close as possible, although I agree it's kind of silly.
> 
> We can change/refine both if practical now.

Ok.  I'll post a cleanup patch for both.

> I’d also like to look over the “generic bindings” machinery to see if there is more we can remove.

The next area I would look at is Location.  The main thing to move into WebCore is shouldAllowNavigation.

[anton muhin]

(In reply to comment #6)
> > > WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:412
> > > +    v8::Handle<v8::Value> returnValue = m_dialogContext->Global()->Get(v8::String::New("returnValue"));
> > 
> > technically one doesn't need to turn empty handle into undefined here, but that might be just safer.   My only concern is it could hide some exception, but I don't think you've introduced it here.
> 
> Yeah, the old code did that, and I thought that was unnecessary.  Is undefined different than an empty handle?

They are different, but when coming back to v8 from C++ callback, empty handle gets converted to undefined.  But as you moved this code into different method, it might be more robust to do this check anyway.