Closed Bug 724452 Opened 9 years ago Closed 9 years ago

use nsFocusManager::GetFocusManager() more

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: tbsaunde, Assigned: jhk)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 2 obsolete files)

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();
Assignee: nobody → jigneshhk1992
Attached patch Patch (obsolete) — Splinter Review
Attachment #595671 - Flags: review?(trev.saunders)
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.
Attachment #595671 - Flags: review?(trev.saunders) → review+
(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.
Attached patch Patch modified for a11y. (obsolete) — Splinter Review
Attachment #595671 - Attachment is obsolete: true
Attachment #595997 - Flags: review?(trev.saunders)
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)
Attachment #595997 - Flags: review?(trev.saunders) → review?(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
Attachment #595997 - Flags: review?(hub) → review+
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
Attachment #595997 - Flags: review+
Attachment #595997 - Attachment is obsolete: true
Comment on attachment 596066 [details] [diff] [review]
Removed nsAccessNode::GetCurrentFocus

Good catch from Alex. Looks good this way too.
Attachment #596066 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e47e25d0a6c1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.