The basic idea here is to move forwarding macros back to the XPCOM entry points. WebIDL entry points should be inner only, and implementations that belong on the outer are now separate functions that we forward to. A handful of callsites elsewhere in Gecko need to be fixed up to accomodate this.

All WebIDL entry points should be inner only anyways, so move forwarding back to the XPCOM versions where appropriate.

This makes FORWARD_TO_OUTER_OR_THROW assert that we're already in an inner, and splits a number of functions into WebIDL entry point Foo() and implementation FooOuter() that lives on the outer window.

Comment on attachment 8631904 [details] [diff] [review] Part 1: Remove FORWARD_TO_INNER_OR_THROW Review of attachment 8631904 [details] [diff] [review]: ----------------------------------------------------------------- I'm a bit torn, it looks safe but I'm worried that we might miss some callers. Maybe it'd make sense to replace the forwards with MOZ_RELEASE_ASSERT? The assert should all eventually go away once you do the real split, right?

Yeah, it'll all get scrapped eventually. This is just to segregate the entry points for future splitting. The XPCOM forwards need to remain for now. There's a bunch of code that depends on them.

We could make these assertions MOZ_RELEASE_ASSERT though. I didn't do it because I was worried about perf. What do you think?

Comment on attachment 8631908 [details] [diff] [review] Part 2: Rework FORWARD_TO_OUTER_OR_THROW Review of attachment 8631908 [details] [diff] [review]: ----------------------------------------------------------------- I find that the Outer suffix leads to weird names, I think I slightly prefer something like a OnOuter suffix or a Outer prefix. But I don't care much given that these should also be temporary, right? ::: dom/base/nsGlobalWindow.cpp @@ +6014,5 @@ > +nsGlobalWindow::GetScrollMaxXY(int32_t* aScrollMaxX, int32_t* aScrollMaxY, > + ErrorResult& aError) > +{ > + FORWARD_TO_OUTER_OR_THROW(GetScrollMaxXYOuter, (aScrollMaxX, aScrollMaxY), > + aError, ); Whitespace is off. @@ -7974,2 @@ > { > - FORWARD_TO_OUTER_OR_THROW(GetFrames, (aError), aError, nullptr); Should probably replace this with a IsOuterWindow assert.

Comment on attachment 8631909 [details] [diff] [review] Part 3: Miscellaneous assertions, remove nsIDOMJSWindow. Review of attachment 8631909 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +4257,3 @@ > > nsDOMWindowList* windows = GetWindowList(); > NS_ENSURE_TRUE(windows, nullptr); Hmm, we're not setting aFound anymore sometimes. However, it looks like that's actually unused anyway, so maybe we could just remove it while we're changing the signature anyway? @@ +4261,5 @@ > return windows->IndexedGetter(aIndex, aFound); > } > > +already_AddRefed<nsIDOMWindow> > +nsGlobalWindow::IndexedGetter(uint32_t aIndex, bool& aFound) Can't you actually remove this? If so then you could rename IndexedGetterOuter to IndexedGetter again.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6) > We could make these assertions MOZ_RELEASE_ASSERT though. I didn't do it > because I was worried about perf. What do you think? Aren't we already doing the check anyway for things called from bindings? If so, then I don't think I'd worry so much about perf.

(In reply to Peter Van der Beken [:peterv] from comment #8) > Comment on attachment 8631909 [details] [diff] [review] > Part 3: Miscellaneous assertions, remove nsIDOMJSWindow. > > Review of attachment 8631909 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/nsGlobalWindow.cpp > @@ +4257,3 @@ > > > > nsDOMWindowList* windows = GetWindowList(); > > NS_ENSURE_TRUE(windows, nullptr); > > Hmm, we're not setting aFound anymore sometimes. However, it looks like > that's actually unused anyway, so maybe we could just remove it while we're > changing the signature anyway? Done. > @@ +4261,5 @@ > > return windows->IndexedGetter(aIndex, aFound); > > } > > > > +already_AddRefed<nsIDOMWindow> > > +nsGlobalWindow::IndexedGetter(uint32_t aIndex, bool& aFound) > > Can't you actually remove this? If so then you could rename > IndexedGetterOuter to IndexedGetter again. It gets called on the inner via xrays.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a7ab1cb95bb https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb87a230841 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3c9358b570

https://hg.mozilla.org/mozilla-central/rev/6a7ab1cb95bb https://hg.mozilla.org/mozilla-central/rev/7bb87a230841 https://hg.mozilla.org/mozilla-central/rev/cd3c9358b570

I think we may need to back this out until bug 185793 is resolved, this is currently the cause of the #2 most verbose warning during testing.

* bug 1183888 that is

No. Do not back this out. Let's just fix bug 1183888.

Er, I assume you _really_ meant bug 1185793.