746484 - text attributes concept don't get mapped well to nsIPersistentProperties

Status: RESOLVED DUPLICATE of bug 1714390

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

similar to bug 609352 but caused by different thoughts and a little bit more complicated.

the problem the text attribute can have more than one value. We could use nsIPersistentProperties and store things like "value1,value2" as a key value. But it'd be cool to have something else than strings.

sketchup:

Internal part:

class TextAttrs
{
public:
  void AddAttrValue(const nsIAtom* aKey, nsAString& aValue);
  const nsTArray<nsAString>* GetAttrValues(const nsIAtom* aKey) const;

  typedef nsClassHashTable<nsPtrHashKey<const nsIAtom> >, nsTArray<nsAString> > AttrsTable;
  void Enumerate(AttrsTable::EnumReadFunction enumFunc, void* userArg) const;

  PRUint32 Count();
private:
  AttrsTable mTable;
};

void
nsAccessible::GetTextAttributes(TextAttrs** aAttrs)
{
  *aAttrs = new TextAttrs();
}

nsAutoPtr<TextAttrs> textAttrs;
accessible->GetTextAttributes(getter_Transfers(textAttrs));

XPCOM part:

interface nsIAccessibleTextAttribute : public nsISupports
{
  readonly AString name;
  void getValues(out unsigned long valuesArraySize,
                 [retval, array, size_is(valuesArraySize)] out long valuesArray);
}

interface nsIAccessibleTextAttributes : public nsISupports
{
  void getValues(in AString aKey,
                 out unsigned long valuesArraySize,
                 [retval, array, size_is(valuesArraySize)] out AString valuesArray);

  void getAttributes(out unsigned long attrsArraySize,
                     [retval, array, size_is(attrsArraySize)] out nsIAccessibleTextAttribute* attrsArray);
}

class xpcTextAttr : public nsIAccessibleTextAttribute
{
public
  xpcTextAttr(const nsIAtom* aName, const nsTArray<nsAString>* aValues);
};

class xpcTextAttrs : public nsIAccessibleTextAttributes
{
public:
  xpcTextAttrs(const TextAttrs* aTextAttrs);

private:
  nsAutoPtr<TextAttrs> mAttrs;
};

xpcTextAttrs::GetValues(const nsAString& aKey, PRUint32* aValuesCount, AString** aValues)
{
}

xpcTextAttrs::GetAttributes(PRUint32* aAttrsCount, nsIAccessibleTextAttribute*** aAttrs)
{
  nsIAccessibleTextAttribute** attrs = new nsIAccessibleTextAttribute*[mAttrs->Count()];
}

Comment 1

[Trevor Saunders (:tbsaunde)]

> the problem the text attribute can have more than one value. We could use
> nsIPersistentProperties and store things like "value1,value2" as a key
> value. But it'd be cool to have something else than strings.

yeah, and I'll bet something non-xpcom will be faster too.

>   typedef nsClassHashTable<nsPtrHashKey<const nsIAtom> >,
> nsTArray<nsAString> > AttrsTable;
>   void Enumerate(AttrsTable::EnumReadFunction enumFunc, void* userArg) const;
> 
>   PRUint32 Count();
> private:
>   AttrsTable mTable;

using a hash table here feels like over kill, we know the set of atoms at build time right? so couldn't this class just hold a fixed array of pointers to arrays, and we'd have a mapping function atom -> idx?

> void
> nsAccessible::GetTextAttributes(TextAttrs** aAttrs)
> {
>   *aAttrs = new TextAttrs();

I'd prefer we stack allocate this unless you can explain a reason not to.

If we need to heap allocate this why do we need to use an out arg? Couldn't we add move summantics to nsAutoPtr if we need to?

>   void getValues(out unsigned long valuesArraySize,
>                  [retval, array, size_is(valuesArraySize)] out long
> valuesArray);

isn't that an array of strings?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)

> using a hash table here feels like over kill, we know the set of atoms at
> build time right? so couldn't this class just hold a fixed array of pointers
> to arrays, and we'd have a mapping function atom -> idx?

for certain accessible? attributes count even can be changed during accessible life cycle.

> > void
> > nsAccessible::GetTextAttributes(TextAttrs** aAttrs)
> > {
> >   *aAttrs = new TextAttrs();
> 
> I'd prefer we stack allocate this unless you can explain a reason not to.

I'd like to avoid the coping of hash or array

> If we need to heap allocate this why do we need to use an out arg? Couldn't
> we add move summantics to nsAutoPtr if we need to?

I think we could. A little bit slower but works.

> >   void getValues(out unsigned long valuesArraySize,
> >                  [retval, array, size_is(valuesArraySize)] out long
> > valuesArray);
> 
> isn't that an array of strings?

it is, copy/paste issue

Comment 3

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> 
> > using a hash table here feels like over kill, we know the set of atoms at
> > build time right? so couldn't this class just hold a fixed array of pointers
> > to arrays, and we'd have a mapping function atom -> idx?
> 
> for certain accessible? attributes count even can be changed during
> accessible life cycle.

sure, we don't know what any given accessible will actually use, but we do know what it could possibly use which is iirc a set of size < 15

> > > void
> > > nsAccessible::GetTextAttributes(TextAttrs** aAttrs)
> > > {
> > >   *aAttrs = new TextAttrs();
> > 
> > I'd prefer we stack allocate this unless you can explain a reason not to.
> 
> I'd like to avoid the coping of hash or array

I don't see why we'd need to copy the list of attributes, and I think we can just .swap() the arrays.

> > If we need to heap allocate this why do we need to use an out arg? Couldn't
> > we add move summantics to nsAutoPtr if we need to?
> 
> I think we could. A little bit slower but works.

why is it slower? Right now I'd expect basically identical code accept where the pointer is put.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> (In reply to alexander :surkov from comment #2)
> sure, we don't know what any given accessible will actually use, but we do
> know what it could possibly use which is iirc a set of size < 15

attribute names number is nearly constant, we could do loosing some flexibility.

> I don't see why we'd need to copy the list of attributes, and I think we can
> just .swap() the arrays.

don't we get weird things like?
Attributes a, b;
a = b;
// continue the work with b

> > > If we need to heap allocate this why do we need to use an out arg? Couldn't
> > > we add move summantics to nsAutoPtr if we need to?
> > 
> > I think we could. A little bit slower but works.
> 
> why is it slower? Right now I'd expect basically identical code accept where
> the pointer is put.

I meant work in other component slows our work.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > (In reply to alexander :surkov from comment #2)
> > sure, we don't know what any given accessible will actually use, but we do
> > know what it could possibly use which is iirc a set of size < 15
> 
> attribute names number is nearly constant, we could do loosing some
> flexibility.

yeah, but on the other hand should be a bit faster since less dynamic memory allocation and no hashing.

> > I don't see why we'd need to copy the list of attributes, and I think we can
> > just .swap() the arrays.
> 
> don't we get weird things like?
> Attributes a, b;
> a = b;
> // continue the work with b

how? usually I think we have something like

Attributes attrs;
hyperText->GetTextAttrs(attrs);
// map text attributes to platform or xpcom layer

> > > > If we need to heap allocate this why do we need to use an out arg? Couldn't
> > > > we add move summantics to nsAutoPtr if we need to?
> > > 
> > > I think we could. A little bit slower but works.
> > 
> > why is it slower? Right now I'd expect basically identical code accept where
> > the pointer is put.
> 
> I meant work in other component slows our work.

sure, so we could just return a raw pointer with the understanding the caller now owns the returned object

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> yeah, but on the other hand should be a bit faster since less dynamic memory
> allocation and no hashing.

ok, could you show a code snippet to make sure we are on the same page.

> > don't we get weird things like?
> > Attributes a, b;
> > a = b;
> > // continue the work with b
> 
> how? usually I think we have something like
> 
> Attributes attrs;
> hyperText->GetTextAttrs(attrs);
> // map text attributes to platform or xpcom layer

ok, if you didn't mean to add copy constructors then it's fine with me.

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> 
> > yeah, but on the other hand should be a bit faster since less dynamic memory
> > allocation and no hashing.
> 
> ok, could you show a code snippet to make sure we are on the same page.

something like

class Attributes
{
private:
  nsTArray<nsString> mAttrValues[15};

  int IndexOfAttr(nsIAtom* aAttr)
  {
    switch(aAttr) {
      case nsGkAtoms::hight: return 0;
      case nsGkAtoms::font: return 1;
...
     default: NS_NOTREACHED("unknown attribute!"); break;
    };
  }

public:
  void AddAttrValue(nsIAtom* aAttr, nsString& aAttrValue)
  {
    mAttrValues[IndexOfAttr(aAttr)].AppendElement(aAttrValue);
  }
};

on further consideration perhaps we should get rid of atoms and just have an enum like

enum Attr {
  eFont,
  ehight,
  eColor
};

I'm not sure what I want to do about access, one idea would be to hve format functions like

AtkAttributeSet* AsAtkAttributeSet()
{
  ...
}

BSTR* ToIA2AtributeFormat()
{ ... }

or we could have a method
nsTArray<nsString>* AttrValuesFor( aAttr)
{ return &mAttrValues[aAttr]; }

> 
> > > don't we get weird things like?
> > > Attributes a, b;
> > > a = b;
> > > // continue the work with b
> > 
> > how? usually I think we have something like
> > 
> > Attributes attrs;
> > hyperText->GetTextAttrs(attrs);
> > // map text attributes to platform or xpcom layer
> 
> ok, if you didn't mean to add copy constructors then it's fine with me.

I had no plans to make sets of attributes copyable.

Comment 8

[alexander :surkov (:asurkov)]

can you compare the array approach vs persistent attributes approach from memory perspective?

Comment 9

[James Teh [:Jamie]]

We replaced nsIPersistentProperties with AccAttributes in bug 1714390. AccAttributes can handle non-string values.