Empty out nsIDOMWindow

RESOLVED FIXED in mozilla44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

({dev-doc-complete})

unspecified
mozilla44
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 8676015 [details] [diff] [review]
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.
Attachment #8676015 - Flags: review?(bzbarsky)
Keywords: dev-doc-needed
Created attachment 8676404 [details] [diff] [review]
Patch

Updated with a couple fixes for test failures.
Attachment #8676015 - Attachment is obsolete: true
Attachment #8676015 - Flags: review?(bzbarsky)
Attachment #8676404 - Flags: review?(bzbarsky)
Comment on attachment 8676404 [details] [diff] [review]
Patch

Er, wrong patch
Attachment #8676404 - Attachment is obsolete: true
Attachment #8676404 - Flags: review?(bzbarsky)
Created attachment 8676406 [details] [diff] [review]
Patch

And the correct one.
Attachment #8676406 - Flags: review?(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.
Attachment #8676406 - Flags: review?(bzbarsky) → review+
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 7

3 years ago
(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?

Updated

3 years ago
Blocks: 1218875

Comment 8

3 years ago
Landed a few hours ago - https://hg.mozilla.org/mozilla-central/rev/9c01b4d30bdd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(In reply to neil@parkwaycc.co.uk 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

3 years ago
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

3 years ago
(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

3 years ago
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), ...
Flags: needinfo?(khuey)
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);
Flags: needinfo?(khuey)

Comment 14

3 years ago
Fantastic, works, thanks!!

Updated

3 years ago
Depends on: 1224840
Keywords: dev-doc-needed → dev-doc-complete

Comment 16

3 years ago
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?
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

3 years ago
Yes, sorry, I shouldn't write method names from my memory. It's GetWindowRoot. Thanks for the answer, I'll do that.
You need to log in before you can comment on or make changes to this bug.