Last Comment Bug 724452 - use nsFocusManager::GetFocusManager() more
: use nsFocusManager::GetFocusManager() more
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jignesh Kakadiya [:jhk]
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-02-05 20:01 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-02-13 08:42 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (18.03 KB, patch)
2012-02-08 23:42 PST, Jignesh Kakadiya [:jhk]
tbsaunde+mozbugs: review+
Details | Diff | Review
Patch modified for a11y. (3.36 KB, patch)
2012-02-10 02:16 PST, Jignesh Kakadiya [:jhk]
hub: review+
surkov.alexander: review+
Details | Diff | Review
Removed nsAccessNode::GetCurrentFocus (4.85 KB, patch)
2012-02-10 09:18 PST, Jignesh Kakadiya [:jhk]
hub: review+
Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-02-05 20:01:15 PST
look for cases of nsCOMPtr<nsIFocusManager> fm = do_GetService(NS_FOCUSMANAGER_CONTRACTID) using grep or http://mxr.mozilla.org/mozilla-central/ and replace them with
nsFocusManager* fm = nsFocusManager::GetFocusManager();
Comment 1 Jignesh Kakadiya [:jhk] 2012-02-08 23:42:04 PST
Created attachment 595671 [details] [diff] [review]
Patch
Comment 2 Trevor Saunders (:tbsaunde) 2012-02-09 22:43:27 PST
Comment on attachment 595671 [details] [diff] [review]
Patch

r=me for accessible/ part, but I'm not a peer for the rest of these things, so you need to find someone else to review that or only do the a11y part.
Comment 3 alexander :surkov 2012-02-09 22:53:46 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 595671 [details] [diff] [review]
> Patch
> 
> r=me for accessible/ part, but I'm not a peer for the rest of these things,
> so you need to find someone else to review that or only do the a11y part.

I suspect everything outside of accessible part might be wrong because nsFocusManager::GetFocusManager() doesn't make sure the nsFocusManager instance is created so I assume one of do_GetService calls can do this. We are safe to assume on accessibility side that GetFocusManager is not null because DOM focus happens prior we can touch focus stuff in a11y.
Comment 4 Jignesh Kakadiya [:jhk] 2012-02-10 02:16:56 PST
Created attachment 595997 [details] [diff] [review]
Patch modified for a11y.
Comment 5 alexander :surkov 2012-02-10 04:17:46 PST
Comment on attachment 595997 [details] [diff] [review]
Patch modified for a11y.

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

moving review request to Hub (Trevor agreed to steal review request from him per irc)
Comment 6 Hubert Figuiere [:hub] 2012-02-10 08:59:01 PST
Comment on attachment 595997 [details] [diff] [review]
Patch modified for a11y.

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

r=me

Looks good for checkin
Comment 7 alexander :surkov 2012-02-10 09:07:55 PST
Comment on attachment 595997 [details] [diff] [review]
Patch modified for a11y.

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

r=me with comment fixed

::: accessible/src/base/nsAccessNode.cpp
@@ +423,5 @@
>    nsIDOMWindow* win = doc->GetWindow();
>  
>    nsCOMPtr<nsIDOMWindow> focusedWindow;
>    nsCOMPtr<nsIDOMElement> focusedElement;
> +  nsFocusManager* fm = nsFocusManager::GetFocusManager();

please remove nsAccessNode::GetCurrentFocus at all since it's not used
Comment 8 Jignesh Kakadiya [:jhk] 2012-02-10 09:18:52 PST
Created attachment 596066 [details] [diff] [review]
Removed nsAccessNode::GetCurrentFocus
Comment 9 Hubert Figuiere [:hub] 2012-02-10 12:58:01 PST
Comment on attachment 596066 [details] [diff] [review]
Removed nsAccessNode::GetCurrentFocus

Good catch from Alex. Looks good this way too.
Comment 10 alexander :surkov 2012-02-11 18:52:44 PST
inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/e47e25d0a6c1
Comment 11 Marco Bonardo [::mak] 2012-02-13 08:42:29 PST
https://hg.mozilla.org/mozilla-central/rev/e47e25d0a6c1

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