Closed
Bug 997963
Opened 10 years ago
Closed 10 years ago
Stop using PLArenaPool in nsStringBundleService
Categories
(Core :: Internationalization, defect)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
Attachments
(9 files)
18.46 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
4.01 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.39 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
3.61 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.55 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.23 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
4.20 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
882 bytes,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.22 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Ehsan, does this sound like a reasonable thing to you?
Flags: needinfo?(ehsan)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → continuation
Assignee | ||
Comment 2•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5684ab4afbc3
Assignee | ||
Comment 3•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5684ab4afbc3
Attachment #8409435 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8409436 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8409437 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•10 years ago
|
||
Also remove the now useless recycleEntry.
Attachment #8409438 -
Flags: review?(ehsan)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8409439 -
Flags: review?(ehsan)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8409440 -
Flags: review?(ehsan)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8409441 -
Flags: review?(ehsan)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8409442 -
Flags: review?(ehsan)
Updated•10 years ago
|
Attachment #8409435 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8409436 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8409437 -
Flags: review?(ehsan) → review+
Comment 11•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8409439 -
Flags: review?(ehsan) → review+
Comment 12•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8409441 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8409442 -
Flags: review?(ehsan) → review+
Comment 13•10 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #1) > Ehsan, does this sound like a reasonable thing to you? Yes. :-)
Flags: needinfo?(ehsan)
Assignee | ||
Comment 14•10 years ago
|
||
> Please make insertIntoCache accept an already_AddRef'ed and .forget() the bundle here to make it clear that this is transferring the ownership. Good suggestion. Done. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a61517ca13ac remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5f5047b9585 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/29173518f045 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7694e60758e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/efb8d7698089 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/34473bcef1b7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e70e9419acc2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9acbab76a7b0
Keywords: leave-open
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8409710 -
Flags: review?(ehsan)
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a61517ca13ac https://hg.mozilla.org/mozilla-central/rev/d5f5047b9585 https://hg.mozilla.org/mozilla-central/rev/29173518f045 https://hg.mozilla.org/mozilla-central/rev/b7694e60758e https://hg.mozilla.org/mozilla-central/rev/efb8d7698089 https://hg.mozilla.org/mozilla-central/rev/34473bcef1b7 https://hg.mozilla.org/mozilla-central/rev/e70e9419acc2 https://hg.mozilla.org/mozilla-central/rev/9acbab76a7b0
Comment 17•10 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=4bb905a25e39 https://hg.mozilla.org/integration/mozilla-inbound/rev/36879243d685
Keywords: leave-open
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/36879243d685
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•