Peter Van der Beken [:peterv] Assignee
	Description • 10 years ago

      No description provided.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 10 years ago

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.

	Peter Van der Beken [:peterv] Assignee
	Comment 2 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 3 • 10 years ago

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