Last Comment Bug 756381 - make FocusManager::FocusedDOMNode faster
: make FocusManager::FocusedDOMNode faster
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: 732872
  Show dependency treegraph
 
Reported: 2012-05-18 00:27 PDT by alexander :surkov
Modified: 2012-05-23 08:05 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.17 KB, patch)
2012-05-18 00:27 PDT, alexander :surkov
bugs: review+
Details | Diff | Splinter Review
patch2 (2.75 KB, patch)
2012-05-21 07:46 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-05-18 00:27:34 PDT
Created attachment 625022 [details] [diff] [review]
patch

It takes up to 3% in example of bug 732872 because these nsCOMPtr stuffs.
Comment 1 Olli Pettay [:smaug] 2012-05-18 02:04:09 PDT
Comment on attachment 625022 [details] [diff] [review]
patch

>diff --git a/accessible/src/base/FocusManager.cpp b/accessible/src/base/FocusManager.cpp
>--- a/accessible/src/base/FocusManager.cpp
>+++ b/accessible/src/base/FocusManager.cpp
>@@ -373,25 +373,18 @@ FocusManager::FocusedDOMNode() const
>   // keeps the focus.
>   if (focusedElm) {
>     if (nsEventStateManager::IsRemoteTarget(focusedElm))
>       return nsnull;
>     return focusedElm;
>   }
> 
>   // Otherwise the focus can be on DOM document.
>-  nsCOMPtr<nsIDOMWindow> focusedWnd;
>-  DOMFocusManager->GetFocusedWindow(getter_AddRefs(focusedWnd));
>-  if (focusedWnd) {
>-    nsCOMPtr<nsIDOMDocument> DOMDoc;
>-    focusedWnd->GetDocument(getter_AddRefs(DOMDoc));
>-    nsCOMPtr<nsIDocument> DOMDocNode(do_QueryInterface(DOMDoc));
>-    return DOMDocNode;
>-  }
>-  return nsnull;
>+  nsPIDOMWindow* focusedWnd = DOMFocusManager->GetFocusedWindow();
>+  return focusedWnd ? focusedWnd->GetExtantDocumentNode() : nsnull;
> }
Hmm, you're changing behavior here. GetDocument() isn't the same 
as GetExtant*. GetDocument() may create the document.
Is that ok for a11y?



>+  nsIDocument* GetExtantDocumentNode() const
Maybe GetExtantDoc()
Thank you for adding this. I've been thinking this for a long time.

r+ for the dom/ parts
Comment 2 Olli Pettay [:smaug] 2012-05-18 02:04:35 PDT
Update the iid of nsPIDOMWindow
Comment 3 alexander :surkov 2012-05-18 02:13:15 PDT
(In reply to Olli Pettay [:smaug] from comment #1)

> Hmm, you're changing behavior here. GetDocument() isn't the same 
> as GetExtant*. GetDocument() may create the document.
> Is that ok for a11y?

yes, no DOM document means no accessible what means this method shouldn't called.
Comment 4 alexander :surkov 2012-05-21 07:46:11 PDT
Created attachment 625644 [details] [diff] [review]
patch2
Comment 5 Trevor Saunders (:tbsaunde) 2012-05-21 13:48:45 PDT
Comment on attachment 625644 [details] [diff] [review]
patch2

sorry I was away so much... but I'm freer now, so figured I'd steel this back :)
Comment 6 alexander :surkov 2012-05-21 22:33:26 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> Comment on attachment 625644 [details] [diff] [review]
> patch2
> 
> sorry I was away so much... but I'm freer now, so figured I'd steel this
> back :)

cool, welcome back :)
Comment 8 David Bolter [:davidb] 2012-05-22 06:18:25 PDT
Comment on attachment 625644 [details] [diff] [review]
patch2

Review of attachment 625644 [details] [diff] [review]:
-----------------------------------------------------------------

cool
Comment 9 Ed Morley [:emorley] 2012-05-23 08:05:34 PDT
https://hg.mozilla.org/mozilla-central/rev/219dc3f47568

Note You need to log in before you can comment on or make changes to this bug.