724452 - use nsFocusManager::GetFocusManager() more

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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]]

Attachment: Patch

Comment 2

[Trevor Saunders (:tbsaunde)]

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 (:asurkov)]

(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]]

Attachment: Patch modified for a11y.

Comment 5

[alexander :surkov (:asurkov)]

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]]

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 (:asurkov)]

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]]

Attachment: Removed nsAccessNode::GetCurrentFocus

Comment 9

[Hubert Figuiere [:hub]]

Comment on attachment 596066 [details] [diff] [review]
Removed nsAccessNode::GetCurrentFocus

Good catch from Alex. Looks good this way too.

Comment 10

[alexander :surkov (:asurkov)]

inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/e47e25d0a6c1

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e47e25d0a6c1