Closed
Bug 545467
Opened 13 years ago
Closed 12 years ago
nsHTMLImageAccessible shouldn't allocate hash in dynamic memory
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file)
27.26 KB,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
nsHTMLImageAccessible allocates the hash used to to cache area accessibles in dynamic memory and release it in Shutdown(). It's more safely to create the hash on stack like nsDocAccessible does.
Comment 2•12 years ago
|
||
Actually I probably misunderstood the bug summary. Alex and you explain this bug?
Assignee | ||
Comment 3•12 years ago
|
||
We shouldn't do mAccessNodeCache = new nsAccessNodeCache(). Just compare implementation of nsXULTreeAccessible and nsHTMLImageAccessible. However the current approach might save some memory. If so we could split up this class into two ones.
Comment 4•12 years ago
|
||
OK. Note we don't create nsXULTreeAccessible on the stack of course... so it's members are not on the stack either. I'm not sure if there is memory impact difference before calling .Init(k).
Assignee | ||
Comment 5•12 years ago
|
||
I'm not sure we talk about the same thing. I meant it would be nice if mAccessNodeCache wasn't a raw pointer. At least we don't need to null check every time (http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHTMLImageAccessible.h#97). And it's more consistent with other code.
Comment 6•12 years ago
|
||
OK great.
Assignee | ||
Comment 7•12 years ago
|
||
Assignee: bolterbugz → surkov.alexander
Status: NEW → ASSIGNED
Attachment #436661 -
Flags: review?(bolterbugz)
Updated•12 years ago
|
Attachment #436661 -
Flags: review?(bolterbugz) → review+
Comment 8•12 years ago
|
||
Comment on attachment 436661 [details] [diff] [review] patch r=me if try server is happy :) >+ if (mapElm) >+ *aAccessible = new nsHTMLImageMapAccessible(node, weakShell, mapElm); >+ else >+ *aAccessible = new nsHTMLImageAccessibleWrap(node, weakShell); Glad to see this! (Was thinking of doing the same in a follow up) >+//////////////////////////////////////////////////////////////////////////////// >+// nsHTMLImageMapAccessible >+//////////////////////////////////////////////////////////////////////////////// >+ >+const PRUint32 kDefaultImageCacheSize = 256; Let's rename this to "kDefaultImageMapCacheSize" so that readers never get confused with image lib cache (from gfx).
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to comment #8) > (From update of attachment 436661 [details] [diff] [review]) > r=me if try server is happy :) it is - http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-93cbafbdfd05 > >+const PRUint32 kDefaultImageCacheSize = 256; > > Let's rename this to "kDefaultImageMapCacheSize" so that readers never get > confused with image lib cache (from gfx). done
Assignee | ||
Comment 10•12 years ago
|
||
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/ac526bc2af65
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•