379366 - reimplement IAccessibleText::newText and oldText

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

from bug 378038:

(From update of attachment 263365 [details] [diff] [review])
Please file a follow-up bug to greatly reduce the footprint of keeping track of
oldText/newText etc. Don't use member variables, just keep track of the last 12
text changes or something like that. When a request for oldText or newText
comes in, find that object in the circular array and get the data for it there.

Comment #33 [reply] Aaron Leventhal  2007-05-01 09:58:34 PDT

Of course, you'll have to throw out older oldText/newText info when newer text
changes occur on the same object. But, you would have that problem if using
member variables anyway.

Comment 1

[Aaron Leventhal]

Actually I'm not sure whether we should bother storing more than 1 previous event. After all, if 2 events occur on the same text object, it's impossible to know which event the AT wants the info for, right? We'll have to assume the last one in that case. Therefore, we'll never be able to guarantee correct info for anything mor than the last event. That's part of the design of IA2, and there's nothing we can do about it. The results may not be gauranteed except for in-process clients, which ensures they can get the info before another event is fired on the same object.

So no circular array is necessary. Just keep a few globals to track the last change (or perhaps the last removal/oldText and last insertion/newText).

I'm not sure if this limitation in how many events we can return is a problem or not. It might not be.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

to store accessible event itself in static variable

Comment 3

[alexander :surkov (:asurkov)]

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

Ben, is there a crash in xulrunner from bug 378038?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Thanks for the quick patch! I'll test this tomorrow morning and post back here.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

bsmedberg, is it ok to have a static nsCOMPtr? Or should he manually AddRef/Release a simple interface pointer?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> bsmedberg, is it ok to have a static nsCOMPtr? Or should he manually
> AddRef/Release a simple interface pointer?
> 

It shoudn't be a problem, for example http://lxr.mozilla.org/mozilla/source/widget/src/gtk/nsWidget.h#410

Comment 7

[[e-mail]]

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

>-nsString nsHyperTextAccessibleWrap::sText;
>-PRInt32 nsHyperTextAccessibleWrap::sOffset = 0;
>-PRUint32 nsHyperTextAccessibleWrap::sLength = 0;
>-PRBool nsHyperTextAccessibleWrap::sIsInserted = PR_FALSE;
>+static nsCOMPtr<nsIAccessibleTextChangeEvent> sTextEvent;
Having read the comments in bug 370838 I have to disappoint you that you can't have a static nsCOMPtr either. However you can use a raw pointer and manage refcounts yourself.

Comment 8

[Aaron Leventhal]

Yes, static COMPtrs are a problem, from what I've heard.

However, I somehow missed the fact that you were already using static. I must not have noticed that changed in the last patches.

So, are any more changes even necessary?

Comment 9

[Aaron Leventhal]

BTW, you can use nsAccessNode::ShutdownXPAccessibility() to do any destruction of
static strings or nsCOMPtrs at the right time.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

Given those comments I'll wait for your next patch.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> Yes, static COMPtrs are a problem, from what I've heard.
> 
> However, I somehow missed the fact that you were already using static. I must
> not have noticed that changed in the last patches.
> 
> So, are any more changes even necessary?
> 

I thought probably it's more fine to keep a bit longer existing accessible event object instead to have additional several members to keep its info.

Comment 13

[Aaron Leventhal]

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

Don't we need to check if the accessible for the gTextAccessible == the hyper text we're asking for the modified text from?

If the last event occurs on Node A, and I ask for the modified text on Node B, I should get some kind of error. Maybe there is a COM equivalent to this:
/* Returned when an operation can't complete due to an unavailable resource */
199 #define NS_ERROR_NOT_AVAILABLE             ((nsresult) 0x80040111L)

Or, I think it's possible to return a NULL BSTR as opposed to an emoty string.

Comment 14

[Aaron Leventhal]

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

Clearing request until I hear an answer.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch3

something like this?

Comment 16

[Aaron Leventhal]

Here is a clear case where a couple of extra { } would make it more readable:
        if (NS_SUCCEEDED(rv)) {
          if (instancePtr)
            gTextEvent = do_QueryInterface(aEvent);
          (NS_STATIC_CAST(IUnknown*, instancePtr))->Release();
        }
Also, can rv ever be a failure code when instancePtr is non-null, or can rv be a success code when instancePtr is null?

I think you also need to check if (gTextAccessible == this), and return if it isn't. Otherwise you might return a change string for an accessible it didn't occur on.
   if ((aGetInsertedText && isInserted) || (!aGetInsertedText && !isInserted)) {

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 264076 [details] [diff] [review]
patch4

Our XULRunner builds no longer crash with this patch. Thank you!

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (From update of attachment 264076 [details] [diff] [review])
> Our XULRunner builds no longer crash with this patch. Thank you!
> 

Nice to hear, thank you for the review.

Comment 20

[[e-mail]]

Comment on attachment 264076 [details] [diff] [review]
patch4

Well, I'm still not 100% happy, so don't be surprised if someone else crashes in ~nsCOMPtr.

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch5

Is it closer to 100% happiness? :)

Comment 22

[[e-mail]]

Comment on attachment 264599 [details] [diff] [review]
patch5

Closer, yes :-)

>+nsIAccessibleTextChangeEvent *nsAccessNodeWrap::gTextEvent;
= nsnull; please

>+  if (gTextEvent)
>+    gTextEvent->Release();
>+  gTextEvent = nsnull;
NS_IF_RELEASE(gTextEvent); (and similarly later)

> 
>-nsString nsHyperTextAccessibleWrap::sText;
>-PRInt32 nsHyperTextAccessibleWrap::sOffset = 0;
>-PRUint32 nsHyperTextAccessibleWrap::sLength = 0;
>-PRBool nsHyperTextAccessibleWrap::sIsInserted = PR_FALSE;
>+
> 
Only one blank line needed ;-)

Comment 23

[alexander :surkov (:asurkov)]

Attachment: patch6

for checking in

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 264604 [details] [diff] [review]
patch6

checked in