990475 - Add WebIDL APIs for WindowModal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8399872 [details] [diff] [review]
v1

> +nsGlobalWindow::GetDialogArguments(JSContext* aCx, ErrorResult& aError)

Do we not need to enter the compartment of GetWrapper() before calling mDialogArguments->Get()?  I think we should, or else we need a nice comment explaining why we're not doing it.

>+nsGlobalWindow::GetReturnValue(JSContext* aCx, ErrorResult& aError)

Same here, for mReturnValue->Get().

>+nsGlobalWindow::SetReturnValue(JSContext* aCx,
>+  FORWARD_TO_OUTER_OR_THROW(SetReturnValue, (aCx, aReturnValue, aError), aError,
>+                            );

The linebreak there is up.  Please either hoist the ");" or drop the aError to the next line?

>+++ b/dom/base/nsGlobalWindow.h
>+      aResult.set(JS::UndefinedValue());

aResult.setUndefined();

>+  nsRefPtr<DialogValueHolder> mReturnValue;

Document that this is outer-window-only?

>+++ b/dom/webidl/Window.webidl

Could we put the new interface above the mozilla-proprietary bits like sidebar?

r=me with that.

Comment 2

[Peter Van der Beken [:peterv]]

Had to remove the dom/base/test/test_openDialogChromeOnly.html fix from this patch, it relies on the new bindings :-/.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fbe5c6db773

and a followup rooting fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/72b4d4463629

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4fbe5c6db773
https://hg.mozilla.org/mozilla-central/rev/72b4d4463629