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

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: jst, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

13 years ago
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 2

13 years ago
Created attachment 203320 [details] [diff] [review]
diff -w of the above.
Attachment #203320 - Flags: superreview?(peterv)
Attachment #203320 - Flags: review?(mrbkap)
(Reporter)

Updated

13 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 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

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

Shouldn't you remove also nsXBLDocGlobalObject::HandleDOMEvent?
(Reporter)

Comment 5

13 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 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

13 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

13 years ago
Created attachment 204414 [details] [diff] [review]
Fix CanExecuteScript().

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+

Comment 9

13 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

13 years ago
Fixes checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.