1415645 - Switch nsGlobalWindow internals to use {Inner,Outer} variants explicitly where possible

Status: RESOLVED FIXED

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

Description

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

Follow up from bug 1414974 to change the internals in preparation for a full split.

Comment 1

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

Attachment: Part 1: Make nsGlobalWindow.cpp use the specific {Inner,Outer} variants in more places internally

MozReview-Commit-ID: LsgjJTbuH3L

Comment 2

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

Attachment: Part 2: Make CleanupCachedXBLHandlers a instance method

This will make the split easier, as this method is used by both
nsGlobalWindowInner and nsGlobalWindowOuter.

MozReview-Commit-ID: FsjK4y6x7NE

Comment 3

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

Attachment: Part 3: Don't cast to nsGlobalWindow when working with the inner window linked list

After the window split is complete, the inner window linked list won't be
homogenously typed anymore, as there will be an nsGlobalWindowOuter member in
addition to the nsGlobalWindowInner members. This patch changes the code to
perform void* pointer comparisons before casting to nsGlobalWindowInner to avoid
this issue.

MozReview-Commit-ID: 56q5XodtGe7

Comment 4

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

Attachment: Part 4: Modify some shared runnables to not refer directly to nsGlobalWindow

MozReview-Commit-ID: ANdBkuhZ2xx

Comment 5

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

Comment on attachment 8926524 [details] [diff] [review]
Part 1: Make nsGlobalWindow.cpp use the specific {Inner,Outer} variants in more places internally

@@ -12559,17 +12561,19 @@ nsGlobalWindow::FreezeInternal()
   if (mFreezeDepth != 1) {
     return;
   }
 
   mozilla::dom::workers::FreezeWorkersForWindow(AsInner());
 
   mTimeoutManager->Freeze();
 
-  NotifyDOMWindowFrozen(this);
+  if (IsInnerWindow()) {
+    NotifyDOMWindowFrozen(AssertInner());
+  }
 }

We do assert in that method that we're dealing with inner.
And same with nsGlobalWindow::ThawInternal()

FromSupports methods could use static_cast, no?

Comment 6

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

Comment on attachment 8926526 [details] [diff] [review]
Part 2: Make CleanupCachedXBLHandlers a instance method


> 
> // static
> void
>-nsGlobalWindow::CleanupCachedXBLHandlers(nsGlobalWindow* aWindow)
>+nsGlobalWindow::CleanupCachedXBLHandlers()
Well, the comment above still say this is static. Please remove that

Comment 7

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

Attachment: Part 3: Don't cast to nsGlobalWindow when working with the inner window linked list

After the window split is complete, the inner window linked list won't be
homogenously typed anymore, as there will be an nsGlobalWindowOuter member in
addition to the nsGlobalWindowInner members. This patch changes the code to
perform void* pointer comparisons before casting to nsGlobalWindowInner to avoid
this issue.

MozReview-Commit-ID: 56q5XodtGe7

Comment 8

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

Attachment: Part 3: Don't cast to nsGlobalWindow when working with the inner window linked list

void* is hard -- This patch fixes the code to correctly use PRCList* instead
which means that we don't mess up offsets and read random memory.

MozReview-Commit-ID: 56q5XodtGe7

Comment 9

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

Attachment: Part 3: Don't cast to nsGlobalWindow when working with the inner window linked list

MozReview-Commit-ID: 56q5XodtGe7

Comment 10

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

Attachment: Part 5: Fix some more consumers of nsGlobalWindow which I missed

I ran into these when I started to write the split patch.

There's some code here which we never seem to call which is broken - perhaps we want to remove it in the future?

MozReview-Commit-ID: DvMH4cVw5NK

Comment 11

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

Comment on attachment 8927004 [details] [diff] [review]
Part 5: Fix some more consumers of nsGlobalWindow which I missed


>+++ b/dom/base/test/gtest/TestParserDialogOptions.cpp
>@@ -2,16 +2,18 @@
> /* vim: set ts=8 sts=2 et sw=2 tw=80: */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>  * You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "gtest/gtest.h"
> #include "nsGlobalWindow.h"
> 
>+// XXX(nika): I can't find any non-test callers of this code, can we remove it?
>+
ask baku, but it isn't even clear what your question refers to.

Not sure about those other XXXs. I guess they are fine as such

Comment 12

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

baku. The file TestParserDialogOptions.cpp calls nsGlobalWindow::TokenizeDialogOptions and nsGlobalWindow::ConvertDialogOptions, both of which are functions which appear to not be called anywhere else in the codebase.

Can we remove the test and those functions?

Comment 13

[Andrea Marchesini [:baku]]

> Can we remove the test and those functions?

Sure.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fdfee217a97
Part 1: Make nsGlobalWindow.cpp use the specific {Inner,Outer} variants in more places internally, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6c2c5490f4
Part 2: Make CleanupCachedXBLHandlers a instance method, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a766cc68671
Part 3: Don't cast to nsGlobalWindow when working with the inner window linked list, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c0344711b6
Part 4: Modify some shared runnables to not refer directly to nsGlobalWindow, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56352d90f67
Part 5: Fix some more consumers of nsGlobalWindow which I missed, r=smaug

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8fdfee217a97
https://hg.mozilla.org/mozilla-central/rev/4a6c2c5490f4
https://hg.mozilla.org/mozilla-central/rev/9a766cc68671
https://hg.mozilla.org/mozilla-central/rev/55c0344711b6
https://hg.mozilla.org/mozilla-central/rev/c56352d90f67