381302 - move application accessible to cross platform code

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Aaron Leventhal]

I've just looked quickly so far. Here is a dumb question before I look more:

Why is there not an
accessible/src/base/nsAppRootAccessible.cpp (and .h)
accessible/src/msaa/nsAppRootAccessibleWrap.cpp (and .h)
accessible/src/atk/nsAppRootAccessibleWrap.cpp (and .h)

I would have expected the ATK file to be renamed (or maybe the class, if we want to have a small patch).

Would that not be more consistent with what we're doing?

Comment 2

[Aaron Leventhal]

Comment on attachment 265404 [details] [diff] [review]
patch

Clearing request until I get overview of the strategy for the patch.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> I've just looked quickly so far. Here is a dumb question before I look more:
> 
> Why is there not an
> accessible/src/base/nsAppRootAccessible.cpp (and .h)
> accessible/src/msaa/nsAppRootAccessibleWrap.cpp (and .h)
> accessible/src/atk/nsAppRootAccessibleWrap.cpp (and .h)

That should be ideally. I can't do it without copying file but file copying takes a long time (something about month). Therefore I named class as nsAccessibleApplication (it's analogue of IA2::IApplicationAccessible). So I made the following structure:

base/nsAccessibleApplication.cpp/h
msaa/nsAccessibleApplicationWrap.cpp/h
atk/nsAppRootAccessible.cpp/h (there nsAccessibleApplicationWrap class is defined).

Comment 4

[Aaron Leventhal]

So I guess we could file separate follow-up bug to rename nsAppRootAccessible.cpp -- the CVS admins can do that for us I think. We'd just need to change it in the Makefile as well.

Comment 5

[Aaron Leventhal]

Comment on attachment 265404 [details] [diff] [review]
patch

It must be wrong that tmpAccWrap is defined twice here:

    nsAccessibleWrap * tmpAccWrap = MAI_ATK_OBJECT(aAtkObj)->accWrap;
 
     // Check if AccessibleWrap was deconstructed
     if (tmpAccWrap == nsnull) {
         return NS_ERROR_NULL_POINTER;
     }
 
-    if (tmpAccWrap != nsAppRootAccessible::Create() && !tmpAccWrap->IsValidObject())
+    nsApplicationAccessibleWrap* tmpAppAcc = NS_REINTERPRET_CAST(nsApplicationAccessibleWrap*, tmpAccWrap);
+    nsApplicationAccessibleWrap* appAcc = nsAccessNode::GetApplicationAccessible();
+    if (tmpAppAcc != appAcc &&
+        !tmpAccWrap->IsValidObject())
         return NS_ERROR_INVALID_POINTER;

Comment 6

[Aaron Leventhal]

Instead of ShutdownXPAccessibility and ShutdownAccessibility methods shouldn't we now just move that into nsApplicationAccessible[Wrap]::Unload() or ::Shutdown() ? 

These are all just things that happen at the end of the life of Mozilla a11y. In fact I'm not sure why Unload() is different from Shutdown().

Comment 7

[Aaron Leventhal]

Why do we need |root = nsnull| here?

NS_IMETHODIMP nsRootAccessibleWrap::Shutdown()
{
    nsAppRootAccessible *root = nsAppRootAccessible::Create();
    if (root) {
        root->RemoveRootAccessible(this);
        root = nsnull;
    }

In the following code: there's no reason to use NS_IF_ADDREF (instead of NS_ADDREF) if you already know it's not null.
Also, there is no reason for the temporary variable |root|
BTW, why doesn't nsAppRootAccessible AddRef() for us? That would be the normal way of doing it, right?

NS_IMETHODIMP nsRootAccessibleWrap::GetParent(nsIAccessible **  aParent)
{
    nsAppRootAccessible *root = nsAppRootAccessible::Create();
    nsresult rv = NS_OK;
    if (root) {
        NS_IF_ADDREF(*aParent = root);
    }
    else {
        *aParent = nsnull;
        rv = NS_ERROR_FAILURE;
    }
    return rv;
}

	

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> So I guess we could file separate follow-up bug to rename
> nsAppRootAccessible.cpp -- the CVS admins can do that for us I think. We'd just
> need to change it in the Makefile as well.
> 

Ok. Yes we need to ask to copy with cvs history these files. Then we will remove old files and made some changes to link up new files.

Do you like new nsAccessibleApplication name?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #7)

> BTW, why doesn't nsAppRootAccessible AddRef() for us? That would be the normal
> way of doing it, right?

Looks correct.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Instead of ShutdownXPAccessibility and ShutdownAccessibility methods shouldn't
> we now just move that into nsApplicationAccessible[Wrap]::Unload() or
> ::Shutdown() ? 
> 
> These are all just things that happen at the end of the life of Mozilla a11y.
> In fact I'm not sure why Unload() is different from Shutdown().
> 

Ginn made Upload() method in bug 330780.

Comment 11

[Aaron Leventhal]

> Do you like new nsAccessibleApplication name?
Yes.

>> BTW, why doesn't nsAppRootAccessible AddRef() for us? That would be the normal
>> way of doing it, right?
> Looks correct.
Do you mean you agree we should change that?

> Ginn made Upload() method in bug 330780.
Right, why isn't the Unload() work just done in Shutdown() ?

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #11)
> >> BTW, why doesn't nsAppRootAccessible AddRef() for us? That would be the normal
> >> way of doing it, right?
> > Looks correct.
> Do you mean you agree we should change that?

Yes, we can use already_AddRefed and nsRefPtr.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to comment #11)

> > Ginn made Upload() method in bug 330780.
> Right, why isn't the Unload() work just done in Shutdown() ?
> 

I guess the answer is:

(In reply to comment #11)
> > > nsAppRootAccessible *root = new nsAppRootAccessible();
> > Do we really need to create an object to shut down? That seems a bit odd. Can't
> > we just call a static method on nsAppRootAccessible to shut down the static
> > variable? Or, would you consider making sAppRoot a member of nsAccessNodeWrap?
> > Then I don't think you would need this, or am I missing something?
> 
> Shutdown is a virtual member function, so I have to change its name.
> 

Comment 14

[alexander :surkov (:asurkov)]

I assume application accessible is not in cache so Shutdown method is not called automatically therefore we would have that hack.

Comment 15

[Aaron Leventhal]

That makes sense.

But can we combine Unload() with Shutdown[XP]Accessibility?

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> But can we combine Unload() with Shutdown[XP]Accessibility?
> 

Then we should make visible for nsAccessNodeWrap (or move to nsAccessNodeWrap) some static variables defined in nsAppRootAccessible.cpp I guess.

Comment 17

[Aaron Leventhal]

Or vice-versa. You could take the code from Shutdown[XP]Accessibility and put it into Unload, no?

Comment 18

[Aaron Leventhal]

I mean, the right place to completely shutdown a11y would seem to be in nsApplicationAccessible, but I don't know if that causes problems because of static variable usage.

If it's too hard, we can file cleanup bug for later.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)

> If it's too hard, we can file cleanup bug for later.
> 

I would take a side two patches it's easier to understand cvs history.

Comment 20

[Aaron Leventhal]

Okay, we can do that for future low priority follow-up bug.

I would like to address comment 5 and comment 7 in this patch/bug. However, I will keep looking at current patch for more problems now.

Comment 21

[Aaron Leventhal]

I'm having a hard time understanding the changes in nsAppRootAccessible.cpp. Obviously a lot of stuff got moved, I understand that.

But, did the order of some of the methods like Init() and LoadGtkModule() change? It doesn't look like those methods do anything differently now, but the patch and CVS history changes are bigger because of them moving around.

Comment 22

[Aaron Leventhal]

- Why did the name change from Create() to PreCreate()
- Should we still override GetNextSibling() and GetPreviousSibling() or do we get that null return for free from the base impl?

Comment 23

[Aaron Leventhal]

- Will we need to implement [Add|Remove]NativeRootAccessible for native windows on Win32? I'm not sure, we can save that question for later.

- Thank you for changing nsRootAccessible::Init() to use GetApplicationAccessible() instead of Create(). It's clearer.

Comment 24

[Aaron Leventhal]

- Do we really need to override nsApplicationAccessible::GetParent()? Wouldn't we get that for free? (Just like nsApplicationAccessible::Get[Next|Previous]Sibling() as mentioned in comment 22)

- Does nsApplicationAccessible::Get[First|Last]Child work?

- Does nsRootAccessible::Get[Previous|Next]Sibling() work?

In future patch we might want to try and make nsApplicationAccessible use the same mFirstChild/mNextSibling caching of children that we use elsewhere, which would fix the first/last child on the app root and next/prev sibling and the root accessibles.

Comment 25

[Aaron Leventhal]

I am finished with my comments now. In general it looks very good but I will wait for answers and patch to address some of the comments (at least comment 5 and comment 7, perhaps some others).

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (From update of attachment 265404 [details] [diff] [review])
> It must be wrong that tmpAccWrap is defined twice here:

Looks no. There are tmpAccWrap and tmpAppAcc.

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Why do we need |root = nsnull| here?

I believe it's not needed.

> NS_IMETHODIMP nsRootAccessibleWrap::Shutdown()
> {
>     nsAppRootAccessible *root = nsAppRootAccessible::Create();
>     if (root) {
>         root->RemoveRootAccessible(this);
>         root = nsnull;
>     }
> 
> In the following code: there's no reason to use NS_IF_ADDREF (instead of
> NS_ADDREF) if you already know it's not null.

Agree.

> Also, there is no reason for the temporary variable |root|
> BTW, why doesn't nsAppRootAccessible AddRef() for us? That would be the normal
> way of doing it, right?

I'll change on already_AddRefed.

Comment 28

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> I'm having a hard time understanding the changes in nsAppRootAccessible.cpp.
> Obviously a lot of stuff got moved, I understand that.
> 
> But, did the order of some of the methods like Init() and LoadGtkModule()
> change? It doesn't look like those methods do anything differently now, but the
> patch and CVS history changes are bigger because of them moving around.
> 

Yes, mostly things were moved. I'll check one more time whether did I change the order of methods.

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #22)
> - Why did the name change from Create() to PreCreate()

I though that Create() means to create, but PreCreate() means to do some preliminary works before acutal creating. If you like to save Create() name then it's ok with me.

> - Should we still override GetNextSibling() and GetPreviousSibling() or do we
> get that null return for free from the base impl?
> 

At the first sight they should, probably they will be more expensive though. I'll check.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> - Will we need to implement [Add|Remove]NativeRootAccessible for native windows
> on Win32? I'm not sure, we can save that question for later.

I guess we do not need it on Windows. Every native window we open has own HWND and accessible is obtained by HWND iirc. But I'll check this too :)

> - Thank you for changing nsRootAccessible::Init() to use
> GetApplicationAccessible() instead of Create(). It's clearer.
> 

No, problem :). I was confused by Create() method too.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> - Do we really need to override nsApplicationAccessible::GetParent()? Wouldn't
> we get that for free? (Just like
> nsApplicationAccessible::Get[Next|Previous]Sibling() as mentioned in comment
> 22)

Looks we needn't.

> - Does nsApplicationAccessible::Get[First|Last]Child work?
> 
> - Does nsRootAccessible::Get[Previous|Next]Sibling() work?
> 
> In future patch we might want to try and make nsApplicationAccessible use the
> same mFirstChild/mNextSibling caching of children that we use elsewhere, which
> would fix the first/last child on the app root and next/prev sibling and the
> root accessibles.
> 

Agree. So, I can address these questions in following up bug?

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)

> Agree. So, I can address these questions in following up bug?
> 

Or is it better to fix them before this bug?

Comment 33

[Aaron Leventhal]

I will wait for a new patch that addresses the items you are ready to deal with now, e.g. stuff from comment 27, etc.

PreCreate() name is probably okay.

> At the first sight they should, probably they will be more expensive though.
> I'll check.
In this scenario they will be fast enough, so I would optimize for code size over speed. In other words, just get rid of them if they're not necessary.

Fixing firstChild/nextSibling can be addressed in a follow-up bug.

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch2

1) order of methods in nsAppRootAccessible.h is saved
2) GetApplicationAccessible uses alreadyAddRefed<>

I would like to save prev/nextSibling/first/lastChild/Unload issues for following up bug and to pose this bug as application accessible moving to cross platform code.

Comment 35

[[e-mail]]

Comment on attachment 266584 [details] [diff] [review]
patch2

>+    nsRefPtr<nsApplicationAccessibleWrap> appAccWrap =
>+        nsAccessNode::GetApplicationAccessible();
>+    nsAccessibleWrap* tmpAppAccWrap =
>+        NS_STATIC_CAST(nsAccessibleWrap*, appAccWrap.get());
>+
>+    if (tmpAppAccWrap != tmpAccWrap && !tmpAccWrap->IsValidObject())
>         return NS_ERROR_INVALID_POINTER;
Can't you compare tmpAccWrap to appAccWrap (or appAccWrap.get()) directly?

>+  NS_IF_ADDREF(gApplicationAccessible);
gApplicationAccessible is never null at this point.

>+  nsresult rv = NS_OK;
>+  mChildren = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
>+  return rv;
No need to init rv, do_CreateInstance always overwrites it.

>+NS_IMETHODIMP
>+nsApplicationAccessible::GetDescription(nsAString& aDescription)
>+{
>+  GetName(aDescription);
>+  aDescription.AppendLiteral(" Root Accessible");
Root?

> /* readonly attribute nsIAccessible accParent; */
> NS_IMETHODIMP nsRootAccessible::GetParent(nsIAccessible * *aParent) 
>-{ 
>+{
>+  NS_ENSURE_ARG_POINTER(aParent);
>   *aParent = nsnull;
>+
>+  nsRefPtr<nsApplicationAccessibleWrap> root = GetApplicationAccessible();
>+  NS_IF_ADDREF(*aParent = root);
*aParent = GetApplicationAccessible(); might work. Or it might not ;-)

Comment 36

[Ginn Chen]

besides Neil's comments, 2 nits

1)
In src/atk/nsAccessibleWrap.cpp
you don't need
+#ifdef MOZ_ACCESSIBILITY_ATK
+#include "nsAppRootAccessible.h"
+#else
+#include "nsApplicationAccessibleWrap.h"
+#endif
just keep
#include "nsAppRootAccessible.h"
no big deal, we will rename it any way

2) I happen to find 2 extra space before NS_ENSURE_ARG_POINTER
+nsApplicationAccessible::AddRootAccessible(nsIAccessible *aRootAccessible)
+{
+    NS_ENSURE_ARG_POINTER(aRootAccessible);
+
+  // add by weak reference

Comment 37

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> (From update of attachment 266584 [details] [diff] [review])
> >+    nsRefPtr<nsApplicationAccessibleWrap> appAccWrap =
> >+        nsAccessNode::GetApplicationAccessible();
> >+    nsAccessibleWrap* tmpAppAccWrap =
> >+        NS_STATIC_CAST(nsAccessibleWrap*, appAccWrap.get());
> >+
> >+    if (tmpAppAccWrap != tmpAccWrap && !tmpAccWrap->IsValidObject())
> >         return NS_ERROR_INVALID_POINTER;
> Can't you compare tmpAccWrap to appAccWrap (or appAccWrap.get()) directly?

I got conversion error here.

Comment 38

[alexander :surkov (:asurkov)]

Attachment: patch3

with neil's and ginn's comments, added wrap files.

Comment 39

[Aaron Leventhal]

You can try SameCOMIdentity instead of ==

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #39)
> You can try SameCOMIdentity instead of ==
> 

SameCOMIdentity makes nsCOMPtr<nsISupports> and then compares them. Do I have any advantage with SameCOMIdentity?

Comment 41

[Aaron Leventhal]

No, just if it looks smaller / easier to read then use it if you want.

Comment 42

[Aaron Leventhal]

Are you leaving this open for more fixes?

Comment 43

[alexander :surkov (:asurkov)]

(In reply to comment #42)
> Are you leaving this open for more fixes?
> 

Actually, no. I rather will file new following up bugs.

Comment 44

[Aaron Leventhal]

Want to mark FIXED then?

Comment 45

[alexander :surkov (:asurkov)]

(In reply to comment #44)
> Want to mark FIXED then?

Yes. I filed followin up bugs so this one is fixed.

Comment 46

[Ginn Chen]

Sorry,

Neil's this comment is wrong,
>>+  NS_IF_ADDREF(gApplicationAccessible);
> gApplicationAccessible is never null at this point.

gApplicationAccessible is null, if XPA11y shuts down
It is causing crashes on Linux.

Comment 47

[Ginn Chen]

Attachment: patch to fix the crash