Closed
Bug 233307
Opened 20 years ago
Closed 13 years ago
deCOMtaminate nsIScript*
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: jst, Assigned: jst)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
377.15 KB,
patch
|
Details | Diff | Splinter Review | |
355.56 KB,
patch
|
bryner
:
review+
bryner
:
superreview+
|
Details | Diff | Splinter Review |
Lots of things that could be cleaned up in those interfaces... patch coming up.
Assignee | ||
Comment 1•20 years ago
|
||
Assignee | ||
Comment 2•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #140763 -
Flags: superreview?(peterv)
Attachment #140763 -
Flags: review?(peterv)
Assignee | ||
Comment 3•20 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•20 years ago
|
Attachment #140763 -
Flags: superreview?(peterv)
Attachment #140763 -
Flags: superreview?(bryner)
Attachment #140763 -
Flags: review?(peterv)
Attachment #140763 -
Flags: review?(bryner)
Comment 4•20 years ago
|
||
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•20 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
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•20 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...
Comment 8•19 years ago
|
||
Is there more needing to be done here or should this be marked fixed?
Updated•15 years ago
|
QA Contact: ian → general
Comment 9•14 years ago
|
||
Should this bug still be open?
Comment 10•13 years ago
|
||
I say no.
Status: ASSIGNED → RESOLVED
Closed: 13 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
•