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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Unassigned)

References

Details

Attachments

(3 files)

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.
Attached patch FixSplinter Review
Attachment #203320 - Flags: superreview?(peterv)
Attachment #203320 - Flags: review?(mrbkap)
Status: NEW → ASSIGNED
Summary: Move HandleDOMEvent() and Get/SetDocShell() from nsIScriptGlobalObjcet to nsPIDOMWindow → Move HandleDOMEvent() and Get/SetDocShell() from nsIScriptGlobalObject to nsPIDOMWindow
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 on attachment 203320 [details] [diff] [review]
diff -w of the above.

Shouldn't you remove also nsXBLDocGlobalObject::HandleDOMEvent?
(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.
Blocks: 317240
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 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);
  }
}
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)
Attachment #204414 - Flags: superreview?(bzbarsky)
Attachment #204414 - Flags: superreview+
Attachment #204414 - Flags: review?(bzbarsky)
Attachment #204414 - Flags: review+
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.
Fixes checked in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Depends on: 324142
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: