316794 - Move HandleDOMEvent() and Get/SetDocShell() from nsIScriptGlobalObject to nsPIDOMWindow

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

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.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Fix

Comment 2

[Johnny Stenback (:jst)]

Attachment: diff -w of the above.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 203320 [details] [diff] [review]
diff -w of the above.

Shouldn't you remove also nsXBLDocGlobalObject::HandleDOMEvent?

Comment 5

[Johnny Stenback (:jst)]

(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

[Peter Van der Beken [:peterv]]

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?

Comment 7

[[e-mail]]

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);
  }
}

Comment 8

[Johnny Stenback (:jst)]

Attachment: Fix CanExecuteScript().

This is what's keeping tinderbox orange, CanExecuteScript() is failing when loading a non-chrome URI as the app chrome.

Comment 9

[Frank Wein [:mcsmurf]]

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.

Comment 10

[Johnny Stenback (:jst)]

Fixes checked in.