Closed Bug 997963 Opened 6 years ago Closed 6 years ago

Stop using PLArenaPool in nsStringBundleService

Categories

(Core :: Internationalization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(9 files)

It seems pretty absurd to use an arena pool here, for this thing that is only going to have 16 entries at a time, and probably doesn't have all that many things allocated.  We should instead just use new and delete, like a normal class.

It would also be nice to turn the nsIStringBundle* in bundleCacheEntry_t into an nsCOMPtr.
Ehsan, does this sound like a reasonable thing to you?
Flags: needinfo?(ehsan)
Assignee: nobody → continuation
Also remove the now useless recycleEntry.
Attachment #8409438 - Flags: review?(ehsan)
Blocks: 984125
Attachment #8409435 - Flags: review?(ehsan) → review+
Attachment #8409436 - Flags: review?(ehsan) → review+
Attachment #8409437 - Flags: review?(ehsan) → review+
Comment on attachment 8409438 [details] [diff] [review]
part 4 - Turn bundleCacheEntry_t::mBundle into an nsCOMPtr.

Review of attachment 8409438 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/strres/src/nsStringBundle.cpp
@@ -488,5 @@
>  #define MAX_CACHED_BUNDLES 16
>  
>  struct bundleCacheEntry_t : public LinkedListElement<bundleCacheEntry_t> {
>    nsAutoPtr<nsCStringKey> mHashKey;
> -  // do not use a nsCOMPtr - this is a struct not a class!

WTF?!
Attachment #8409438 - Flags: review?(ehsan) → review+
Attachment #8409439 - Flags: review?(ehsan) → review+
Comment on attachment 8409440 [details] [diff] [review]
part 6 - Use a refptr in nsStringBundleService::getStringBundle.

Review of attachment 8409440 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/strres/src/nsStringBundle.cpp
@@ +593,1 @@
>      cacheEntry = insertIntoCache(bundle, &completeKey);

Please make insertIntoCache accept an already_AddRef'ed and .forget() the bundle here to make it clear that this is transferring the ownership.
Attachment #8409440 - Flags: review?(ehsan) → review+
Attachment #8409441 - Flags: review?(ehsan) → review+
Attachment #8409442 - Flags: review?(ehsan) → review+
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Ehsan, does this sound like a reasonable thing to you?

Yes.  :-)
Flags: needinfo?(ehsan)
Comment on attachment 8409710 [details] [diff] [review]
part 9 - Make bundleCacheEntry_t FINAL, have COUNT_CTOR.

Review of attachment 8409710 [details] [diff] [review]:
-----------------------------------------------------------------

Sigh, please link to the hg commit in bug 991331 once you land this so that we can remove this once that bug is fixed.
Attachment #8409710 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/36879243d685
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.