Closed
Bug 316794
Opened 19 years ago
Closed 19 years ago
Move HandleDOMEvent() and Get/SetDocShell() from nsIScriptGlobalObject to nsPIDOMWindow
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: jst, Unassigned)
References
Details
Attachments
(3 files)
308.04 KB,
patch
|
Details | Diff | Splinter Review | |
303.55 KB,
patch
|
mrbkap
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
DOM event and docshell related methods just don't belong in nsIScriptGlobalObject. They're window methods and should be in nsPIDOMWindow. This change alone lets me remove *most* users of nsIScriptGlobalObject in the tree (outside the DOM code) and lets people deal with DOM window interfaces rather than script oriented interfaces for these things... Patch coming up.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Attachment #203320 -
Flags: superreview?(peterv)
Attachment #203320 -
Flags: review?(mrbkap)
Reporter | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Summary: Move HandleDOMEvent() and Get/SetDocShell() from nsIScriptGlobalObjcet to nsPIDOMWindow → Move HandleDOMEvent() and Get/SetDocShell() from nsIScriptGlobalObject to nsPIDOMWindow
Comment 3•19 years ago
|
||
Comment on attachment 203320 [details] [diff] [review] diff -w of the above. >Index: docshell/base/nsDocShell.cpp > // This will AddRef() aResult... This comment is slightly misleading, imo. The addref actually happens below. >Index: layout/xul/base/src/nsResizerFrame.cpp >@@ -131,25 +131,25 @@ nsResizerFrame::HandleEvent(nsPresContex > *aEventStatus = nsEventStatus_eConsumeNoDefault; > doDefault = PR_FALSE; > } > } > break; > > case NS_MOUSE_MOVE: { > if(mTrackingMouseMove) > { >- // get the document and the global script object - should this be cached? >- nsIScriptGlobalObject *scriptGlobalObject = >- aPresContext->PresShell()->GetDocument()->GetScriptGlobalObject(); >- NS_ENSURE_TRUE(scriptGlobalObject, NS_ERROR_FAILURE); >+ // get the document and the window - should this be cached? >+ nsPIDOMWindow *domWindow = >+ aPresContext->PresShell()->GetDocument()->GetWindow(); >+ NS_ENSURE_TRUE(domWindow, NS_ERROR_FAILURE); > > nsCOMPtr<nsIDocShellTreeItem> docShellAsItem = >- do_QueryInterface(scriptGlobalObject->GetDocShell()); >+ do_QueryInterface(domWindow->GetDocShell()); > NS_ENSURE_TRUE(docShellAsItem, NS_ERROR_FAILURE); This file seems to be using tabs and spaces inconsistently (even in the context provided here); but some of the added lines here add spaces and some of them add tabs, please use one or the other. >Index: layout/xul/base/src/nsTitleBarFrame.cpp >@@ -143,21 +142,21 @@ nsTitleBarFrame::HandleEvent(nsPresConte > *aEventStatus = nsEventStatus_eConsumeNoDefault; > doDefault = PR_FALSE; > } > } > break; > > case NS_MOUSE_MOVE: { > if(mTrackingMouseMove) > { >- // get the document and the global script object - should this be cached? >- nsCOMPtr<nsIDOMWindowInternal> >- window(do_QueryInterface(aPresContext->PresShell()->GetDocument()->GetScriptGlobalObject())); >+ // get the document and the window - should this be cached? >+ nsPIDOMWindow *window = >+ aPresContext->PresShell()->GetDocument()->GetWindow(); > Same thing here. >Index: mailnews/base/src/nsMsgWindow.cpp > NS_IMETHODIMP nsMsgWindow::SetDOMWindow(nsIDOMWindowInternal *aWindow) > { > if (!aWindow) > return NS_ERROR_NULL_POINTER; > > nsresult rv = NS_OK; > >- nsCOMPtr<nsIScriptGlobalObject> globalScript(do_QueryInterface(aWindow)); >+ nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(aWindow)); > nsIDocShell *docShell = nsnull; >- if (globalScript) >- docShell = globalScript->GetDocShell(); >+ if (win) >+ docShell = win->GetDocShell(); The first half of this function seems to be using tabs, but the rest of the context is using spaces, want to fix that? There are a few places where you turn strong refs into weak ones. I don't think they're problems (though the one in SetFocus sort of scared me, in case the window went away as a result of setting focus), but I thought I'd mention it here. r=mrbkap
Attachment #203320 -
Flags: review?(mrbkap) → review+
Comment 4•19 years ago
|
||
Comment on attachment 203320 [details] [diff] [review] diff -w of the above. Shouldn't you remove also nsXBLDocGlobalObject::HandleDOMEvent?
Reporter | ||
Comment 5•19 years ago
|
||
(In reply to comment #4) > (From update of attachment 203320 [details] [diff] [review] [edit]) > Shouldn't you remove also nsXBLDocGlobalObject::HandleDOMEvent? > Yes, good catch. Did that locally, and fixed the issues pointed out by mrbkap.
Comment 6•19 years ago
|
||
Comment on attachment 203320 [details] [diff] [review] diff -w of the above. >Index: content/events/src/nsEventStateManager.cpp >=================================================================== >@@ -631,22 +610,22 @@ nsEventStateManager::PreHandleEvent(nsPr > // Now we should fire the focus event. We fire it on the document, > // then the content node, then the window. > >- nsCOMPtr<nsIScriptGlobalObject> globalObject = >- GetDocumentOuterWindow(mDocument); >+ nsCOMPtr<nsPIDOMWindow> window = >+ mDocument->GetWindow(); Put this on one line. >Index: content/html/content/src/nsHTMLLegendElement.cpp >=================================================================== >@@ -252,23 +251,24 @@ nsHTMLLegendElement::SetFocus(nsPresCont >+ nsCOMPtr<nsIDOMElement> domElement = >+ do_QueryInterface((nsIContent *)this); NS_STATIC_CAST or use CallQueryInterface(this, getter_AddRefs(domElement)); (I prefer the latter) >Index: dom/src/base/nsGlobalWindow.cpp >=================================================================== >@@ -5226,49 +5219,47 @@ nsGlobalWindow::GetPrivateParent() > nsGlobalWindow::GetPrivateRoot() > { >+ NS_ASSERTION(ptop, "cannot get parentTop"); cannot get ptop? >Index: modules/plugin/base/src/ns4xPlugin.cpp >=================================================================== >@@ -1875,19 +1875,19 @@ _getvalue(NPP npp, NPNVariable variable, >- nsIDOMWindow *domWindow = inst->GetDOMWindow().get(); >+ nsPIDOMWindow *domWindow = inst->GetDOMWindow().get(); No need to change this. >Index: security/manager/boot/src/nsSecureBrowserUIImpl.cpp >=================================================================== >@@ -200,22 +199,22 @@ nsSecureBrowserUIImpl::Init(nsIDOMWindow > getter_AddRefs(mStringBundle)); > if (NS_FAILED(rv)) return rv; > > // hook up to the form post notifications: > nsCOMPtr<nsIObserverService> svc(do_GetService("@mozilla.org/observer-service;1", &rv)); > if (NS_SUCCEEDED(rv)) { > rv = svc->AddObserver(this, NS_FORMSUBMIT_SUBJECT, PR_TRUE); > } > >- nsCOMPtr<nsIScriptGlobalObject> sgo(do_QueryInterface(mWindow)); >- if (!sgo) return NS_ERROR_FAILURE; >+ nsCOMPtr<nsPIDOMWindow> piwindow(do_QueryInterface(mWindow)); >+ if (!window) return NS_ERROR_FAILURE; !piwindow?
Attachment #203320 -
Flags: superreview?(peterv) → superreview+
Comment 7•19 years ago
|
||
Comment on attachment 203320 [details] [diff] [review] diff -w of the above. > if (ourWindow) { >- nsIFocusController* focusController = ourWindow->GetRootFocusController(); >- nsIDOMElement* domElement = nsnull; >- CallQueryInterface(this, &domElement); >+ nsIFocusController* focusController = >+ ourWindow->GetRootFocusController(); >+ nsCOMPtr<nsIDOMElement> domElement = >+ do_QueryInterface((nsIContent *)this); > if (focusController && domElement) { > focusController->MoveFocus(PR_TRUE, domElement); > } > } Surely this suffices: if (ourWindow) { nsIFocusController* focusController = ourWindow->GetRootFocusController(); if (focusController) { focusController->MoveFocus(PR_TRUE, this); } }
Reporter | ||
Comment 8•19 years ago
|
||
This is what's keeping tinderbox orange, CanExecuteScript() is failing when loading a non-chrome URI as the app chrome.
Attachment #204414 -
Flags: superreview?(bzbarsky)
Attachment #204414 -
Flags: review?(bzbarsky)
Updated•19 years ago
|
Attachment #204414 -
Flags: superreview?(bzbarsky)
Attachment #204414 -
Flags: superreview+
Attachment #204414 -
Flags: review?(bzbarsky)
Attachment #204414 -
Flags: review+
Comment 9•19 years ago
|
||
Note this bug here probably caused a new assertion (judging from the checkins) and causes the orange on balsa on the Firefox tree. I filed Bug 318106 for this.
Reporter | ||
Comment 10•19 years ago
|
||
Fixes checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•