Closed Bug 545467 Opened 10 years ago Closed 10 years ago

nsHTMLImageAccessible shouldn't allocate hash in dynamic memory

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

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.
I'm poking around this code anyway. Taking.
Assignee: nobody → bolterbugz
Actually I probably misunderstood the bug summary.

Alex and you explain this bug?
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.
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).
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.
Attached patch patchSplinter Review
Assignee: bolterbugz → surkov.alexander
Status: NEW → ASSIGNED
Attachment #436661 - Flags: review?(bolterbugz)
Attachment #436661 - Flags: review?(bolterbugz) → review+
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).
(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
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/ac526bc2af65
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.