545391 - keep nsAccessNode objects in cache instead of nsIAccessNode

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

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

Comment on attachment 426234 [details] [diff] [review]
patch

r=me. I worry about leaks but it looks okay to me. Maybe worth Neil's eyes too.

>+PLDHashOperator
>+nsAccessNode::ClearCacheEntry(const void* aKey,
>+                              nsCOMPtr<nsAccessNode>& aAccessNode,
>+                              void* aUserArg)

While you are here can you put a comment before the method like
// callback used when clearing the cache.

>-    // Static methods for handling per-document cache
>-    static void PutCacheEntry(nsAccessNodeHashtable& aCache,
>-                              void* aUniqueID, nsIAccessNode *aAccessNode);
>-    static void GetCacheEntry(nsAccessNodeHashtable& aCache,
>-                              void* aUniqueID, nsIAccessNode **aAccessNode);
>-    static void ClearCache(nsAccessNodeHashtable& aCache);
>+  // Static methods for handling cache.

It is still per document right?

> static PLDHashOperator
>-ElementTraverser(const void *aKey, nsIAccessNode *aAccessNode,
>-                 void *aUserArg)
>+ElementTraverser(const void *aKey, nsAccessNode *aAccessNode, void *aUserArg)

Please add a short comment that this is a callback.

>-nsDocAccessible::CacheAccessNode(void *aUniqueID, nsIAccessNode *aAccessNode)
>+nsDocAccessible::CacheAccessNode(void *aUniqueID, nsAccessNode *aAccessNode)
> {
>   // If there is an access node for the given unique ID then let's shutdown it.
>   // The unique ID may be presented in the cache if originally we created
>   // access node object and then we want to create accessible object when
>   // DOM node is changed.

Might as well fix this comment while we're here.
"If there is already an access node with the given unique ID, shut it down because the DOM node has changed"

Does that capture the idea?

> static PLDHashOperator
>-ElementTraverser(const void *aKey, nsIAccessNode *aAccessNode,
>-                 void *aUserArg)
>+ElementTraverser(const void *aKey, nsAccessNode *aAccessNode, void *aUserArg)

// callback

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> (From update of attachment 426234 [details] [diff] [review])
> r=me. I worry about leaks but it looks okay to me. Maybe worth Neil's eyes too.

Ok.

> While you are here can you put a comment before the method like
> // callback used when clearing the cache.

ok

> 
> >-    // Static methods for handling per-document cache
> >-    static void PutCacheEntry(nsAccessNodeHashtable& aCache,
> >-                              void* aUniqueID, nsIAccessNode *aAccessNode);
> >-    static void GetCacheEntry(nsAccessNodeHashtable& aCache,
> >-                              void* aUniqueID, nsIAccessNode **aAccessNode);
> >-    static void ClearCache(nsAccessNodeHashtable& aCache);
> >+  // Static methods for handling cache.
> 
> It is still per document right?

not exactly, it's for any cache.

> >   // If there is an access node for the given unique ID then let's shutdown it.
> >   // The unique ID may be presented in the cache if originally we created
> >   // access node object and then we want to create accessible object when
> >   // DOM node is changed.
> 
> Might as well fix this comment while we're here.
> "If there is already an access node with the given unique ID, shut it down
> because the DOM node has changed"
> 
> Does that capture the idea?

fine with me

Comment 3

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

(In reply to comment #2)

> > >+  // Static methods for handling cache.
> > 
> > It is still per document right?
> 
> not exactly, it's for any cache.

Ah! Right. Thank you.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 5

[alexander :surkov (:asurkov)]

try server build - http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry

Comment 6

[[e-mail]]

Comment on attachment 426305 [details] [diff] [review]
patch2

>+  nsAccessNode* accessNode =
>+    gGlobalDocAccessibleCache.GetWeak(static_cast<void*>(aDocument));
>+  if (!accessNode)
>+    return nsnull;
>+
>   nsIAccessibleDocument *docAccessible = nsnull;
>-  nsCOMPtr<nsIAccessNode> accessNode;
>-  gGlobalDocAccessibleCache.Get(static_cast<void*>(aDocument),
>-                                getter_AddRefs(accessNode));
>-  if (accessNode) {
>-    CallQueryInterface(accessNode, &docAccessible);
>-  }
>+  CallQueryInterface(accessNode, &docAccessible);
>   return docAccessible;
I think this could be simplified something like this:
nsCOMPtr<nsIAccessibleDocument> docAccessible(do_QueryInterface(
  gGlobalDocAccessibleCache.GetWeak(static_cast<void*>(aDocument))));
return docAccessible.forget();

>+  /**
>+   * Clear the cache and shutdown the access nodes.
>+   */
>+  static void ClearCache(nsAccessNodeHashtable& aCache);
> 
>     // Static cache methods for global document cache
Indentation mismatch? (the removed block was too long to quote.)

>-  nsCOMPtr<nsIAccessNode> accessNode = GetCachedAccessNode(aStartNode);
>+  nsCOMPtr<nsAccessNode> accessNode = GetCachedAccessNode(aStartNode);
[Why not nsAccessNode* ?]

>-    accessNode = new nsHTMLAreaAccessible(domNode, this, mWeakShell);
>+     accessNode = new nsHTMLAreaAccessible(domNode, this, mWeakShell);
Random reindentation error?

>-  CallQueryInterface(accessNode, aAccessible);
>+  CallQueryInterface(accessNode.get(), aAccessible);
File a bug on using CallQueryInterface with an nsRefPtr?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> I think this could be simplified something like this:
> nsCOMPtr<nsIAccessibleDocument> docAccessible(do_QueryInterface(
>   gGlobalDocAccessibleCache.GetWeak(static_cast<void*>(aDocument))));
> return docAccessible.forget();

fine with me

> >+  /**
> >+   * Clear the cache and shutdown the access nodes.
> >+   */
> >+  static void ClearCache(nsAccessNodeHashtable& aCache);

> Indentation mismatch? (the removed block was too long to quote.)

Part of the file keeps two spaces, part - four spaces. While I'm around I'm making two spaces.

> 
> >-  nsCOMPtr<nsIAccessNode> accessNode = GetCachedAccessNode(aStartNode);
> >+  nsCOMPtr<nsAccessNode> accessNode = GetCachedAccessNode(aStartNode);
> [Why not nsAccessNode* ?]

This code is related with the cache refreshing so I secure myself by addref.

> >-    accessNode = new nsHTMLAreaAccessible(domNode, this, mWeakShell);
> >+     accessNode = new nsHTMLAreaAccessible(domNode, this, mWeakShell);
> Random reindentation error?

yes, thanks, fixed.

> 
> >-  CallQueryInterface(accessNode, aAccessible);
> >+  CallQueryInterface(accessNode.get(), aAccessible);
> File a bug on using CallQueryInterface with an nsRefPtr?

I'll file a bug. I think it's worth.

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 426305 [details] [diff] [review]
patch2

I tested the try server build that contains this patch and found it to work fine with JAWS and NVDA. Even feels snappier a bit!

Comment 9

[alexander :surkov (:asurkov)]

Great!

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