Closed Bug 589399 Opened 9 years ago Closed 9 years ago

add pseudo HyperLinkAccessible interface

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access, perf)

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
see bug 574336 comment #5.

Side note: StartOffset/EndOffset implementation much quicker than existing one.
Attachment #468011 - Flags: review?(bolterbugz)
Attachment #468011 - Flags: approval2.0?
Attached patch patch2 (obsolete) — Splinter Review
fixed linux build errors
Attachment #468011 - Attachment is obsolete: true
Attachment #468102 - Flags: review?(bolterbugz)
Attachment #468102 - Flags: approval2.0?
Attachment #468011 - Flags: review?(bolterbugz)
Attachment #468011 - Flags: approval2.0?
I'm wondering if there is a better C++ pattern we can use here.
(In reply to comment #3)
> I'm wondering if there is a better C++ pattern we can use here.

what do you keep in mind?
Comment on attachment 468102 [details] [diff] [review]
patch2

Neil, what do you think?
Attachment #468102 - Flags: review?(neil)
Attachment #468102 - Flags: review?(neil) → feedback?(neil)
Comment on attachment 468102 [details] [diff] [review]
patch2

I don't think those reinterpret_casts are safe; I'd stick to do_QueryObject.

Or was there something else that you wanted my feedback on?
Attachment #468102 - Flags: feedback?(neil) → feedback-
(In reply to comment #6)
> Comment on attachment 468102 [details] [diff] [review]
> patch2
> 
> I don't think those reinterpret_casts are safe; I'd stick to do_QueryObject.

you're right. I've fixed this already.

> Or was there something else that you wanted my feedback on?

I think the question was if proposed architecture is good and there's not better and also easy solution.
Attached patch patch3Splinter Review
Attachment #468102 - Attachment is obsolete: true
Attachment #469756 - Flags: review?(bolterbugz)
Attachment #469756 - Flags: feedback?(neil)
Attachment #469756 - Flags: approval2.0?
Attachment #468102 - Flags: review?(bolterbugz)
Attachment #468102 - Flags: approval2.0?
Comment on attachment 469756 [details] [diff] [review]
patch3

Do you mean better than adding lots of virtual methods to your base class so that you don't have to call QueryInterface? The only thing better than that is if only one of your subclasses needs to implement the virtual methods; in that case you call one virtual method AsHyperlink() which returns the subclass or null if the accessible isn't a hyperlink, and then the virtual methods become normal methods on the subclass. For example:

XPCOM code:
class nsBar: public nsFoo, nsIBar {
  NS_DECL_NSIBAR
};
nsCOMPtr<nsIBar> bar(do_QueryInterface(foo));
if (bar) {
  rv = bar->Bar(this);
  rv = bar->Baz(this);
}

DeCOM code:
class nsFoo {
  virtual bool IsBar() { return false; }
  virtual void Bar(nsBaz*) { NS_NOTREACHED };
  virtual void Baz(nsBaz*) { NS_NOTREACHED };
};
class nsBar: public nsFoo {
  virtual bool IsBar() { return true; }
  virtual void Bar(nsBaz*);
  virtual void Baz(nsBaz*);
};
if (foo->IsBar()) {
  foo->Bar(this);
  foo->Baz(this);
}

DeVirtual code:
class nsBar;
class nsFoo {
  virtual nsBar* AsBar() { return nsnull; }
};
class nsBar: public nsFoo {
  virtual nsBar* AsBar() { return this; }
  void Bar(nsBaz*);
  void Baz(nsBaz*);
};
nsBar *bar = foo->AsBar();
if (bar) {
  bar->Bar(this);
  bar->Baz(this);
}

Having said that, I don't think that this applies in this particular case!
Attachment #469756 - Flags: feedback?(neil) → feedback+
Yes, some subclasses need to provide own implementation of these virtual methods. Other alternative could be a new abstact class and inherit the base class from it, it's similar to xpcom version. But while we're working with base class directly (nsAccessible) it sounds that it isn't much nicer than the chosen approach since all these methods are "visible" when I have pointer to base class.
Comment on attachment 469756 [details] [diff] [review]
patch3

> // readonly attribute boolean nsIAccessibleHyperLink::valid
> NS_IMETHODIMP
> nsAccessible::GetValid(PRBool *aValid)
> {
>   NS_ENSURE_ARG_POINTER(aValid);
>-  PRUint32 state = nsAccUtils::State(this);
>-  *aValid = (0 == (state & nsIAccessibleStates::STATE_INVALID));
>-  // XXX In order to implement this we would need to follow every link
>-  // Perhaps we can get information about invalid links from the cache
>-  // In the mean time authors can use role="link" aria-invalid="true"
>-  // to force it for links they internally know to be invalid
>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
>+
>+  *aValid = IsValid();
>   return NS_OK;
> }

We might want to set *aValid = PR_FALSE before the IsDefunct check.
Comment on attachment 469756 [details] [diff] [review]
patch3

Will the vtable for all our nsAccessible's now be larger? I'm unsure of the memory impact here.
(In reply to comment #11)
> > NS_IMETHODIMP
> > nsAccessible::GetValid(PRBool *aValid)

> We might want to set *aValid = PR_FALSE before the IsDefunct check.

right, thank you for the catch.

(In reply to comment #12)
> Comment on attachment 469756 [details] [diff] [review]
> patch3
> 
> Will the vtable for all our nsAccessible's now be larger? I'm unsure of the
> memory impact here.

yes but not per instance
Right the vtable is shared by the virtual classes, okay.

OK still going...
Comment on attachment 469756 [details] [diff] [review]
patch3

r+a=me (sorry again for the delay - I was on the fence about this but I think it may actually also help us morph accessibles on the fly if we need to do that in ARIA 2.0)

Let's not land this until after beta5 is tagged. (Which should be later today).
Attachment #469756 - Flags: review?(bolterbugz)
Attachment #469756 - Flags: review+
Attachment #469756 - Flags: approval2.0?
Attachment #469756 - Flags: approval2.0+
(In reply to comment #15)
> Comment on attachment 469756 [details] [diff] [review]
> patch3
> 
> r+a=me (sorry again for the delay - I was on the fence about this but I think
> it may actually also help us morph accessibles on the fly if we need to do that
> in ARIA 2.0)

It might be a problem for COM (MSAA and IA2)

> Let's not land this until after beta5 is tagged. (Which should be later today).

Sure, but we can't break it more than it is :)
Darn COM identity rules :(
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/82ed1b11d8f1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.