1405805 - Move window closing logic into nsDocShell

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Remove some unused macros & fields from nsGlobalWindow

MozReview-Commit-ID: BpNykQdha7L

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add FORWARD_TO_DOCSHELL macros to nsGlobalWindow.cpp

MozReview-Commit-ID: 2fsVRy6INr3

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Move window closing logic into nsDocShell

MozReview-Commit-ID: EtDEaaeoARB

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8924672 [details] [diff] [review]
Part 2: Add FORWARD_TO_DOCSHELL macros to nsGlobalWindow.cpp

># HG changeset patch
># User Nika Layzell <[e-mail]>
>
>Bug 1405805 - Part 2: Add FORWARD_TO_DOCSHELL macros to nsGlobalWindow.cpp, r=smaug
>
>MozReview-Commit-ID: 2fsVRy6INr3
>
>diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
>index fe0519c85ee3..08692a0037ae 100644
>--- a/dom/base/nsGlobalWindow.cpp
>+++ b/dom/base/nsGlobalWindow.cpp
>@@ -410,16 +410,73 @@ static uint32_t gThrottledIdlePeriodLength;
>       if (!mInnerWindow) {                                                    \
>         return err_rval;                                                      \
>       }                                                                       \
>     }                                                                         \
>     return GetCurrentInnerWindowInternal()->method args;                      \
>   }                                                                           \
>   PR_END_MACRO
> 
>+// NOTE: This macro doesn't take the fallback as an argument. Instead, the
>+// fallback is the block of code which is placed after the macro invocation, as
>+// if the forwarding was successful we will return from the current function.
>+#define FORWARD_TO_DOCSHELL_WITH_FALLBACK(method, args)             \
the naming is weird. There is no fallback there.
Perhaps something like TRY_TO_FORWARD_TO_DOCSHELL


>+
>+#define FORWARD_TO_DOCSHELL(method, args, err_rval)                     \
>+  PR_BEGIN_MACRO                                                        \
>+  FORWARD_TO_DOCSHELL_WITH_FALLBACK(method, args);                      \
>+  return err_rval;                                                      \
>+  PR_END_MACRO
>+
>+#define FORWARD_TO_DOCSHELL_OR_THROW(method, args, errorresult, err_rval) \
>+  PR_BEGIN_MACRO                                                        \
>+  FORWARD_TO_DOCSHELL_WITH_FALLBACK(method, args);                      \
>+  errorresult.Throw(NS_ERROR_XPC_SECURITY_MANAGER_VETO);                \
>+  return err_rval;                                                      \
>+  PR_END_MACRO
align \s

>+
>+#define FORWARD_TO_DOCSHELL_VOID(method, args, err_rval)                \
what? _VOID taking an err_rval which is then returned? Something wrong here

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8924673 [details] [diff] [review]
Part 3: Move window closing logic into nsDocShell

>+class nsCloseEvent : public Runnable {
Since you're modifying the coding style anyhow, could you fix this.
{ to its own line

>+  PostCloseEvent(nsDocShell* aDocShell, bool aIndirect) {
Ditto, and same also with many other methods

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8924671 [details] [diff] [review]
Part 1: Remove some unused macros & fields from nsGlobalWindow

The DialogValueHolder class can go away altogether.  Mind doing that?  Looks like these bits were missed in bug 1374460.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Taking a different approach with the nsGlobalWindow split, so marking wontfix.