828141 - windows shouldn't hold onto text change events

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Trevor Saunders (:tbsaunde)]

Attachment: windows shouldn't hold onto events

Comment 2

[Trevor Saunders (:tbsaunde)]

Attachment: bug 828141 - windows shouldn't hold onto events

Comment 3

[alexander :surkov (:asurkov)]

could you give a short description of the bug for the record otherwise it might be hard to say why it was necessary in half of year? or it might be better: add a descriptive blocking meta bug.

btw, how do you want to handle XPCOM event wrappers?

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> could you give a short description of the bug for the record otherwise it
> might be hard to say why it was necessary in half of year? or it might be
> better: add a descriptive blocking meta bug.

holding onto the evnethas two bad effects, one it holds onto objects we don't really need, and it means events need to be refcounted which otherwise they don't really need to be.

> btw, how do you want to handle XPCOM event wrappers?

I think my plan is to autogenerate C++ from idl similar to DOM events.  The xpcom events will copy the data they need from the AccEvent when created.  How does it sound?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to alexander :surkov from comment #3)
> > could you give a short description of the bug for the record otherwise it
> > might be hard to say why it was necessary in half of year? or it might be
> > better: add a descriptive blocking meta bug.
> 
> holding onto the evnethas two bad effects, one it holds onto objects we
> don't really need, and it means events need to be refcounted which otherwise
> they don't really need to be.

thanks

> > btw, how do you want to handle XPCOM event wrappers?
> 
> I think my plan is to autogenerate C++ from idl similar to DOM events.  The
> xpcom events will copy the data they need from the AccEvent when created. 
> How does it sound?

It should be ok, I don't have other ideas.

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 699938 [details] [diff] [review]
bug 828141 - windows shouldn't hold onto events

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

::: accessible/src/msaa/HyperTextAccessibleWrap.cpp
@@ +16,5 @@
>  
> +StaticRefPtr<Accessible> HyperTextAccessibleWrap::sLastTextChangedAcc;
> +StaticAutoPtr<nsString> HyperTextAccessibleWrap::sLastTextChangedString;
> +uint32_t HyperTextAccessibleWrap::sLasttextChangeStart = 0;
> +uint32_t HyperTextAccessibleWrap::sLasttextChangeEnd = 0;

nit: pls uppercase 'text' in sLasttextChange' in both cases

btw, first two are named sLastTextChanged, but two following are named sLastTextChange, without 'd' in the end.

@@ +17,5 @@
> +StaticRefPtr<Accessible> HyperTextAccessibleWrap::sLastTextChangedAcc;
> +StaticAutoPtr<nsString> HyperTextAccessibleWrap::sLastTextChangedString;
> +uint32_t HyperTextAccessibleWrap::sLasttextChangeStart = 0;
> +uint32_t HyperTextAccessibleWrap::sLasttextChangeEnd = 0;
> +bool HyperTextAccessibleWrap::sLastTextChangeWasInsert = false;

would you rather have a struct having all these fields?

@@ +59,5 @@
>      Accessible* accessible = aEvent->GetAccessible();
> +    if (accessible && accessible->IsHyperText()) {
> +      sLastTextChangedAcc = accessible;
> +      if (!sLastTextChangedString)
> +        sLastTextChangedString = new nsString;

nit: I think I would prefer new nsString()

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> Comment on attachment 699938 [details] [diff] [review]
> bug 828141 - windows shouldn't hold onto events
> 
> Review of attachment 699938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/msaa/HyperTextAccessibleWrap.cpp
> @@ +16,5 @@
> >  
> > +StaticRefPtr<Accessible> HyperTextAccessibleWrap::sLastTextChangedAcc;
> > +StaticAutoPtr<nsString> HyperTextAccessibleWrap::sLastTextChangedString;
> > +uint32_t HyperTextAccessibleWrap::sLasttextChangeStart = 0;
> > +uint32_t HyperTextAccessibleWrap::sLasttextChangeEnd = 0;
> 
> nit: pls uppercase 'text' in sLasttextChange' in both cases
> 
> btw, first two are named sLastTextChanged, but two following are named
> sLastTextChange, without 'd' in the end.

which do you prefer?

> @@ +17,5 @@
> > +StaticRefPtr<Accessible> HyperTextAccessibleWrap::sLastTextChangedAcc;
> > +StaticAutoPtr<nsString> HyperTextAccessibleWrap::sLastTextChangedString;
> > +uint32_t HyperTextAccessibleWrap::sLasttextChangeStart = 0;
> > +uint32_t HyperTextAccessibleWrap::sLasttextChangeEnd = 0;
> > +bool HyperTextAccessibleWrap::sLastTextChangeWasInsert = false;
> 
> would you rather have a struct having all these fields?

I think not, it seems silly unless we find some other place it would be used.

> @@ +59,5 @@
> >      Accessible* accessible = aEvent->GetAccessible();
> > +    if (accessible && accessible->IsHyperText()) {
> > +      sLastTextChangedAcc = accessible;
> > +      if (!sLastTextChangedString)
> > +        sLastTextChangedString = new nsString;
> 
> nit: I think I would prefer new nsString()

ok

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> > nit: pls uppercase 'text' in sLasttextChange' in both cases
> > 
> > btw, first two are named sLastTextChanged, but two following are named
> > sLastTextChange, without 'd' in the end.
> 
> which do you prefer?

TextChange without 'd' to stay closer to used naming (AccTextChangeEvent)


> > @@ +17,5 @@
> > > +StaticRefPtr<Accessible> HyperTextAccessibleWrap::sLastTextChangedAcc;
> > > +StaticAutoPtr<nsString> HyperTextAccessibleWrap::sLastTextChangedString;
> > > +uint32_t HyperTextAccessibleWrap::sLasttextChangeStart = 0;
> > > +uint32_t HyperTextAccessibleWrap::sLasttextChangeEnd = 0;
> > > +bool HyperTextAccessibleWrap::sLastTextChangeWasInsert = false;
> > 
> > would you rather have a struct having all these fields?
> 
> I think not, it seems silly unless we find some other place it would be used.

ok

Comment 9

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b53f4c1ef7c

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7b53f4c1ef7c

Comment 11

[Jacek Caban]

Pushed mingw linkage failure (we compile with -fno-keep-inline-dllexport, so all includes must be included in .cpp files that use them):

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b8c19604f12

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/4b8c19604f12