Last Comment Bug 671991 - decomtaminate GetFocusedChild()
: decomtaminate GetFocusedChild()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: Trevor Saunders (:tbsaunde)
:
Mentors:
Depends on: 677467
Blocks: dexpcoma11y
  Show dependency treegraph
 
Reported: 2011-07-15 15:58 PDT by Trevor Saunders (:tbsaunde)
Modified: 2011-08-09 00:20 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10.38 KB, patch)
2011-07-15 23:02 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
patch v2 (10.28 KB, patch)
2011-07-22 14:18 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review
patch v2 (14.54 KB, patch)
2011-07-22 14:53 PDT, Trevor Saunders (:tbsaunde)
surkov.alexander: review+
Details | Diff | Review
patch (14.75 KB, patch)
2011-07-22 23:20 PDT, Trevor Saunders (:tbsaunde)
no flags Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2011-07-15 15:58:52 PDT

    
Comment 1 Trevor Saunders (:tbsaunde) 2011-07-15 23:02:32 PDT
Created attachment 546284 [details] [diff] [review]
patch
Comment 2 alexander :surkov 2011-07-16 09:51:18 PDT
Comment on attachment 546284 [details] [diff] [review]
patch

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

btw, please use new FocusedChild() on platform layer (linux, windows and mac)

::: accessible/src/base/nsAccessible.cpp
@@ +770,5 @@
> +nsAccessible*
> +nsAccessible::FocusedChild()
> +{ 
> +  if (!mContent)
> +    return nsnull;

why is that for?

@@ +775,5 @@
> +
> +  if (gLastFocusedNode == mContent)
> +    return this;
> +
> +    nsAccessible* focusedChild = GetAccService()->GetAccessible(gLastFocusedNode);

I think I prefer GetDocAccessible()->GetAccessible()

@@ +778,5 @@
> +
> +    nsAccessible* focusedChild = GetAccService()->GetAccessible(gLastFocusedNode);
> +
> +    // XXX this doesn't consider the focused child being a grand child of this
> +    // or focusedChild == this althought I'm not sure the second is actually possible

it appears AT happy with this, if they need a child from subtree then they call it on document accessible, otherwise doing this is perf problem. Having said that I think we should do exception for ARIA documents, please file bug on this

::: accessible/src/base/nsAccessible.h
@@ +127,5 @@
>  
>    /**
> +   * Return the focused child if any.
> +   */
> +  virtual nsAccessible* FocusedChild();

let's get an agreement how we group methods, I'd suggest to list properies on top (like role, name and etc) and then accessible getters (like focused, child from point)

::: accessible/src/base/nsDocAccessible.cpp
@@ +359,4 @@
>  
>    // Return an accessible for the current global focus, which does not have to
>    // be contained within the current document.
> +  return GetAccService()->GetAccessible(gLastFocusedNode);

maybe inline it? like

return gLastFocusedNode ? GetAcccessible(gLastFocusedNode) : nsnull;
Comment 3 Trevor Saunders (:tbsaunde) 2011-07-22 14:18:28 PDT
Created attachment 547802 [details] [diff] [review]
patch v2
Comment 4 :Ms2ger 2011-07-22 14:21:38 PDT
Comment on attachment 547802 [details] [diff] [review]
patch v2

>+nsAccessible*
>+nsAccessible::FocusedChild()
>+{ 
>+  if (gLastFocusedNode == mContent)
>+    return this;
>+
>+    nsAccessible* focusedChild = GetDocAccessible()->GetAccessible(gLastFocusedNode);
>+    if (!focusedChild || focusedChild->GetParent() != this)
>+      return nsnull;

Indentation is weird here.
Comment 5 Trevor Saunders (:tbsaunde) 2011-07-22 14:53:22 PDT
Created attachment 547811 [details] [diff] [review]
patch v2

thanks, didn't notice that.
Comment 6 alexander :surkov 2011-07-22 22:38:51 PDT
Comment on attachment 547811 [details] [diff] [review]
patch v2

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

::: accessible/src/base/nsAccessible.cpp
@@ +758,5 @@
>  NS_IMETHODIMP
> +nsAccessible::GetFocusedChild(nsIAccessible** aChild)
> +{
> +  NS_ENSURE_ARG_POINTER(aChild);
> +  *aChild = nsnull;

nit: new line please

@@ +772,5 @@
> +{ 
> +  if (gLastFocusedNode == mContent)
> +    return this;
> +
> +  nsAccessible* focusedChild = GetDocAccessible()->GetAccessible(gLastFocusedNode);

I'd prefer to have gLastFocusedNode null check before getting an accessible

::: accessible/src/base/nsDocAccessible.cpp
@@ +357,4 @@
>  
>    // Return an accessible for the current global focus, which does not have to
>    // be contained within the current document.
> +  return gLastFocusedNode ? GetAccessible(gLastFocusedNode) : nsnull;

I know I suggseted this but GetAccessible() contradicts with comment above, please get back to AccService() usage

::: accessible/src/xul/nsXULTreeGridAccessible.h
@@ +160,5 @@
>    virtual PRBool Init();
>    virtual bool IsPrimaryForNode() const;
>  
>    // nsAccessible
> +  virtual nsAccessible* FocusedChild();

please keep accessible getters after property getters
Comment 7 Trevor Saunders (:tbsaunde) 2011-07-22 23:20:44 PDT
Created attachment 547894 [details] [diff] [review]
patch
Comment 8 alexander :surkov 2011-07-31 20:17:46 PDT
Comment on attachment 547894 [details] [diff] [review]
patch

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

otherwise looks good

::: accessible/src/base/nsAccessible.cpp
@@ +773,5 @@
> +{ 
> +  if (gLastFocusedNode == mContent)
> +    return this;
> +  if (!gLastFocusedNode)
> +    return nsnull;

it's cleaner when you check !gLastFocusedNode prior to gLastFocusedNode == mContent check
Comment 9 Trevor Saunders (:tbsaunde) 2011-07-31 20:29:01 PDT
landed http://hg.mozilla.org/integration/mozilla-inbound/rev/2d6026252d03
Comment 10 Marco Bonardo [::mak] 2011-08-01 08:07:08 PDT
http://hg.mozilla.org/mozilla-central/rev/2d6026252d03

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