545467 - nsHTMLImageAccessible shouldn't allocate hash in dynamic memory

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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 1

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm poking around this code anyway. Taking.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Actually I probably misunderstood the bug summary.

Alex and you explain this bug?

Comment 3

[alexander :surkov (:asurkov)]

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

[David Bolter [:davidb] (NeedInfo me for attention)]

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).

Comment 5

[alexander :surkov (:asurkov)]

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

[David Bolter [:davidb] (NeedInfo me for attention)]

OK great.

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

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).

Comment 9

[alexander :surkov (:asurkov)]

(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/[e-mail]-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

Comment 10

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/ac526bc2af65