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

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect, P3)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 1

[alexander :surkov (:asurkov)]

making bug a more generic, xml-roles is not unique case we have (for example, ongoing IA2 1.3 needs it)

Comment 2

[alexander :surkov (:asurkov)]

changing to Accessible::Attribute(const nsIAtom* aName, nsAString& aValue) as Trev suggested iirc

Comment 3

[alexander :surkov (:asurkov)]

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?

Comment 4

[Trevor Saunders (:tbsaunde)]

(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

[alexander :surkov (:asurkov)]

(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?

Comment 6

[Trevor Saunders (:tbsaunde)]

(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

[alexander :surkov (:asurkov)]

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.

Comment 8

[Trevor Saunders (:tbsaunde)]

(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.

Comment 9

[Trevor Saunders (:tbsaunde)]

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

[alexander :surkov (:asurkov)]

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 ==)

Comment 11

[Trevor Saunders (:tbsaunde)]

(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

[alexander :surkov (:asurkov)]

(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?

Comment 13

[alexander :surkov (:asurkov)]

in the light of innerHTML object attribute how would AttrValue approach look like?

Comment 14

[Trevor Saunders (:tbsaunde)]

(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

[alexander :surkov (:asurkov)]

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

Comment 16

[Trevor Saunders (:tbsaunde)]

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

use raw nsStringBuffer*, or maybe nsString*?

Comment 17

[alexander :surkov (:asurkov)]

short example pls? who will own the string?

Comment 18

[Morgan Reschenberg [:morgan]]

:eeejay marked as [mac2020_1] but this is attribute stuff, and we're moving to method stuff, so ?ni'ing you to see if we should consider closing.

Comment 19

[Eitan Isaacson [:eeejay]]

I think this is about our internal Gecko attribute bag. It is a collection of arbitrary attributes that don't have formal object properties. In this bug they mentioned exporting the xml-roles attribute to the platform. I'm not sure which other attributes need to be exported, if any.

I guess investigating this, and figuring it out would be the next step here.

Comment 20

[James Teh [:Jamie]]

I think it's probably just easier to split attributes we need separately into separate methods as these cases arise. We've already started doing this with methods like Accessible::DOMNodeID, ::DOMNodeClass, ::GetIntARIAAttr, ::LiveRegionAttributes, ::ARIASelected, ::Opacity, etc. I don't see a good reason to deviate from that pattern at this stage.