Closed Bug 975065 Opened 10 years ago Closed 10 years ago

implement Text accessible text range methods

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Depends on: 982212
So we need to add TextRange objects into the array what requires blocking bug 982212 fixed. As I understand there are certain technical problems and bug 982212 may be not fixed in nearest future. Trev  suggests to add a copy constructor for TextRange class. I lean towards that copy constructor approach will be a bad design and reference counting for TextRange would be preferable.

Neil, can I ask you to take a look at this class and give your opinion please?
Flags: needinfo?(neil)
If you want to avoid the three pairs of addrefs and releases, then you could define a move assignment operator and write
mTextRangeArray.AppendElement() = Move(aTextRange);

If you don't want to do that then my preference would be for the copy constructor over refcounting for TextRange; there'd be a significant penalty to separately allocating all of your TextRange objects when only a few of them would need to be copied.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk from comment #2)
> If you want to avoid the three pairs of addrefs and releases, then you could
> define a move assignment operator and write
> mTextRangeArray.AppendElement() = Move(aTextRange);

in that case I still need a copy constructor, right?
(In reply to alexander :surkov from comment #3)
> (In reply to neil@parkwaycc.co.uk from comment #2)
> > If you want to avoid the three pairs of addrefs and releases, then you could
> > define a move assignment operator and write
> > mTextRangeArray.AppendElement() = Move(aTextRange);
> 
> in that case I still need a copy constructor, right?

got it, that must be nicer than copy constructor approach
there's another problem, since TextRange keeps strong references to accessible objects and cycle collector is not aware of TextRange so it destroys accessible objects no taking into account TextRange objects. I'm curious if cycle collection stuff can be implemented with no ref counting.
Olli, you probably can advise something about cycle collection here
Hmm, what is the setup here? If TextRange (or the array containing them) is some temporary object which keeps strong references to cycle collectable object, there isn't anything to do.
But if TextRange objects stay alive longer, they should be traversed/unlinked.
(In reply to Olli Pettay [:smaug] from comment #7)
> Hmm, what is the setup here? If TextRange (or the array containing them) is
> some temporary object which keeps strong references to cycle collectable
> object, there isn't anything to do.
> But if TextRange objects stay alive longer, they should be
> traversed/unlinked.

TextRange may stay longer as part of its XPCOM wrapper (xpcTextRange class), I guess that class should implement cycle collecting (and then any class that is supposed to keep TextRange for a while).
What is the setup here? http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/TextRange.h#25 is not refcounted. What keeps it alive in case of xpc?
in my example if was a mochitest that kept xpcom text range alive
Aha, it has TextRange as a member variable.
Looks like TextRange should be refcounted object and cycle collectable, and never used
TextRange mFoo; member variable, or TextRange as a stack variable.
I'm not sure I got second part of your comment. But the idea was:

internal TextRange that impelments all logic
xpcTextRange that implements XPCOM interface and aggregates internal TextRange
uiaTextRange that impelments MS COM interface and aggregates internal TextRange
(In reply to alexander :surkov from comment #5)
> there's another problem, since TextRange keeps strong references to
> accessible objects and cycle collector is not aware of TextRange so it
> destroys accessible objects no taking into account TextRange objects. I'm
> curious if cycle collection stuff can be implemented with no ref counting.

I don't think there's any macros for it, but its perfectly possible to do,  you probably just have to more or less manually implement Traverse and Unlink.  As far as the actual cycle collecting goes

dealing with 
class foo
{
  nsRefPtr<T> x, y, z;
};

is the same as dealing with
class foo1
{
  nsRefPtr<T> x, y, z;
};

class foo
{
  foo1 mFoo;
};

given there memory layout is the same.

(In reply to Olli Pettay [:smaug] from comment #12)
> Aha, it has TextRange as a member variable.
> Looks like TextRange should be refcounted object and cycle collectable, and
> never used
> TextRange mFoo; member variable, or TextRange as a stack variable.

or we can always use it as a member of something else that either doesn't need to be cc'd (e.g. stuff !gecko owns) or have the cc'd object its a member of traverse / unlink it after all conceptually its the same as a couple of refptr members.

Now that has the downside that afaik we don't have things to make that super easy, but it shouldn't be super hard either, and maybe we can do some horrible template thing and make it nice.
(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> (In reply to alexander :surkov from comment #5)
> > there's another problem, since TextRange keeps strong references to
> > accessible objects and cycle collector is not aware of TextRange so it
> > destroys accessible objects no taking into account TextRange objects. I'm
> > curious if cycle collection stuff can be implemented with no ref counting.
> 
> I don't think there's any macros for it, but its perfectly possible to do, 
> you probably just have to more or less manually implement Traverse and
> Unlink.

if you volunteer for that then it's perfectly fine with me
Attached patch patch (obsolete) — Splinter Review
I tried to implement cycle collection on xpc TextRange object but it crashes now on destruction time of internal TextRange. Are there any ideas?
What is the stack trace?
#0	0x000000010319928e in ~nsRefPtr at /Users/alex/mozilla/0411/obj-ff-dbg/widget/cocoa/../../dist/include/nsAutoPtr.h:894
#1	0x0000000103197065 in ~nsRefPtr at /Users/alex/mozilla/0411/obj-ff-dbg/widget/cocoa/../../dist/include/nsAutoPtr.h:892
#2	0x0000000104c0a5d5 in ~TextRange at /Users/alex/mozilla/0411/accessible/src/generic/../base/TextRange.h:22
#3	0x0000000104c03435 in ~TextRange at /Users/alex/mozilla/0411/accessible/src/generic/../base/TextRange.h:22
#4	0x0000000104c2c02c in ~xpcAccessibleTextRange at /Users/alex/mozilla/0411/accessible/src/xpcom/xpcAccessibleTextRange.h:20
#5	0x0000000104c2afe5 in ~xpcAccessibleTextRange at /Users/alex/mozilla/0411/accessible/src/xpcom/xpcAccessibleTextRange.h:20
#6	0x0000000104c29cea in mozilla::a11y::xpcAccessibleTextRange::DeleteCycleCollectable() at /Users/alex/mozilla/0411/accessible/src/xpcom/xpcAccessibleTextRange.cpp:28
#7	0x0000000104c2b141 in mozilla::a11y::xpcAccessibleTextRange::cycleCollection::DeleteCycleCollectable(void*) at /Users/alex/mozilla/0411/accessible/src/xpcom/xpcAccessibleTextRange.h:24
#8	0x0000000101602f9f in ~SnowWhiteKiller at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:2386
#9	0x00000001015ec725 in ~SnowWhiteKiller at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:2379
#10	0x00000001015d764d in nsCycleCollector::FreeSnowWhite(bool) at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:2550
#11	0x00000001015da6bc in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:3426
#12	0x00000001015da145 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*) at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:3288
#13	0x00000001015dc158 in nsCycleCollector_collect(nsICycleCollectorListener*) at /Users/alex/mozilla/0411/xpcom/base/nsCycleCollector.cpp:3841
#14	0x00000001035049de in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) at /Users/alex/mozilla/0411/dom/base/nsJSEnvironment.cpp:1936
#15	0x000000010348a36f in nsDOMWindowUtils::GarbageCollect(nsICycleCollectorListener*, int) at /Users/alex/mozilla/0411/dom/base/nsDOMWindowUtils.cpp:1470
#16	0x00000001016bc7ab in NS_InvokeByIndex at /Users/alex/mozilla/0411/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
I have no idea what Move() does to refcounted stuff. Why do you use Move() and not swap which
is usually used with nsCOM/RefPtr
(In reply to alexander :surkov from comment #16)
> Created attachment 8401981 [details] [diff] [review]
> patch
> 
> I tried to implement cycle collection on xpc TextRange object but it crashes
> now on destruction time of internal TextRange. Are there any ideas?

for me it asserts because the refcount became negative, but the patch appears reasonable so I'll investigate it.
ok, so I just spent the afternoon discover somebody is a nub, this crash has nothing to do with cycle collection.  If you look at xpcAccessbielTextRange::GetEndContainer() you see more or less *aEndContainer = mRange.mEndContainer which obviously doesn't AddRef the thing mRange.mEndContainer points to like it should.  GetStartContainer() suffers from the same bug.
Attached patch patch2Splinter Review
impl is somehow not ready and even broken but it's still good to have as structure fill (I need to extend TextRange interface to understand how best to implement it)
Assignee: nobody → surkov.alexander
Attachment #8401981 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8403512 - Flags: review?(trev.saunders)
Trev, plans for review?
Flags: needinfo?(trev.saunders)
Comment on attachment 8403512 [details] [diff] [review]
patch2

>+TextRange::Set(HyperTextAccessible* aRoot,

why not just use operator =?

> 
>+  TextRange& operator= (TextRange&& aRange)
>+  {
>+    mRoot = Move(aRange.mRoot);
>+    mStartContainer = Move(aRange.mStartContainer);
>+    mEndContainer = Move(aRange.mEndContainer);

technically using swap() might be better till 980753 is fixed, but whatever.

> void
> HyperTextAccessible::EnclosingRange(a11y::TextRange& aRange) const
> {
>+  if (IsTextField()) {
>+    aRange.Set(mDoc, const_cast<HyperTextAccessible*>(this), 0,
>+               const_cast<HyperTextAccessible*>(this), ChildCount());
>+  } else {
>+    aRange.Set(mDoc, mDoc, 0, mDoc, mDoc->ChildCount());

this not a text field case doesn't seem to make much sense.

> HyperTextAccessible::SelectionRanges(nsTArray<a11y::TextRange>* aRanges) const
> {
>+  aRanges->Clear();

can't have anything in it so just assert its empty

>+  ErrorResult rv;
>+  for (int32_t idx = 0; idx < sel->RangeCount(); idx++) {

hoist the call to RangeCount() and it returns uin32_t so use that for idx too.

>+    nsRange* DOMRange = sel->GetRangeAt(idx, rv);

why not use the version that doesn't take a ErrorResult?

> HyperTextAccessible::RangeByChild(Accessible* aChild,
>                                   a11y::TextRange& aRange) const
> {
>+  aRange.Set(mDoc, aChild, 0, aChild, aChild->ChildCount());

why mDoc not this?

> HyperTextAccessible::RangeAtPoint(int32_t aX, int32_t aY,
>                                   a11y::TextRange& aRange) const
> {
>+  Accessible* child = mDoc->ChildAtPoint(aX, aY, eDeepestChild);
>+  if (child)
>+    aRange.Set(mDoc, child, 0, child, child->ChildCount());

same

>+// nsISupports and cycle collection
>+
>+NS_IMPL_CYCLE_COLLECTION_3(xpcAccessibleTextRange,
>+                           mRange.mRoot,
>+                           mRange.mStartContainer,
>+                           mRange.mEndContainer)

have you actually seen this leak? the only way I can see a cycle of strong refs existing involves someone setting a property on a dom node to point at a text range.

>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(xpcAccessibleTextRange)
>+  NS_INTERFACE_MAP_ENTRY(nsIAccessibleTextRange)
>+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessibleTextRange)

why is it ambiguous?

>+++ b/accessible/tests/mochitest/moz.build
>@@ -21,16 +21,17 @@ DIRS += [
>     'role',
>     'scroll',
>     'selectable',
>     'states',
>     'table',
>     'text',
>     'textattrs',
>     'textcaret',
>+    'textrange',

just add textrange/a11y.ini to the next list then get rid of textrange/moz.build?
Attachment #8403512 - Flags: review?(trev.saunders) → review+
(In reply to alexander :surkov from comment #23)
> Trev, plans for review?

was on pto ;)
Flags: needinfo?(trev.saunders)
(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> (In reply to alexander :surkov from comment #23)
> > Trev, plans for review?
> 
> was on pto ;)

got it, I missed that somehow
(In reply to Trevor Saunders (:tbsaunde) from comment #24)
> >+TextRange::Set(HyperTextAccessible* aRoot,
> 
> why not just use operator =?

what object should it take?

> > void
> > HyperTextAccessible::EnclosingRange(a11y::TextRange& aRange) const
> > {
> >+  if (IsTextField()) {
> >+    aRange.Set(mDoc, const_cast<HyperTextAccessible*>(this), 0,
> >+               const_cast<HyperTextAccessible*>(this), ChildCount());
> >+  } else {
> >+    aRange.Set(mDoc, mDoc, 0, mDoc, mDoc->ChildCount());
> 
> this not a text field case doesn't seem to make much sense.

I follow IE impl here, i.e. textpattern is implemented by documents and text fields.

> > HyperTextAccessible::RangeByChild(Accessible* aChild,
> >                                   a11y::TextRange& aRange) const
> > {
> >+  aRange.Set(mDoc, aChild, 0, aChild, aChild->ChildCount());
> 
> why mDoc not this?

same

> >+NS_IMPL_CYCLE_COLLECTION_3(xpcAccessibleTextRange,
> >+                           mRange.mRoot,
> >+                           mRange.mStartContainer,
> >+                           mRange.mEndContainer)
> 
> have you actually seen this leak? the only way I can see a cycle of strong
> refs existing involves someone setting a property on a dom node to point at
> a text range.

iirc that was a cycle collection assertion

> 
> >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(xpcAccessibleTextRange)
> >+  NS_INTERFACE_MAP_ENTRY(nsIAccessibleTextRange)
> >+  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIAccessibleTextRange)
> 
> why is it ambiguous?

just copied/pasted, I believe they didn't have an explicit reason to be ambiguous as well.

> >+++ b/accessible/tests/mochitest/moz.build
> >@@ -21,16 +21,17 @@ DIRS += [
> >     'role',
> >     'scroll',
> >     'selectable',
> >     'states',
> >     'table',
> >     'text',
> >     'textattrs',
> >     'textcaret',
> >+    'textrange',
> 
> just add textrange/a11y.ini to the next list then get rid of
> textrange/moz.build?

I don't mind, why do we have two options here?
(In reply to alexander :surkov from comment #27)
> (In reply to Trevor Saunders (:tbsaunde) from comment #24)
> > >+TextRange::Set(HyperTextAccessible* aRoot,
> > 
> > why not just use operator =?
> 
> what object should it take?

just create a temporary? or maybe just make this a wrapper around operator = or the reverse if x = Move(TextRange(blah)) is too ugly

> > > void
> > > HyperTextAccessible::EnclosingRange(a11y::TextRange& aRange) const
> > > {
> > >+  if (IsTextField()) {
> > >+    aRange.Set(mDoc, const_cast<HyperTextAccessible*>(this), 0,
> > >+               const_cast<HyperTextAccessible*>(this), ChildCount());
> > >+  } else {
> > >+    aRange.Set(mDoc, mDoc, 0, mDoc, mDoc->ChildCount());
> > 
> > this not a text field case doesn't seem to make much sense.
> 
> I follow IE impl here, i.e. textpattern is implemented by documents and text
> fields.

why not other things that are editable like inputs? and why does TextPattern whatever it is matter here?

> > >+NS_IMPL_CYCLE_COLLECTION_3(xpcAccessibleTextRange,
> > >+                           mRange.mRoot,
> > >+                           mRange.mStartContainer,
> > >+                           mRange.mEndContainer)
> > 
> > have you actually seen this leak? the only way I can see a cycle of strong
> > refs existing involves someone setting a property on a dom node to point at
> > a text range.
> 
> iirc that was a cycle collection assertion

I don't see how that could make any sense, but I guess it doesn't matter.

> > >+++ b/accessible/tests/mochitest/moz.build
> > >@@ -21,16 +21,17 @@ DIRS += [
> > >     'role',
> > >     'scroll',
> > >     'selectable',
> > >     'states',
> > >     'table',
> > >     'text',
> > >     'textattrs',
> > >     'textcaret',
> > >+    'textrange',
> > 
> > just add textrange/a11y.ini to the next list then get rid of
> > textrange/moz.build?
> 
> I don't mind, why do we have two options here?

not really sure, though it seems like it would make more sense to remove the one involving more indirection.
(In reply to Trevor Saunders (:tbsaunde) from comment #28)
> (In reply to alexander :surkov from comment #27)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #24)
> > > >+TextRange::Set(HyperTextAccessible* aRoot,
> > > 
> > > why not just use operator =?
> > 
> > what object should it take?
> 
> just create a temporary? or maybe just make this a wrapper around operator =
> or the reverse if x = Move(TextRange(blah)) is too ugly

I think I like Set even if = tricks are not slower.

> > > this not a text field case doesn't seem to make much sense.
> > 
> > I follow IE impl here, i.e. textpattern is implemented by documents and text
> > fields.
> 
> why not other things that are editable like inputs?

I don't know. That's IE.
https://hg.mozilla.org/mozilla-central/rev/28765a60c8be
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: