This bug will do steps 3 and 4 of bug 558192 comment 6. We will move nsIDOMWindow to being a legacy XPIDL type like nsIDOMWindowInternal. Two new XPIDL interfaces will be introduced, mozIDOMWindow and mozIDOMWindowProxy, corresponding to inner and outer windows respectively. This will allow XPIDL entry points to specify whether they take inner or outer windows. Then nsPIDOMWindow will be replaced by nsPIDOMWindowInner (inheriting from mozIDOMWindow) and nsPIDOMWindowOuter (inheriting from mozIDOMWindowProxy). nsGlobalWindow will continue to be the concrete implementation class for all types. When the dust settles every use of nsIDOMWindow and nsPIDOMWindow in our code today will use a type that specifies whether the window we have is an inner or outer window.

Comment on attachment 8711825 [details] [diff] [review] Patch Review of attachment 8711825 [details] [diff] [review]: ----------------------------------------------------------------- Some over the shoulder comments from mrbkap. ::: docshell/base/nsDocShell.cpp @@ +3566,5 @@ > static bool > ItemIsActive(nsIDocShellTreeItem* aItem) > { > + if (nsCOMPtr<nsPIDOMWindowOuter> window = aItem->GetWindow()) { > + auto* win = static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(window.get())); nsGlobalWindow::Cast ::: dom/base/ScriptSettings.cpp @@ +425,2 @@ > { > + return Init(static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(aWindow)), aCx); nsGlobalWindow::Cast @@ +431,2 @@ > { > + return Init(static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(aWindow))); ibid @@ +449,2 @@ > { > + return InitWithLegacyErrorReporting(static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(aWindow))); ... ::: dom/base/nsFocusManager.cpp @@ +368,3 @@ > { > // only top-level windows can be made active > + nsCOMPtr<nsPIDOMWindowOuter> piWindow = nsPIDOMWindowOuter::From(aWindow); null check first @@ +433,5 @@ > { > // the focus method is stored on the inner window > + nsCOMPtr<nsPIDOMWindowOuter> window; > + if (aWindow) > + window = nsPIDOMWindowOuter::From(aWindow); braces! ::: dom/base/nsGlobalWindow.cpp @@ +1018,5 @@ > JS::Handle<jsid> id, > JS::MutableHandle<JS::Value> vp, > bool& found) const > { > + nsCOMPtr<nsPIDOMWindowOuter>frame = GetSubframeWindow(cx, proxy, id); ' ' after > @@ +2960,5 @@ > // are GC-owned. > for (RefPtr<nsGlobalWindow> inner = (nsGlobalWindow *)PR_LIST_HEAD(this); > inner != this; > inner = (nsGlobalWindow*)PR_NEXT_LINK(inner)) { > + MOZ_ASSERT(!inner->mOuterWindow || inner->mOuterWindow == this->AsOuter()); skip this-> @@ +3024,1 @@ > #endif Remove this assertion. @@ +3270,5 @@ > nsContentUtils::GetLocalizedString(nsContentUtils::eCOMMON_DIALOG_PROPERTIES, > "ScriptDialogLabel", label); > nsContentUtils::GetLocalizedString(nsContentUtils::eCOMMON_DIALOG_PROPERTIES, > "ScriptDialogPreventTitle", title); > + promptSvc->Confirm(this->AsOuter(), title.get(), label.get(), &disableDialog); Skip this-> @@ +3579,5 @@ > { > MOZ_RELEASE_ASSERT(IsInnerWindow()); > > if (!mNavigator) { > + mNavigator = new Navigator(this->AsInner()); Skip this-> @@ +3602,5 @@ > { > MOZ_RELEASE_ASSERT(IsInnerWindow()); > > if (!mScreen) { > + mScreen = nsScreen::Create(this->AsInner()); ibid @@ +3629,5 @@ > { > MOZ_RELEASE_ASSERT(IsInnerWindow()); > > if (!mHistory) { > + mHistory = new nsHistory(this->AsInner()); ibid @@ +3780,5 @@ > { > MOZ_RELEASE_ASSERT(IsInnerWindow()); > > if (!mSpeechSynthesis) { > + mSpeechSynthesis = new SpeechSynthesis(this->AsInner()); ... @@ +3814,2 @@ > if (mDocShell->GetIsBrowserOrApp()) { > + parent = this->AsOuter(); ... @@ +3863,4 @@ > return win.forget(); > } > > + nsCOMPtr<nsPIDOMWindowOuter> win(this->AsOuter()); .... @@ +4343,5 @@ > nsContentUtils::GetOfflineAppManifest(mDoc, getter_AddRefs(manifestURI)); > > RefPtr<nsDOMOfflineResourceList> applicationCache = > + new nsDOMOfflineResourceList(manifestURI, uri, mDoc->NodePrincipal(), > + this->AsInner()); ... @@ +4539,5 @@ > aError.Throw(NS_ERROR_DOM_SECURITY_ERR); > return; > } > > + win = xpc::WindowOrNull(unwrapped)->AsInner(); null check? add a test? @@ +6486,5 @@ > return false; > } > > nsCOMPtr<nsIPrompt> prompt; > + aError = promptFac->GetPrompt(this->AsOuter(), NS_GET_IID(nsIPrompt), this! @@ +6605,5 @@ > return; > } > > nsCOMPtr<nsIPrompt> prompt; > + aError = promptFac->GetPrompt(this->AsOuter(), NS_GET_IID(nsIPrompt), ... @@ +6720,5 @@ > // about:blank loaded. We don't want to focus our widget in that case. > // XXXbz should we really be checking for IsInitialDocument() instead? > bool lookForPresShell = true; > if (mDocShell->ItemType() == nsIDocShellTreeItem::typeChrome && > + GetPrivateRoot() == this->AsOuter() && mDoc) { ... @@ +6758,5 @@ > if (canFocus) { > // if there is no parent, this must be a toplevel window, so raise the > // window if canFocus is true. If this is a child process, the raise > // window request will get forwarded to the parent by the puppet widget. > + aError = fm->SetActiveWindow(this->AsOuter()); ... @@ +6803,5 @@ > // if the root is focused, clear the focus > nsIFocusManager* fm = nsFocusManager::GetFocusManager(); > if (fm && mDoc) { > nsCOMPtr<nsIDOMElement> element; > + fm->GetFocusedElementForWindow(this->AsOuter(), false, nullptr, getter_AddRefs(element)); ... @@ +6808,3 @@ > nsCOMPtr<nsIContent> content = do_QueryInterface(element); > if (content == mDoc->GetRootElement()) > + fm->ClearFocus(this->AsOuter()); ... @@ +7598,5 @@ > if (!IsPopupBlocked(mDoc)) > return true; > > + nsCOMPtr<nsPIDOMWindowOuter> parent = GetParent(); > + if (parent == this->AsOuter()) ... @@ +7829,3 @@ > nsGlobalWindow::GetFramesOuter() > { > + RefPtr<nsPIDOMWindowOuter> frames(this->AsOuter()); ... @@ +8298,5 @@ > from the list of browsers) (and has an unload handler > that closes the window). */ > // XXXbz now that we have mHavePendingClose, is this needed? > bool isTab = false; > + if (rootWin == this->AsOuter() || ... @@ +8553,3 @@ > if (!skipNukeCrossCompartment && window) { > + nsCOMPtr<mozIDOMWindow> inner = do_QueryInterface(window); > + nsCOMPtr<mozIDOMWindowProxy> outer = do_QueryInterface(window); Upcast to nsGlobalWindow and use that directly. @@ +9093,5 @@ > // should be the current search frame. > nsCOMPtr<nsIWebBrowserFindInFrames> framesFinder(do_QueryInterface(finder)); > if (framesFinder) { > + framesFinder->SetRootSearchFrame(this->AsOuter()); // paranoia > + framesFinder->SetCurrentSearchFrame(this->AsOuter()); ... @@ +9167,5 @@ > +nsPIDOMWindowOuter* > +nsGlobalWindow::GetOwnerGlobalForBindings() > +{ > + if (IsOuterWindow()) { > + return this->AsOuter(); Remove this-> @@ +9343,2 @@ > > + if (this->AsOuter() == parent) { ... @@ +9390,5 @@ > MOZ_RELEASE_ASSERT(IsInnerWindow()); > > nsIDocShell *docShell = GetDocShell(); > if (!mLocation && docShell) { > + mLocation = new nsLocation(this->AsInner(), docShell); ... @@ +9544,5 @@ > // update the chrome event handler on all our inner windows > for (nsGlobalWindow *inner = (nsGlobalWindow *)PR_LIST_HEAD(this); > inner != this; > inner = (nsGlobalWindow*)PR_NEXT_LINK(inner)) { > + NS_ASSERTION(!inner->mOuterWindow || inner->mOuterWindow == this->AsOuter(), ... @@ +9724,5 @@ > } > > nsIFocusManager* fm = nsFocusManager::GetFocusManager(); > + if (fm) { > + fm->WindowShown(this->GetOuterWindow(), oldNeedsFocus); ... @@ +9739,5 @@ > // is being destroyed. > > nsIFocusManager* fm = nsFocusManager::GetFocusManager(); > + if (fm) { > + fm->WindowHidden(this->GetOuterWindow()); ... @@ +9871,2 @@ > AutoJSAPI jsapi; > + result = jsapi.Init(this->AsInner()); ... @@ +10015,5 @@ > > if (!presShell) { > // Try flushing frames on our parent in case there's a pending > // style change that will create the presshell. > + nsGlobalWindow *parent = nsGlobalWindow::Cast(GetPrivateParent()); auto? @@ +10104,5 @@ > return nullptr; > } > > nsCOMPtr<nsIDOMStorage> storage; > + aError = storageManager->CreateStorage(this->AsInner(), principal, documentURI, .. @@ +10141,5 @@ > return nullptr; > } > > if (!mLocalStorage) { > + if (!DOMStorage::CanUseStorage(this->AsInner())) { ... @@ +10165,5 @@ > mDoc->GetDocumentURI(documentURI); > } > > nsCOMPtr<nsIDOMStorage> storage; > + aError = storageManager->CreateStorage(this->AsInner(), principal, documentURI, ... @@ +10186,5 @@ > MOZ_RELEASE_ASSERT(IsInnerWindow()); > if (!mIndexedDB) { > // This may keep mIndexedDB null without setting an error. > + aError = IDBFactory::CreateForWindow(this->AsInner(), > + getter_AddRefs(mIndexedDB)); ... @@ +10292,3 @@ > > nsContentUtils::StorageAccess access = > + nsContentUtils::StorageAllowedForWindow(this->AsInner()); ... @@ +11254,5 @@ > docShell->GetChildAt(i, getter_AddRefs(childShell)); > NS_ASSERTION(childShell, "null child shell"); > > + if (nsCOMPtr<nsPIDOMWindowOuter> pWin = childShell->GetWindow()) { > + nsGlobalWindow *win = nsGlobalWindow::Cast(pWin); auto? @@ +11284,2 @@ > > + if (parent && parent != this->AsOuter()) { ... @@ +11433,5 @@ > > if (!aCalledNoScript) { > // We asserted at the top of this function that aNavigate is true for > // !aCalledNoScript. > + rv = pwwatch->OpenWindow2(this->AsOuter(), url.get(), name_ptr, ... @@ +11453,5 @@ > nojsapi.emplace(); > } > > > + rv = pwwatch->OpenWindow2(this->AsOuter(), url.get(), name_ptr, ... @@ +11534,1 @@ > if (!forwardTo) { Followup on why this is here? @@ +11538,5 @@ > > // If the caller and the callee share the same outer window, forward to the > // caller inner. Else, we forward to the current inner (e.g. someone is > // calling setTimeout() on a reference to some other window). > + if (forwardTo->GetOuterWindow() != this->AsOuter() || ... @@ +11553,5 @@ > > // If forwardTo is not the window with an active document then we want the > // call to setTimeout/Interval to be a noop, so return null but don't set an > // error. > + MOZ_ASSERT(forwardTo->IsInnerWindow()); Drop the redundant assertion. @@ +12578,2 @@ > } else { > + mozilla::dom::workers::SuspendWorkersForWindow(this->AsInner()); ... @@ +12618,5 @@ > docShell->GetChildAt(i, getter_AddRefs(childShell)); > NS_ASSERTION(childShell, "null child shell"); > > + if (nsCOMPtr<nsPIDOMWindowOuter> pWin = childShell->GetWindow()) { > + nsGlobalWindow* win = nsGlobalWindow::Cast(pWin); auto? @@ +12670,2 @@ > } else { > + mozilla::dom::workers::ResumeWorkersForWindow(this->AsInner()); ... @@ +12847,5 @@ > > void > nsGlobalWindow::EnableTimeChangeNotifications() > { > + mozilla::time::AddWindowListener(this->AsInner()); ... @@ +12853,5 @@ > > void > nsGlobalWindow::DisableTimeChangeNotifications() > { > + mozilla::time::RemoveWindowListener(this->AsInner()); ... @@ +12874,5 @@ > // Multiple global windows can share a document. So only measure the > // document if it (a) doesn't have a global window, or (b) it's the > // primary document for the window. > + if (!mDoc->GetInnerWindow() || > + mDoc->GetInnerWindow() == this->AsInner()) { ... @@ +13672,5 @@ > { > MOZ_RELEASE_ASSERT(IsInnerWindow()); > > if (!mConsole) { > + mConsole = new Console(this->AsInner()); ... ::: dom/base/nsGlobalWindow.h @@ +537,5 @@ > > inline nsGlobalWindow* GetScriptableTopInternal() > { > + nsPIDOMWindowOuter* top = GetScriptableTop(); > + return static_cast<nsGlobalWindow*>((nsPIDOMWindow<nsISupports>*)top); nsGlobalWindow::Cast? ::: dom/base/nsINode.cpp @@ +1329,4 @@ > nsINode::GetOwnerGlobalForBindings() > { > bool dummy; > + auto* window = static_cast<nsGlobalWindow*>(OwnerDoc()->GetScriptHandlingObject(dummy)); nsGlobalWindow::Cast ::: dom/base/nsPIDOMWindow.h @@ +67,5 @@ > UIStateChangeType_Set, > UIStateChangeType_Clear > }; > > +enum FullscreenReason enum class? @@ +771,5 @@ > + return static_cast<nsPIDOMWindowInner*>(aFrom); > + } > + > + // Returns true if this object has an outer window and it is the current inner > + // window of that outer. Only call this on inner windows. Remove the obvious. ::: dom/base/nsPIDOMWindowInlines.h @@ +1,1 @@ > +template<class T> License header. @@ +43,5 @@ > +template <class T> > +bool > +nsPIDOMWindow<T>::IsLoading() const > +{ > + const nsPIDOMWindowInner* win; move inside the branch @@ +71,5 @@ > +bool > +nsPIDOMWindow<T>::IsHandlingResizeEvent() const > +{ > + if (IsOuterWindow()) { > + const nsPIDOMWindowInner* win = AsOuter()->GetCurrentInnerWindow(); indentation? @@ +127,5 @@ > +{ > + if (IsOuterWindow()) { > + return mInnerWindow->GetFocusedNode(); > + } > + return mFocusedNode; \n? ::: dom/interfaces/smil/nsIDOMTimeEvent.idl @@ +5,5 @@ > > #include "nsIDOMEvent.idl" > > +interface mozIDOMWindowProxy; > + axe ::: js/xpconnect/src/XPCConvert.cpp @@ +931,5 @@ > + // want the outer! nsGlobalWindow will give us the outer via > + // getInterface. > + if (iid->Equals(NS_GET_IID(mozIDOMWindowProxy))) { > + nsCOMPtr<nsIInterfaceRequestor> ireq = do_QueryInterface(iface); > + return ireq && NS_SUCCEEDED(ireq->GetInterface(*iid, dest)); Do this by getting the inner explicitly. ::: js/xpconnect/wrappers/XrayWrapper.cpp @@ +1177,2 @@ > nsContentUtils::XPConnect()->GetNativeOfWrapper(cx, target)); > + return static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(piWin.get())); nsGlobalWindow::Cast @@ +1929,5 @@ > nsAutoJSString name; > if (!name.init(cx, JSID_TO_STRING(id))) > return false; > + if (nsCOMPtr<nsPIDOMWindowOuter> childDOMWin = win->GetChildWindow(name)) { > + nsGlobalWindow* cwin = static_cast<nsGlobalWindow*>(reinterpret_cast<nsPIDOMWindow<nsISupports>*>(childDOMWin.get())); And here.

Comment on attachment 8711825 [details] [diff] [review] Patch Review of attachment 8711825 [details] [diff] [review]: ----------------------------------------------------------------- I did a few spot checks and things generally look good. I'd be good to get Olli's stamp on the event stuff. I'm more confident in this because the type system is helping us out. There are a couple of places where you left comments that should be addressed before landing. I'd also appreciate a comment in nsGlobalWindow explaining the nsPIDOMWindow<nsISupports> stuff. ::: dom/base/nsXMLHttpRequest.cpp @@ +306,5 @@ > nsILoadGroup* aLoadGroup) > { > NS_ENSURE_ARG_POINTER(aPrincipal); > + /* > + if (nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(aGlobalObject)) { This looks like it should just be removed. ::: dom/html/HTMLMediaElement.cpp @@ +5032,5 @@ > > if (aCapture != mAudioCapturedByWindow) { > if (aCapture) { > mAudioCapturedByWindow = true; > + nsCOMPtr<nsPIDOMWindowInner> window =OwnerDoc()->GetInnerWindow(); Nit: Space after =. ::: hal/sandbox/SandboxHal.cpp @@ +521,5 @@ > PBrowserParent *browserParent) override > { > // We give all content vibration permission. > + // TabParent *tabParent = TabParent::GetFrom(browserParent); > + /* xxxkhuey wtf Can't you just pass the nsPIDOMWindowInner through here?

Olli, could you look at the event handling changes?

Comment on attachment 8713303 [details] [diff] [review] Patch OT, I wonder why InitMessageEvent takes cx as param. MouseScrollEvent::Axis() { return mEvent->AsMouseScrollEvent()->isHorizontal ? - static_cast<int32_t>(HORIZONTAL_AXIS) : - static_cast<int32_t>(VERTICAL_AXIS); + static_cast<int32_t>(nsIDOMMouseScrollEvent::HORIZONTAL_AXIS) : + static_cast<int32_t>(nsIDOMMouseScrollEvent::VERTICAL_AXIS); } Oh, right, we don't expose webidl constants in any sane way to C++ :/ +void +UIEvent::InitUIEvent(const nsAString& typeArg, + bool canBubbleArg, + bool cancelableArg, + nsGlobalWindow* viewArg, + int32_t detailArg) I could complain about wrong naming of the params, but since the other init has the same stuff, fine. (but if you fix this kind of stuff, fix this too) So UIEvent has stored usually inner window currently, but your change makes it store outer. I'd prefer inner, since C++ callers should usually play with inner window. But - I guess it doesn't matter much since there aren't C++ callers and the new setup guarantees that one knows which object is returned. However, in general C++ side should play with inner whenever possible. History has shown that using outer leads easily to errors. void InitCommandEvent(const nsAString& aType, bool aCanBubble, bool aCancelable, - nsIDOMWindow* aView, + nsGlobalWindow* aView, int32_t aDetail, bool aCtrlKey, bool aAltKey, bool aShiftKey, bool aMetaKey, - Event* aSourceEvent, - ErrorResult& aRv) + Event* aSourceEvent) { - aRv = InitCommandEvent(aType, aCanBubble, aCancelable, aView, aDetail, - aCtrlKey, aAltKey, aShiftKey, aMetaKey, - aSourceEvent); + InitCommandEvent(aType, aCanBubble, aCancelable, aView->AsInner(), + aDetail, aCtrlKey, aAltKey, aShiftKey, aMetaKey, + aSourceEvent); This is a tad error prone that aView->AsInner() is called always without knowing that aView is inner, but I guess C++ callers should be caught asap by the assertion in AsInner(). so fine. Not about events but.. AudioTrackList* HTMLMediaElement::AudioTracks() { if (!mAudioTrackList) { - nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(OwnerDoc()->GetParentObject()); + nsCOMPtr<nsPIDOMWindowInner> window = OwnerDoc()->GetInnerWindow(); mAudioTrackList = new AudioTrackList(window, this); } return mAudioTrackList; } Why is that ok? GetParentObject() may return non-null in cases when GetInnerWindow() returns null. data document do have parent object, but don't have inner window. Same also elsewhere. // This is safe since we're on main-thread, and the windowlist can only // be added to from the main-thread - nsPIDOMWindow *window = static_cast<nsPIDOMWindow*> - (nsGlobalWindow::GetInnerWindowWithId(aWindowID)); + nsPIDOMWindowInner* window = + nsGlobalWindow::GetInnerWindowWithId(aWindowID)->AsInner(); if (window) { Why it is safe to call ->AsInner()? GetInnerWindowWithId may return null, afais. Same also elsewhere. anyhow, r+ for the event stuff, although, I'd prefer storing inner window as member variables, not outer. But I can live with the setup the patch has. (looks like also TimeEvent has mView similar to UIEvent)

https://hg.mozilla.org/mozilla-central/rev/e22b3043887e https://hg.mozilla.org/mozilla-central/rev/998d61de14ee

Looks like there's an issue in toolkit/: In file included from /Users/buildbot/buildslave/macosx/build/mozilla/toolkit/components/downloads/nsDownloadManager.cpp:25: In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsILocalFileWin.h:10: In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsILocalFile.h:10: In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsIFile.h:920: In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsDirectoryServiceUtils.h:10: In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsIServiceManager.h:138: In file included from /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsComponentManagerUtils.h:11: /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsCOMPtr.h:390:5: error: static_assert failed "nsCOMPtr only works for types with IIDs. Either use RefPtr; add an IID to your type with NS_DECLARE_STATIC_IID_ACCESSOR/NS_DEFINE_STATIC_IID_ACCESSOR; or make the nsCOMPtr point to a base class with an IID." static_assert(1 < sizeof(TestForIID<T>(nullptr)), "nsCOMPtr only works " ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/buildbot/buildslave/macosx/build/obj-instantbird/x86_64/dist/include/nsCOMPtr.h:430:5: note: in instantiation of member function 'nsCOMPtr<mozIDOMWindowProxy>::assert_validity' requested here assert_validity(); ^ /Users/buildbot/buildslave/macosx/build/mozilla/toolkit/components/downloads/nsDownloadManager.cpp:2576:32: note: in instantiation of member function 'nsCOMPtr<mozIDOMWindowProxy>::nsCOMPtr' requested here nsCOMPtr<mozIDOMWindowProxy> dmWindow; ^ 1 error generated.

Sounds like some sort of unified build issue. An include missing somewhere, most likely.

Adding #include "mozIDOMWindow.h" to nsDownloadManager.h fixes the build failure, but I'm not sure if it's really the right place to add it.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > Sounds like some sort of unified build issue. An include missing somewhere, > most likely. It's not a OSX unified build issue as it also happens on Linux.

(In reply to aleth [:aleth] from comment #12) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > > Sounds like some sort of unified build issue. An include missing somewhere, > > most likely. > > It's not a OSX unified build issue as it also happens on Linux. I believe Kyle is referring to the "unified compilation" technique where multiple C++ files are compiled together as a group to reduce compile time. This is done on all platforms and can cause compilation errors when disabled since there can be #includes missing in the individual files that aren't needed when compiled as group.