Closed Bug 828141 Opened 7 years ago Closed 7 years ago

windows shouldn't hold onto text change events

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #699561 - Flags: review?(surkov.alexander)
Attachment #699561 - Attachment is obsolete: true
Attachment #699561 - Flags: review?(surkov.alexander)
Attachment #699938 - Flags: review?(surkov.alexander)
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?
Blocks: 829320
(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?
(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 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()
Attachment #699938 - Flags: review?(surkov.alexander) → review+
(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
(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
https://hg.mozilla.org/mozilla-central/rev/7b53f4c1ef7c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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
You need to log in before you can comment on or make changes to this bug.