Closed
Bug 1216401
Opened 10 years ago
Closed 9 years ago
Empty out nsIDOMWindow
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: khuey, Assigned: khuey)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 2 obsolete files)
265.66 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•10 years ago
|
||
Updated with a couple fixes for test failures.
Attachment #8676015 -
Attachment is obsolete: true
Attachment #8676015 -
Flags: review?(bzbarsky)
Attachment #8676404 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8676404 [details] [diff] [review]
Patch
Er, wrong patch
Attachment #8676404 -
Attachment is obsolete: true
Attachment #8676404 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 3•10 years ago
|
||
And the correct one.
Attachment #8676406 -
Flags: review?(bzbarsky)
![]() |
||
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
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•9 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?
![]() |
||
Comment 8•9 years ago
|
||
Landed a few hours ago - https://hg.mozilla.org/mozilla-central/rev/9c01b4d30bdd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 9•9 years ago
|
||
(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•9 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•9 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•9 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)
Assignee | ||
Comment 13•9 years ago
|
||
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•9 years ago
|
||
Fantastic, works, thanks!!
Comment 15•9 years ago
|
||
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.
Updated•9 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 16•9 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?
Assignee | ||
Comment 17•9 years ago
|
||
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•9 years ago
|
||
Yes, sorry, I shouldn't write method names from my memory. It's GetWindowRoot. Thanks for the answer, I'll do that.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•