The default bug view has changed. See this FAQ.

Status

()

Core
DOM
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

13 years ago
Lots of things that could be cleaned up in those interfaces... patch coming up.
(Assignee)

Comment 1

13 years ago
Created attachment 140762 [details] [diff] [review]
deCOMtaminate.
(Assignee)

Comment 2

13 years ago
Created attachment 140763 [details] [diff] [review]
deCOMtaminate (diff -w of the above).
(Assignee)

Updated

13 years ago
Attachment #140763 - Flags: superreview?(peterv)
Attachment #140763 - Flags: review?(peterv)
(Assignee)

Comment 3

13 years ago
FYI, there's a couple of silly errors in this patch that makes it not compile
(from last-minute changes that were not compiled before the full-tree diffs were
made). Nothing that should affect reviews tho...
(Assignee)

Updated

13 years ago
Attachment #140763 - Flags: superreview?(peterv)
Attachment #140763 - Flags: superreview?(bryner)
Attachment #140763 - Flags: review?(peterv)
Attachment #140763 - Flags: review?(bryner)
Comment on attachment 140763 [details] [diff] [review]
deCOMtaminate (diff -w of the above).

>Index: caps/src/nsScriptSecurityManager.cpp
>-already_AddRefed<nsIScriptContext>
>+static nsIScriptContext *
> GetScriptContext(JSContext *cx)
> {
>-    nsIScriptContext *scriptContext;
>-    GetScriptContextFromJSContext(cx, &scriptContext);
>-
>-    return scriptContext;
>+    return GetScriptContextFromJSContext(cx);
> }
> 

Hm, you could just eliminate this in favor of changing call callers to use
GetScriptContextFromJSContext... is this just to save on code size by only
inlining GetScriptContextFromJSContext once?

>Index: content/base/src/nsContentUtils.cpp
>@@ -152,88 +152,79 @@ nsContentUtils::GetParserServiceWeakRef(
>+nsIScriptGlobalObject *
>+nsContentUtils::GetStaticScriptGlobal(JSContext* aContext, JSObject* aObj)
> {
>   if (!sXPConnect) {
>-    *aNativeGlobal = nsnull;
>-
>-    return NS_OK;
>+    return nsnull;
>   }
> 
>   JSObject* parent;
>   JSObject* glob = aObj; // starting point for search
> 
>   if (!glob)
>-    return NS_ERROR_FAILURE;
>+    return nsnull;
> 
>   while (nsnull != (parent = JS_GetParent(aContext, glob))) {
>     glob = parent;
>   }
> 
>   nsCOMPtr<nsIXPConnectWrappedNative> wrapped_native;
> 
>   nsresult rv =
>     sXPConnect->GetWrappedNativeOfJSObject(aContext, glob,
>                                            getter_AddRefs(wrapped_native));
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_SUCCESS(rv, nsnull);
> 
>   nsCOMPtr<nsISupports> native;
>   rv = wrapped_native->GetNative(getter_AddRefs(native));
>-  NS_ENSURE_SUCCESS(rv, rv);

Don't bother assigning to rv here if it's not going to be used for anything.

>Index: content/base/src/nsRange.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsRange.cpp,v
>retrieving revision 1.178
>diff -u -9 -p -w -r1.178 nsRange.cpp
>--- content/base/src/nsRange.cpp	3 Feb 2004 23:23:09 -0000	1.178
>+++ content/base/src/nsRange.cpp	6 Feb 2004 21:44:58 -0000
>@@ -2484,32 +2484,32 @@ nsRange::CreateContextualFragment(const 
>         // Just to compare, not to use!
>         result = secMan->GetSystemPrincipal(getter_AddRefs(sysPrin));
>         if (NS_SUCCEEDED(result))
>             result = secMan->GetSubjectPrincipal(getter_AddRefs(subjectPrin));
>         // If there's no subject principal, there's no JS running, so we're in system code.
>         // (just in case...null subject principal will probably never happen)
>         if (NS_SUCCEEDED(result) &&
>            (!subjectPrin || sysPrin.get() == subjectPrin.get())) {
>           nsIScriptGlobalObject *globalObj = document->GetScriptGlobalObject();
>+          JSContext* cx = nsnull;
> 
>-          nsCOMPtr<nsIScriptContext> scriptContext;
>           if (globalObj) {
>-            result = globalObj->GetContext(getter_AddRefs(scriptContext));
>-          }
>+            nsIScriptContext *scriptContext = nsnull;
>+            scriptContext = globalObj->GetContext();

Combine these into a single line.

> 
>-          JSContext* cx = nsnull;
>-          if (NS_SUCCEEDED(result) && scriptContext) {
>+            if (scriptContext) {
>             cx = (JSContext*)scriptContext->GetNativeContext();
>           }
>+          }
> 
>           if(cx) {
>-            ContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1", &result);
>-            if(NS_SUCCEEDED(result)) {
>+            ContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+            if(ContextStack) {

Can you clean up the parentheses style to match the rest of the function while
you're here (space after 'if')?

>               result = ContextStack->Push(cx);
>             }
>           }
>         }
>       }
> 
>       nsDTDMode mode = eDTDMode_autodetect;
>       nsCOMPtr<nsIHTMLDocument> htmlDoc(do_QueryInterface(domDocument));
>       if (htmlDoc) {

>Index: dom/public/nsIJSEventListener.h
>+  virtual ~nsIJSEventListener()
>+  {
>+    NS_IF_RELEASE(mContext);
>+  }

This destructor should not need to be virtual unless someone will be calling
|delete| on a nsIJSEventListener pointer (which they'd better not be).

>Index: embedding/components/printingui/src/win/nsPrintingPromptService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/components/printingui/src/win/nsPrintingPromptService.cpp,v
>retrieving revision 1.12
>diff -u -9 -p -w -r1.12 nsPrintingPromptService.cpp
>--- embedding/components/printingui/src/win/nsPrintingPromptService.cpp	19 Nov 2003 06:34:45 -0000	1.12
>+++ embedding/components/printingui/src/win/nsPrintingPromptService.cpp	6 Feb 2004 21:45:20 -0000
>@@ -135,24 +135,21 @@ nsPrintingPromptService::GetHWNDForDOMWi
>         {
>             HWND w;
>             site->GetSiteWindow(reinterpret_cast<void **>(&w));
>             return w;
>         }
>     }
> 
>     // Now we might be the Browser so check this path
>     nsCOMPtr<nsIScriptGlobalObject> scriptGlobal(do_QueryInterface(aWindow));
>-    nsCOMPtr<nsIDocShell> docShell;
>-    scriptGlobal->GetDocShell(getter_AddRefs(docShell));
>-    if (!docShell) return nsnull;
> 
>     nsCOMPtr<nsIDocShellTreeItem> treeItem;
>-    treeItem = do_QueryInterface(docShell);
>+    treeItem = do_QueryInterface(scriptGlobal->GetDocShell());

Maybe do this all on one line.

>Index: extensions/inspector/base/src/inLayoutUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/base/src/inLayoutUtils.cpp,v
>retrieving revision 1.33
>diff -u -9 -p -w -r1.33 inLayoutUtils.cpp
>--- extensions/inspector/base/src/inLayoutUtils.cpp	22 Oct 2003 06:09:40 -0000	1.33
>+++ extensions/inspector/base/src/inLayoutUtils.cpp	6 Feb 2004 21:45:23 -0000
>@@ -77,20 +77,19 @@ inLayoutUtils::GetWindowFor(nsIDOMDocume
>   
>   nsCOMPtr<nsIDOMWindowInternal> window = do_QueryInterface(view);
>   return window;
> }
> 
> nsIPresShell* 
> inLayoutUtils::GetPresShellFor(nsISupports* aThing)
> {
>   nsCOMPtr<nsIScriptGlobalObject> so = do_QueryInterface(aThing);
>-  nsCOMPtr<nsIDocShell> docShell;
>-  so->GetDocShell(getter_AddRefs(docShell));
>+  nsIDocShell *docShell = so->GetDocShell();
>   
>   nsCOMPtr<nsIPresShell> presShell;
>   docShell->GetPresShell(getter_AddRefs(presShell));
> 

Since this isn't being null checked you could just chain the calls.

Just a general comment - I'm wondering about the efficiency of a few places
where you QI into a nsCOMPtr and then return it as a raw pointer.  If you QI'd
it into a raw pointer and returned it as |already_AddRefed|, you'd save an
addref and release if the caller is putting the value into a nsCOMPtr.	On the
other hand, it may increase code size a bit since you'll be spreading out
Release() calls to call sites that don't need to now.  Just something to
consider; do what you think is best.

r+sr=bryner with those changes.
Attachment #140763 - Flags: superreview?(bryner)
Attachment #140763 - Flags: superreview+
Attachment #140763 - Flags: review?(bryner)
Attachment #140763 - Flags: review+
(Assignee)

Comment 5

13 years ago
(In reply to comment #4)
> (From update of attachment 140763 [details] [diff] [review])
> >Index: caps/src/nsScriptSecurityManager.cpp
> >-already_AddRefed<nsIScriptContext>
> >+static nsIScriptContext *
> > GetScriptContext(JSContext *cx)
> > {
...
> >+    return GetScriptContextFromJSContext(cx);
> > }
> > 
> 
> Hm, you could just eliminate this in favor of changing call callers to use
> GetScriptContextFromJSContext... is this just to save on code size by only
> inlining GetScriptContextFromJSContext once?

Yup, just saving code size here, so I'll leave it as is...

...
> Just a general comment - I'm wondering about the efficiency of a few places
> where you QI into a nsCOMPtr and then return it as a raw pointer.  If you QI'd
> it into a raw pointer and returned it as |already_AddRefed|, you'd save an
> addref and release if the caller is putting the value into a nsCOMPtr.	On the
> other hand, it may increase code size a bit since you'll be spreading out
> Release() calls to call sites that don't need to now.  Just something to
> consider; do what you think is best.

Yeah, the bigger point of this patch was to reduce code size, performance isn't
much of an issue in most of this code. So I'd rather not spread out code for
nsCOMPtr<> dtor calls.

I fixed the rest.

Thanks for the reviews!
Status: NEW → ASSIGNED

Comment 6

13 years ago
This bug has been fixed/checked in right? I only ask because my builds are now
broken. I haven't been about to produce any Firebird/Firefox (trunk) builds
since the 9th. I get the following error:

nsSecureBrowserUIImpl.cpp: In member function `virtual nsresult 
   nsSecureBrowserUIImpl::Init(nsIDOMWindow*)':
nsSecureBrowserUIImpl.cpp:196: error: no matching function for call to `
   nsDerivedSafe<nsIScriptGlobalObject>::GetDocShell()'
../../../../dist/include/dom/nsIScriptGlobalObject.h:73: error: candidates are: 
   virtual nsresult nsIScriptGlobalObject::GetDocShell(nsIDocShell**)
make[5]: *** [nsSecureBrowserUIImpl.o] Error 1


Looks, to me this check-in broke my build settings... no other builds seem to
exibit the behavior http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox
(Assignee)

Comment 7

13 years ago
(In reply to comment #6)
...
> nsSecureBrowserUIImpl.cpp:196: error: no matching function for call to `
>    nsDerivedSafe<nsIScriptGlobalObject>::GetDocShell()'
> ../../../../dist/include/dom/nsIScriptGlobalObject.h:73: error: candidates are: 
>    virtual nsresult nsIScriptGlobalObject::GetDocShell(nsIDocShell**)
> make[5]: *** [nsSecureBrowserUIImpl.o] Error 1

Looks like your nsIScriptGlobalObject.h file is not up to date, update in
mozilla/dom and make sure you're getting the latest version (3.22 or later).

> Looks, to me this check-in broke my build settings... no other builds seem to
> exibit the behavior http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox

Looks to me like your system either has old files laying around, or that your
tree is not up to date.

Email me (jst@mozilla.org) if you've got more questions...
Is there more needing to be done here or should this be marked fixed?
QA Contact: ian → general

Updated

7 years ago
Blocks: 105431
Should this bug still be open?
I say no.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.