1216401 - Empty out nsIDOMWindow

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

nsIDOMWindow isn't used from script anymore, so the methods/attributes on it fall into two categories

1. Those that are no longer used at all.
2. Those that are only used from C++.

We can remove the XPCOM methods for #1, and move those for #2 (which is a fairly small set) to nsPIDOMWindow.

After this patch nsIDOMWindow exists only to say "I'm passing a Window through WebIDL."

This patch is kind of big and mostly monotonous.  I would not object to you bouncing it to someone else.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Updated with a couple fixes for test failures.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8676404 [details] [diff] [review]
Patch

Er, wrong patch

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

And the correct one.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8676406 [details] [diff] [review]
Patch

>+++ b/accessible/base/DocManager.cpp
>@@ -172,21 +172,20 @@ DocManager::OnStateChange(nsIWebProgress* aWebProgress,
>   NS_ENSURE_STATE(DOMWindow);
>+  nsCOMPtr<nsPIDOMWindow> piWindow = do_QueryInterface(DOMWindow);

Probably best to reverse the order of those lines and NS_ENSURE_STATE(piWindow).

>+++ b/accessible/generic/ApplicationAccessible.cpp
>+        nsCOMPtr<nsIDocument> docNode = DOMWindow->GetDoc();
>+      if (docNode) {

Please fix the indent (outdent the GetDoc line by 2).

>+++ b/accessible/windows/msaa/nsWinUtils.cpp
>+  ErrorResult dummy;

You need to dummy.SuppressException() after the GetComputedStyle() call and before returning, right?

>+++ b/docshell/base/nsDocShell.cpp
>@@ -9722,17 +9721,17 @@ nsDocShell::InternalLoad(nsIURI* aURI,
>+          nsCOMPtr<nsPIDOMWindow> domWin = do_QueryInterface(targetDocShell->GetWindow());

No need for QI here, right?

>+++ b/dom/base/ThirdPartyUtil.cpp
>-    rv = current->GetScriptableParent(getter_AddRefs(parent));
>+    parent = current->GetScriptableParent();
>     NS_ENSURE_SUCCESS(rv, rv);

That NS_ENSURE_SUCCESS should go away, right?

>+++ b/dom/base/nsDocument.cpp
> nsIDocument::GetLocation() const
>+  ErrorResult dummy;

Need to SuppressException after the GetLocation() call, right?

>+++ b/dom/base/nsGlobalWindow.cpp
>+nsGlobalWindow::GetNavigator()
>+  ErrorResult dummy;
>+  return GetNavigator(dummy);

Need to SuppressException().

>+nsGlobalWindow::GetScreen()
>+  ErrorResult dummy;
>+  return GetScreen(dummy);

And here.

>+nsPIDOMWindow*
>+nsGlobalWindow::GetScriptableParent()

Can we make this return an already_AddRefed?  Or too much pain?

>  * nsIDOMWindow::GetParent (when called from C++)

There is no such thing.

>+nsGlobalWindow::GetApplicationCache()
>+  ErrorResult dummy;

Need to SuppressException().

>+nsGlobalWindow::GetOpener()
>+  ErrorResult dummy;

And here.

>+nsGlobalWindow::GetFullScreen()
>+  ErrorResult dummy;

And here.

> nsGlobalWindow::Open(const nsAString& aUrl, const nsAString& aName,
>+  if (NS_SUCCEEDED(rv) && window) {
>+    return window->QueryInterface(NS_GET_IID(nsPIDOMWindow), (void**)_retval);

  return CallQueryInterface(window, _retval);

please.

>@@ -9073,40 +8321,34 @@ nsGlobalWindow::RunPendingTimeoutsRecursive(nsGlobalWindow *aTopWindow,
>+  // XXXkhuey aren't we just getting ourselves here?
>+  nsCOMPtr<nsIDOMWindowCollection> frames = aWindow->GetFrames();

No, the C++ GetFrames() with no args doesn't return a window, and window doesn't implement nsIDOMWindowCollection.  It's a mess.  We should just nix the C++ GetFrames or something, in a followup.

>+nsGlobalWindow::GetFrameElement()
>+  ErrorResult dummy;

SuppressException().

>+already_AddRefed<nsISelection>
>+nsGlobalWindow::GetSelection()
> {
>+  nsCOMPtr<nsISelection> selection = GetSelectionOuter();

Wait, how do you know this is an outer?  You need to leave the forward-to-outer bits, no?

>+nsGlobalWindow::GetLocation()
>+  ErrorResult dummy;

SuppressException().

>+++ b/dom/base/nsObjectLoadingContent.cpp
>+  nsCOMPtr<nsIDocument> topDoc = topWindow->GetDoc();

Need to NS_ENSURE_TRUE(topDoc), no?

>+++ b/dom/base/nsPIDOMWindow.h
>+   * This property is "replaceable" in JavaScript.

Dunno that we need to document that here.

>+++ b/dom/events/EventStateManager.cpp
> GetWindowInnerRectCenter(nsPIDOMWindow* aWindow,
>+  nsGlobalWindow* window = static_cast<nsGlobalWindow*>(aWindow);

nsGlobalWindow::Cast(aWindow)

>+  ErrorResult dummy;

SuppressException() after each use.

>+++ b/dom/html/nsGenericHTMLElement.cpp
>+      window = top.get();

Just |window = top|.

>+++ b/dom/xml/nsXMLPrettyPrinter.cpp
>@@ -50,32 +49,33 @@ nsXMLPrettyPrinter::PrettyPrint(nsIDocument* aDocument,
>+                ErrorResult dummy;

SuppressException().

>+++ b/embedding/browser/nsContextMenuInfo.cpp
>@@ -255,32 +258,37 @@ nsContextMenuInfo::GetBackgroundImageRequestInternal(nsIDOMNode* aDOMNode,
>+    ErrorResult dummy;

SuppressException().

>+++ b/embedding/components/printingui/unixshared/nsPrintProgress.cpp
>-    NS_ENSURE_STATE(ownerWindow);
>+    NS_ENSURE_STATE(ownerWindow);\

Stray backslash.

>+++ b/layout/xul/nsXULPopupManager.cpp
>-    focusedDoc = do_QueryInterface(domDoc);
>+    nsCOMPtr<nsIDocument> doc = piWindow->GetDoc();

Please keep calling this focusedDoc.

>+++ b/layout/base/nsDocumentViewer.cpp
>@@ -4344,19 +4344,19 @@ nsDocumentViewer::OnDonePrinting()
>+          win->ForceClose();

Why ForceClose() here and Close() in other places?  At least needs documenting.

>+++ b/netwerk/protocol/http/nsHttpHandler.cpp
>-    nsCOMPtr<nsIDOMWindow> domWindow;
>+    nsCOMPtr<nsPIDOMWindow> domWindow;
>     cb->GetInterface(NS_GET_IID(nsIDOMWindow), getter_AddRefs(domWindow));

No, this is not OK, except insofar as nsPIDOMWindow inherits from nsIDOMWindow so the actual pointer value is the same.  This code should really use do_GetInterface, to avoid these sorts of issues!

I would much prefer we did the GetInterface into an nsCOMPtr<nsIDOMWindow> and then QIed, or did it into nsCOMPtr<nsPIDOMWindow> and checked that the interface requestor impls we care about support that interface.

>+++ b/widget/cocoa/nsMenuItemIconX.mm
>+    ErrorResult dummy;

SuppressException().

r=me with those fixed, especially the GetInterface thing and the nsGlobalWindow::GetSelection bits.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

All applied, except:

(In reply to Boris Zbarsky [:bz] from comment #4)
> >+nsPIDOMWindow*
> >+nsGlobalWindow::GetScriptableParent()
> 
> Can we make this return an already_AddRefed?  Or too much pain?

It is quite painful to do that right now.  Later, perhaps.

> >+already_AddRefed<nsISelection>
> >+nsGlobalWindow::GetSelection()
> > {
> >+  nsCOMPtr<nsISelection> selection = GetSelectionOuter();
> 
> Wait, how do you know this is an outer?  You need to leave the
> forward-to-outer bits, no?

I audited all the call sites (and changed any that weren't on the outer).

> >+++ b/dom/events/EventStateManager.cpp
> > GetWindowInnerRectCenter(nsPIDOMWindow* aWindow,
> >+  nsGlobalWindow* window = static_cast<nsGlobalWindow*>(aWindow);
> 
> nsGlobalWindow::Cast(aWindow)

Ugh, it's really annoying that nsGlobalWindow has a Cast and a FromSupports that do different things!

> >+++ b/netwerk/protocol/http/nsHttpHandler.cpp
> >-    nsCOMPtr<nsIDOMWindow> domWindow;
> >+    nsCOMPtr<nsPIDOMWindow> domWindow;
> >     cb->GetInterface(NS_GET_IID(nsIDOMWindow), getter_AddRefs(domWindow));
> 
> No, this is not OK, except insofar as nsPIDOMWindow inherits from
> nsIDOMWindow so the actual pointer value is the same.  This code should
> really use do_GetInterface, to avoid these sorts of issues!

Nice catch.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c01b4d30bdd

Comment 7

[[e-mail]]

(In reply to Kyle Huey from comment #0)
> After this patch nsIDOMWindow exists only to say "I'm passing a Window
> through WebIDL."
Doesn't script still need it to request a window from an interface requestor?

Comment 8

[Guilherme Lima]

Landed a few hours ago - https://hg.mozilla.org/mozilla-central/rev/9c01b4d30bdd

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to [e-mail] from comment #7)
> (In reply to Kyle Huey from comment #0)
> > After this patch nsIDOMWindow exists only to say "I'm passing a Window
> > through WebIDL."
> Doesn't script still need it to request a window from an interface requestor?

Yes, it's still there for that too.  Good point.

Comment 10

[aleth [:aleth]]

Still seeing
 
c:/builds/moz2_slave/tb-c-cen-w32-00000000000000000/build/mailnews/base/src/nsMessengerWinIntegration.cpp(127) : error C2039: 'Focus' : is not a member of 'nsIDOMWindow'

on Windows.

Comment 11

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #10)
> Still seeing
>  
> c:/builds/moz2_slave/tb-c-cen-w32-00000000000000000/build/mailnews/base/src/
> nsMessengerWinIntegration.cpp(127) : error C2039: 'Focus' : is not a member
> of 'nsIDOMWindow'
> 
> on Windows.

Oops, sorry, this should have gone in a c-c specific bug.

Comment 12

[Jorg K (CEST = GMT+2)]

The assertion at
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#7678
is busting Thunderbird. We can't even send a message out in a debug compile, see bug 1224840 comment #4.

Can you enlighten us why this change was made and what we're doing wrong or need to fix.

Before the code read:
FORWARD_TO_OUTER(OpenDialog, (aUrl, aName, aOptions, aExtraArgument, _retval), ...

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It was made because nothing in mozilla-central made that call on an inner window.  You can fix it by following http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgProgress.cpp#53 with

parent = parent->GetOuterWindow();
NS_ENSURE_ARG_POINTER(parent);

Comment 14

[Jorg K (CEST = GMT+2)]

Fantastic, works, thanks!!

Comment 15

[Eric Shepherd [:sheppy]]

Documentation updated: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDOMWindow

Also updated https://developer.mozilla.org/en-US/Firefox/Releases/44#XPCOM to include this change.

Comment 16

[Samuel Verschelde]

GetRootWindow() was removed with all the other functions, but I see no equivalent in nsPIDOMWindow.h that would give in return an nsIDOMEventTarget related to the window. Is there another way to connect event listeners to the window root and its nested windows?

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Are you talking about GetWindowRoot?  There never was a GetRootWindow, afaik.  You can just call GetTopWindowRoot and QI to nsIDOMEventTarget.  That's actually exactly what the old function did.  https://hg.mozilla.org/releases/mozilla-esr24/annotate/9cd2ab4d0029/dom/base/nsGlobalWindow.cpp#l6054

Comment 18

[Samuel Verschelde]

Yes, sorry, I shouldn't write method names from my memory. It's GetWindowRoot. Thanks for the answer, I'll do that.