Closed
Bug 378327
Opened 17 years ago
Closed 17 years ago
minor cleanup of ForwardReferences code in nsXULDocument
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha5
People
(Reporter: asqueella, Assigned: asqueella)
Details
Attachments
(1 file)
11.30 KB,
patch
|
smaug
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
- move AddForwardReference and ResolveForwardReferences from nsIXULDocument to nsXULDocument, make them non-virtual. - make mForwardReferences an nsTArray<nsAutoPtr<nsForwardReference> > instead of nsAutoVoidArray.
Attachment #262378 -
Flags: superreview?(bzbarsky)
Attachment #262378 -
Flags: review?(Olli.Pettay)
Comment 1•17 years ago
|
||
Nickolay, I'm not sure I'll be able to get to this in an expeditious manner. You might want to ask another sr....
Comment 2•17 years ago
|
||
Comment on attachment 262378 [details] [diff] [review] patch >+nsresult > nsXULDocument::AddForwardReference(nsForwardReference* aRef) > { > if (mResolutionPhase < aRef->GetPhase()) { >- mForwardReferences.AppendElement(aRef); >+ if (!mForwardReferences.AppendElement(aRef)) { >+ delete aRef; >+ return NS_ERROR_OUT_OF_MEMORY; >+ } I'm not quite sure what happens if AppendElement fails. Is there a nsAutoPtr<nsForwardReference> object already created, and that has taken the ownership. In that case |delete aRef| would delete already deleted object.
Assignee | ||
Comment 3•17 years ago
|
||
My reading of http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsTArray.h#501 was that the allocation (which can fail) happens first. And the nsAutoPtrs are created only after allocation succeeds. I.e. "class Item" there is nsForwardReference, not nsAutoPtr<nsForwardReference> or something...
Assignee | ||
Updated•17 years ago
|
Attachment #262378 -
Flags: superreview?(bzbarsky) → superreview?(roc)
Comment 4•17 years ago
|
||
Comment on attachment 262378 [details] [diff] [review] patch OK, I think OOM handling is ok. It just took some time to read nsTArray. > const nsForwardReference::Phase* pass = nsForwardReference::kPasses; > while ((mResolutionPhase = *pass) != nsForwardReference::eDone) { > PRInt32 previous = 0; >- while (mForwardReferences.Count() && mForwardReferences.Count() != previous) { >- previous = mForwardReferences.Count(); >+ while (mForwardReferences.Length() && >+ mForwardReferences.Length() != previous) { >+ previous = mForwardReferences.Length(); > >- for (PRInt32 i = 0; i < mForwardReferences.Count(); ++i) { >- nsForwardReference* fwdref = NS_REINTERPRET_CAST(nsForwardReference*, mForwardReferences[i]); >+ for (PRInt32 i = 0; i < mForwardReferences.Length(); ++i) { >+ nsForwardReference* fwdref = mForwardReferences[i]; nsTArray<..>.Length() returns PRUint32, not PRInt32 like nsVoidArray.Count()
Attachment #262378 -
Flags: review?(Olli.Pettay) → review+
Attachment #262378 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 5•17 years ago
|
||
Checked in with the PRUint32 fix, good catch! mozilla/content/xul/document/public/nsIXULDocument.h 1.40 mozilla/content/xul/document/src/nsXULDocument.cpp 1.761 mozilla/content/xul/document/src/nsXULDocument.h 1.194
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Target Milestone: --- → mozilla1.9alpha5
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•