Closed Bug 543961 Opened 12 years ago Closed 12 years ago

nsAccessibilityService::GetAccessible() shouldn't try to get document accessible from global cache twice

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

nsAccessibilityService::GetAccessible() shouldn't try to get document accessible from global cache if it's unable to get it from GetCachedAccessNode because the document accessible cache itself in own cache.

It's not good the document is cached twice in the document cache and in global cache. So we might want to fix this.

Ideally I would like GetCachedAccessNode() should return an accessible if it was cached somewhere.
Summary: nsAccessibilityService::GetAccessible() shouldn't try to get document accessible from global cache → nsAccessibilityService::GetAccessible() shouldn't try to get document accessible from global cache twice
(In reply to comment #0)
> Ideally I would like GetCachedAccessNode() should return an accessible if it
> was cached somewhere.

A util method?
(In reply to comment #1)
> (In reply to comment #0)
> > Ideally I would like GetCachedAccessNode() should return an accessible if it
> > was cached somewhere.
> 
> A util method?

It's kind of util aready :) I didn't get your question.
(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > Ideally I would like GetCachedAccessNode() should return an accessible if it
> > > was cached somewhere.
> > 
> > A util method?
> 
> It's kind of util aready :) I didn't get your question.

OK that's fine. Why isn't the method static now?
(In reply to comment #3)

> OK that's fine. Why isn't the method static now?

It can be I think.
Attached patch patch (obsolete) — Splinter Review
I think it doesn't make sense to keep the doc accessible in two caches. Removing it from the document cache.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #425011 - Flags: review?(marco.zehe)
Attachment #425011 - Flags: review?(bolterbugz)
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > (In reply to comment #0)
> > > > Ideally I would like GetCachedAccessNode() should return an accessible if it
> > > > was cached somewhere.
> > > 
> > > A util method?
> > 
> > It's kind of util aready :) I didn't get your question.
> 
> OK that's fine. Why isn't the method static now?

I think it's not necessary since we guarantee only one instance of nsAccessibilityService exists.
Attachment #425011 - Attachment is obsolete: true
Attachment #425011 - Flags: review?(marco.zehe)
Attachment #425011 - Flags: review?(bolterbugz)
Comment on attachment 425011 [details] [diff] [review]
patch

something wrong with my patch
Attached patch patch2Splinter Review
Attachment #426217 - Flags: review?(marco.zehe)
Attachment #426217 - Flags: review?(bolterbugz)
Comment on attachment 426217 [details] [diff] [review]
patch2

r=me thanks.

>-  // Check to see if we already have an accessible for this
>-  // node in the cache
>-  nsCOMPtr<nsIAccessNode> accessNode;
>-  GetCachedAccessNode(aNode, aWeakShell, getter_AddRefs(accessNode));
>+  // Check to see if we already have an accessible for this node in the cache.
>+  nsCOMPtr<nsIAccessNode> cachedAccessNode =
>+    GetCachedAccessNode(aNode, aWeakShell);
>+  if (cachedAccessNode) {
>+    // XXX: the cache might contain the access node for the DOM node but
>+    // accessible because of wrong cache update. In this case try to create new
>+    // accessible.

Please change "DOM node but" to "DOM node that is not"

>+already_AddRefed<nsIAccessNode>
>+nsDocAccessible::GetCachedAccessNode(void *aUniqueID)
> {
>-  GetCacheEntry(mAccessNodeCache, aUniqueID, aAccessNode); // Addrefs for us
>+  nsIAccessNode* accessNode = nsnull;
>+  GetCacheEntry(mAccessNodeCache, aUniqueID, &accessNode);
>+
>+  // No accessible in the cache, check if the given ID is unique ID of this
>+  // document accesible.
>+  if (!accessNode) {
>+    void* thisUniqueID = nsnull;
>+    GetUniqueID(&thisUniqueID);
>+    if (thisUniqueID == aUniqueID) {
>+      NS_ADDREF(this);
>+      return this;
>+    }
>+  }

Hrmmm ok... should be quick enough. Cool.

> #define NS_DOCACCESSIBLE_IMPL_CID                       \
>-{  /* 5641921c-a093-4292-9dca-0b51813db57d */           \
>-  0x5641921c,                                           \
>-  0xa093,                                               \
>-  0x4292,                                               \
>-  { 0x9d, 0xca, 0x0b, 0x51, 0x81, 0x3d, 0xb5, 0x7d }    \
>+{  /* 6baed0f5-3285-4a84-88b0-004fddad7f03 */           \
>+  0x6baed0f5,                                           \
>+  0x3285,                                               \
>+  0x4a84,                                               \
>+  { 0x88, 0xb0, 0x00, 0x4f, 0xdd, 0xad, 0x7f, 0x03 }    \
> }

Not sure this is necessary but probably doesn't hurt anything.
Attachment #426217 - Flags: review?(bolterbugz) → review+
try-server build - http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-d04ebbeae973

I'm not sure why but this patch makes mochitests faster into 2 times. The trunk build takes 15 minutes on my Mac, the patched build takes 7 minutes.
Comment on attachment 426217 [details] [diff] [review]
patch2

r+ for functionality. I tried several constellations of documents in different tabs and windows, and they all work as expected still with JAWS and NVDA.
Attachment #426217 - Flags: review?(marco.zehe) → review+
(In reply to comment #10)
> try-server build -
> http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-d04ebbeae973
> 
> I'm not sure why but this patch makes mochitests faster into 2 times. The trunk
> build takes 15 minutes on my Mac, the patched build takes 7 minutes.

I expected a performance win, but that seems quite substantial -- although our cache is a hot path.

Great!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 560939
You need to log in before you can comment on or make changes to this bug.