[FIXr] more typesafety and such with arrays in nsScriptLoader

RESOLVED FIXED in mozilla1.5beta

Status

()

Core
DOM: Core & HTML
P2
normal
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.5beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Fixes a leak in the destructor if mPendingRequests is nonempty too.
Attachment #127425 - Flags: superreview?(jst)
Attachment #127425 - Flags: review?(caillon)
Comment on attachment 127425 [details] [diff] [review]
patch

- In nsScriptLoader::FireErrorNotification() and others:

   for (i = 0; i < count; i++) {
-    nsCOMPtr<nsISupports> sup(dont_AddRef(mObservers.ElementAt(i)));
-    nsCOMPtr<nsIScriptLoaderObserver> observer(do_QueryInterface(sup));
+    nsIScriptLoaderObserver* observer = mObservers[i];

     if (observer) {
       observer->ScriptAvailable(...);
     }
   }

I'd feel more comfortable with this if |observer| remained a strong references,
written like this, we're likely to crash if an observer removes itself as an
observer from one of these callbacks due to the lack of a strong refence that's
guaranteed to last the length of the observer call.

sr=jst
Attachment #127425 - Flags: superreview?(jst) → superreview+
Comment on attachment 127425 [details] [diff] [review]
patch

> nsScriptLoader::~nsScriptLoader()
> {
>   mObservers.Clear();
>   
>-  PRUint32 i, count;
>-  mPendingRequests.Count(&count);
>+  PRInt32 i, count;
>+  count = mPendingRequests.Count();

Could you maybe assign into the declaration for both of these PRInt32s?  |i|
could be declared in the for() loop...

>   for (i = 0; i < count; i++) {


- In nsScriptLoader::ProcessScriptElement()

>     // If we've got existing pending requests, add ourselves
>     // to this list.
>-    if (pendingRequestCount) {
>+    if (mPendingRequests.Count()) {

Count() returns a PRInt32; should this check Count() > 0?


Same two comments apply in FireErrorNotification(), FireScriptAvailable(),
FireScriptEvaluated(), and ProcessPendingReqests() twice.  (I notice that you
_did_ use Count() > 1 for your assert in OnStreamComplete()).

r=caillon with nits addressed.
Attachment #127425 - Flags: review?(caillon) → review+
Comment on attachment 127425 [details] [diff] [review]
patch

> nsScriptLoader::~nsScriptLoader()
> {
>   mObservers.Clear();
>   
>-  PRUint32 i, count;
>-  mPendingRequests.Count(&count);
>+  PRInt32 i, count;
>+  count = mPendingRequests.Count();

Could you maybe assign into the declaration for both of these PRInt32s?  |i|
could be declared in the for() loop...

>   for (i = 0; i < count; i++) {


- In nsScriptLoader::ProcessScriptElement()

>     // If we've got existing pending requests, add ourselves
>     // to this list.
>-    if (pendingRequestCount) {
>+    if (mPendingRequests.Count()) {

Count() returns a PRInt32; should this check Count() > 0?


Same two comments apply in FireErrorNotification(), FireScriptAvailable(),
FireScriptEvaluated(), and ProcessPendingReqests() twice.  (I notice that you
_did_ use Count() > 0 for your assert in OnStreamComplete()).

r=caillon with nits addressed.
Created attachment 127493 [details] [diff] [review]
updated to comments
Attachment #127425 - Attachment is obsolete: true
Priority: -- → P2
Summary: more typesafety and such with arrays in nsScriptLoader → [FIXr] more typesafety and such with arrays in nsScriptLoader
Target Milestone: --- → mozilla1.5beta
Fixed for 1.4b.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
*** Bug 96141 has been marked as a duplicate of this bug. ***

Updated

10 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: desale → general
You need to log in before you can comment on or make changes to this bug.