Closed Bug 545391 Opened 10 years ago Closed 10 years ago

keep nsAccessNode objects in cache instead of nsIAccessNode

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, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #426234 - Flags: review?(bolterbugz)
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
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
Attachment #426234 - Flags: review?(bolterbugz) → review+
(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
(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.
Attached patch patch2Splinter Review
Attachment #426234 - Attachment is obsolete: true
Attachment #426305 - Flags: superreview?(neil)
Attachment #426305 - Flags: superreview?(neil) → superreview+
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?
(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 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!
Attachment #426305 - Flags: review+
Great!

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/484a3c2949c2
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.