move application accessible to cross platform code

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Trunk
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

62.12 KB, patch
Details | Diff | Splinter Review
1.09 KB, patch
Aaron Leventhal
: review+
Details | Diff | Splinter Review
(Assignee)

Description

11 years ago
Created attachment 265404 [details] [diff] [review]
patch
Attachment #265404 - Flags: review?(aaronleventhal)
(Assignee)

Updated

11 years ago
Assignee: aaronleventhal → surkov.alexander
OS: Windows XP → All
Hardware: PC → All

Comment 1

11 years ago
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?

Updated

11 years ago
Keywords: access

Comment 2

11 years ago
Comment on attachment 265404 [details] [diff] [review]
patch

Clearing request until I get overview of the strategy for the patch.
Attachment #265404 - Flags: review?(aaronleventhal)
(Assignee)

Comment 3

11 years ago
(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

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
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;
}

	

(Assignee)

Comment 8

11 years ago
(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?
(Assignee)

Comment 9

11 years ago
(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.
(Assignee)

Comment 10

11 years ago
(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

11 years ago
> 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() ?
(Assignee)

Comment 12

11 years ago
(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.
(Assignee)

Comment 13

11 years ago
(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.
> 

(Assignee)

Comment 14

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

Comment 15

11 years ago
That makes sense.

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

Comment 16

11 years ago
(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

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

Comment 18

11 years ago
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.
(Assignee)

Comment 19

11 years ago
(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

11 years ago
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

11 years ago
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

11 years ago
- 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

11 years ago
- 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

11 years ago
- 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

11 years ago
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).
(Assignee)

Comment 26

11 years ago
(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.
(Assignee)

Comment 27

11 years ago
(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.
(Assignee)

Comment 28

11 years ago
(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.
(Assignee)

Comment 29

11 years ago
(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.
(Assignee)

Comment 30

11 years ago
(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.
(Assignee)

Comment 31

11 years ago
(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?
(Assignee)

Comment 32

11 years ago
(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

11 years ago
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.

(Assignee)

Comment 34

11 years ago
Created attachment 266584 [details] [diff] [review]
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.
Attachment #265404 - Attachment is obsolete: true
Attachment #266584 - Flags: review?(aaronleventhal)
(Assignee)

Updated

11 years ago
Attachment #266584 - Flags: superreview?(neil)
Attachment #266584 - Flags: review?(ginn.chen)

Comment 35

11 years ago
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 ;-)
Attachment #266584 - Flags: superreview?(neil) → superreview+

Updated

11 years ago
Attachment #266584 - Flags: review?(aaronleventhal) → review+

Comment 36

11 years ago
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

Updated

11 years ago
Attachment #266584 - Flags: review?(ginn.chen) → review+
(Assignee)

Comment 37

11 years ago
(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.
(Assignee)

Comment 38

11 years ago
Created attachment 266735 [details] [diff] [review]
patch3

with neil's and ginn's comments, added wrap files.
Attachment #266584 - Attachment is obsolete: true

Comment 39

11 years ago
You can try SameCOMIdentity instead of ==
(Assignee)

Comment 40

11 years ago
(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

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

Comment 42

11 years ago
Are you leaving this open for more fixes?
(Assignee)

Comment 43

11 years ago
(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

11 years ago
Want to mark FIXED then?
(Assignee)

Comment 45

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

Yes. I filed followin up bugs so this one is fixed.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 46

11 years ago
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

11 years ago
Created attachment 267118 [details] [diff] [review]
patch to fix the crash
Attachment #267118 - Flags: review?(surkov.alexander)

Updated

11 years ago
Attachment #267118 - Flags: review?(surkov.alexander) → review+
You need to log in before you can comment on or make changes to this bug.