Closed
Bug 543961
Opened 13 years ago
Closed 13 years ago
nsAccessibilityService::GetAccessible() shouldn't try to get document accessible from global cache twice
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 1 obsolete file)
22.65 KB,
patch
|
MarcoZ
:
review+
davidb
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•13 years ago
|
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
Comment 1•13 years ago
|
||
(In reply to comment #0) > Ideally I would like GetCachedAccessNode() should return an accessible if it > was cached somewhere. A util method?
Assignee | ||
Comment 2•13 years ago
|
||
(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.
Comment 3•13 years ago
|
||
(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?
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to comment #3) > OK that's fine. Why isn't the method static now? It can be I think.
Assignee | ||
Comment 5•13 years ago
|
||
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)
Assignee | ||
Comment 6•13 years ago
|
||
(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.
Assignee | ||
Updated•13 years ago
|
Attachment #425011 -
Attachment is obsolete: true
Attachment #425011 -
Flags: review?(marco.zehe)
Attachment #425011 -
Flags: review?(bolterbugz)
Assignee | ||
Comment 7•13 years ago
|
||
Comment on attachment 425011 [details] [diff] [review] patch something wrong with my patch
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #426217 -
Flags: review?(marco.zehe)
Attachment #426217 -
Flags: review?(bolterbugz)
Comment 9•13 years ago
|
||
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+
Assignee | ||
Comment 10•13 years ago
|
||
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 11•13 years ago
|
||
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+
Comment 12•13 years ago
|
||
(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!
Assignee | ||
Comment 13•13 years ago
|
||
Great. Landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/72b38a63ccc6
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•