Implement Accessible::Attribute(const nsIAtom* aName, nsAString& aValue)

NEW
Unassigned

Status

()

Core
Disability Access APIs
6 years ago
5 years ago

People

(Reporter: davidb, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

The extracted method can at least be used on Mac (see bug 718700).

Updated

6 years ago
Blocks: 389800

Comment 1

6 years ago
making bug a more generic, xml-roles is not unique case we have (for example, ongoing IA2 1.3 needs it)
Blocks: 531850
No longer blocks: 389800
Summary: Extract a method for getting the xml-role from GetAttributes() → Implement Accessible::Attributes(const nsAString& aNames, nsAString& aValues)

Comment 2

6 years ago
changing to Accessible::Attribute(const nsIAtom* aName, nsAString& aValue) as Trev suggested iirc
Summary: Implement Accessible::Attributes(const nsAString& aNames, nsAString& aValues) → Implement Accessible::Attribute(const nsIAtom* aName, nsAString& aValue)

Comment 3

6 years ago
It seems it'd be a good idea if we had enum for attribute names, we could do a switch which faster than atom ifing. Ok?
(In reply to alexander :surkov from comment #3)
> It seems it'd be a good idea if we had enum for attribute names, we could do
> a switch which faster than atom ifing. Ok?

I'd guess it'll be slightly faster since the enum values are compile time constants, and never 8 bytes as pointers are on x64, but afaik the real win with a switch is using a jump table instead of a large number of compare and jumps which I suspect isn't going to be possible here since I think all the switches will look like

switch (value) {
  case 5: x; break;
  case 1: y; break;
  default: supper::foo();
}

not
switch (value) {
  case 0: x; break;
  case 1: y; break;
  case 2: z; break;
 ... in order
  case 20: c; break;
  default: return 0;
}

Comment 5

6 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #4)

> switch (value) {
>   case 5: x; break;
>   case 1: y; break;
>   default: supper::foo();
> }

making them alphabetical we can do 

switch (value) {
   case 1: x; break;
   case 5: y; break;
   default: supper::foo();
}

so compilers don't do any maric for this?
(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> 
> > switch (value) {
> >   case 5: x; break;
> >   case 1: y; break;
> >   default: supper::foo();
> > }
> 
> making them alphabetical we can do 
> 
> switch (value) {
>    case 1: x; break;
>    case 5: y; break;
>    default: supper::foo();
> }
> 
> so compilers don't do any maric for this?

I don't think there's much you *can* do for something like that other than compare against each posibility and branch.  On the other hand if you have all the numbers in some range you can just check your within the range and then compute an address based on where in the range you are to jump to.

Comment 7

6 years ago
generally the biggest if belongs to Accessible version, other versions usually don't expose more than couple of attributes. So if would do an enum then we are targeted primarily to Accessible::NativeAttributes and perhaps that's ok.

Alternatively we could provide split this method into a number of methods like XMLRolesAttr or just XMLRoles(). Not sure it makes things nicer.
(In reply to alexander :surkov from comment #7)
> generally the biggest if belongs to Accessible version, other versions
> usually don't expose more than couple of attributes. So if would do an enum
> then we are targeted primarily to Accessible::NativeAttributes and perhaps
> that's ok.
> 
> Alternatively we could provide split this method into a number of methods
> like XMLRolesAttr or just XMLRoles(). Not sure it makes things nicer.

I think I like enum or atom approach better, but don'tcare much between them.
So, I don't totally like the retun a string approach since it can often mean we need to turn int / bool / atom into a string, and then in the case of some platforms turn that string back into a useful value.

SO some alternate API ideas

struct AttrValue {
  enum Type
  {
    eAtom,
    eString,
    eInt,
    eBool,
    eUint
  };

  Type mType;
  uintptr_t value;

  operator =(int aVal)
  {
    mType = eInt;
    mValue = aVal;
  }

  operator =(bool aVal)
  {
  mType = eBool;
    mValue = aVal;
  }

  Type Type() const
  { return mType; }

  operator int()
{
  NS_ASSERTION(mType == eInt, "converting non int value to int!");
  return static_cast<int>(mValue);
}

operator nsIAtom*()
{
  NS_ASSERTION(mType == eAtom, "turning not atom value into a atom!!!");
  return static_cast<nsIAtom*>(mValue);
}

then we'd have
AttrValue Attribute(nsIAtom* aAttr);

strings might be a little tricky since there larger than a pointer, but that can probably be sorted out.

or we could do
each platform defines a class / inherits from a generic one
class ValueCallback
{
  operator =(nsIAtom* aVal)
  {
    mHashtable.put(mAttr, aVal);
  }

or for different platform
  operator =(bool aVal)
  {
    mString.Append(":");
    mString.Append(aVal ? "true" : "false");
  }
};

then have method
void Attribute(nsIAtom* aAttr, ValueCallback& aVal)
{
  if (aAttr == nsGkAtoms::level) {
    aVal = 5;
    return;
}

thoughts?

Comment 10

5 years ago
1) Is uintptr_t preferable over union?
2) We could avoid string usage by dealing with nsIAtom*. It makes sense if HTML attributes (ARIA attributes) are atom based.
3) Not sure I see connection between AttrValue and ValueCallback&.

On internal level it should be nice to have:
AttrValue Accessible::Attribute(nsIAtom* aName);

I think I lean towards to have an enum for attribute names, it should be a little bit faster than if else (atom ==)
(In reply to alexander :surkov from comment #10)
> 1) Is uintptr_t preferable over union?

well, it ensures you won't put anything bigger than a pointer in it, but I'm not sure which is more reasonable here.

> 2) We could avoid string usage by dealing with nsIAtom*. It makes sense if
> HTML attributes (ARIA attributes) are atom based.

I think we can most of the time, just not sure about all of it.

> 3) Not sure I see connection between AttrValue and ValueCallback&.

they're two different approaches.

> I think I lean towards to have an enum for attribute names, it should be a
> little bit faster than if else (atom ==)

maybe slightly, I don't particularly care, but I tend to lead away from one off enums that are subsets of similar things.

Comment 12

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> (In reply to alexander :surkov from comment #10)
> > 1) Is uintptr_t preferable over union?
> 
> well, it ensures you won't put anything bigger than a pointer in it, but I'm
> not sure which is more reasonable here.

union might be nicer

> > 2) We could avoid string usage by dealing with nsIAtom*. It makes sense if
> > HTML attributes (ARIA attributes) are atom based.
> 
> I think we can most of the time, just not sure about all of it.
> 
> > 3) Not sure I see connection between AttrValue and ValueCallback&.
> 
> they're two different approaches.
> 
> > I think I lean towards to have an enum for attribute names, it should be a
> > little bit faster than if else (atom ==)
> 
> maybe slightly, I don't particularly care, but I tend to lead away from one
> off enums that are subsets of similar things.

ok

would you like to take it?

Updated

5 years ago
Blocks: 873437

Comment 13

5 years ago
in the light of innerHTML object attribute how would AttrValue approach look like?
(In reply to alexander :surkov from comment #13)
> in the light of innerHTML object attribute how would AttrValue approach look
> like?

what is the question? innerHTML is just a big string type value.

Comment 15

5 years ago
atoms idea doesn't work so you said:

then we'd have
AttrValue Attribute(nsIAtom* aAttr);

strings might be a little tricky since there larger than a pointer, but that can probably be sorted out.

the question is how
(In reply to alexander :surkov from comment #15)
> atoms idea doesn't work so you said:

use raw nsStringBuffer*, or maybe nsString*?

Comment 17

5 years ago
short example pls? who will own the string?
You need to log in before you can comment on or make changes to this bug.