597368 - nsScriptLoader should use nsRefPtr to hold nsScriptLoadRequest

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Henri Sivonen (:hsivonen)]

nsScriptLoader should use nsRefPtr to hold nsScriptLoadRequest. It's currently using nsCOMPtr and nsCOMArray.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Make nsScriptLoader use nsRefPtr/nsTArray for holding nsScriptLoadRequest

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 476220 [details] [diff] [review]
Make nsScriptLoader use nsRefPtr/nsTArray for holding nsScriptLoadRequest

>+  while (mEnabled && i < mAsyncRequests.Length()) {
>     if (!mAsyncRequests[i]->mLoading) {
>-      request = mAsyncRequests[i];
>-      mAsyncRequests.RemoveObjectAt(i);
>+      request.swap(mAsyncRequests[i]);
>+      mAsyncRequests.RemoveElementAt(i);
>       ProcessRequest(request);
>       continue;
>     }
>     ++i;
>   }

This is somewhat odd since you're actually swapping in the old requests into the array before you remove it. I think it works, but it would IMHO be more understandable to do

request = mAsyncRequests[i].forget();

Same in the loop below.

r=me with that fixed.

Thanks for doing this cleanup.

Comment 3

[Henri Sivonen (:hsivonen)]

Comment on attachment 476220 [details] [diff] [review]
Make nsScriptLoader use nsRefPtr/nsTArray for holding nsScriptLoadRequest

Requesting approval: This is low-risk cleanup. Taking this on the branch would make branch maintanance easier if we end up having to patch nsScriptLoader.

Comment 4

[Henri Sivonen (:hsivonen)]

http://hg.mozilla.org/mozilla-central/rev/aacd84d91b66