609352 - Consider alternatives to persistent properties usage for attributes

Status: RESOLVED DUPLICATE of bug 1714390

(Core :: Disability Access APIs, defect)

Description

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

We discovered neither Alexander nor I love nsAccUtils::SetAccAttr, so captured here. Spun off bug 593368.

Comment 1

[alexander :surkov (:asurkov)]

we could wrap nsPersistentProperties like

nsAccessible.h

namespace mozilla {
namespace a11y {

class Attributes : public nsPersistentProperties
{
public:
  void Set(const nsIAtom* aKey, const nsIAtom* aValue);
  void Set(const nsIAtom* aKey, const nsAString& aValue);
  void Get(const nsIAtom* aKey, nsAString& aValue) const;
};

}
}

and then use for GetAttributes() implementations like in nsAccessible.cpp
nsAccessible::GetAttributes(Attributes** aAttributes)
{
  if (!*aAttributes)
    *aAttributes = new Attributes();
}

that allows us to get rid helpers form nsAccUtils like GetAccAttr/SetAccAttr

please do the work after bug 740764 is fixed to avoid merging conflicts.

Comment 2

[Trevor Saunders (:tbsaunde)]

> class Attributes : public nsPersistentProperties

I don't think we have access to the concrete type implementing nsIPersistantProperties, so I'm not sure how this'll work.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > class Attributes : public nsPersistentProperties
> 
> I don't think we have access to the concrete type implementing
> nsIPersistantProperties, so I'm not sure how this'll work.

why not? just add a dependency on xpcom/ds and you can use nsPersistentProperties

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > > class Attributes : public nsPersistentProperties
> > 
> > I don't think we have access to the concrete type implementing
> > nsIPersistantProperties, so I'm not sure how this'll work.
> 
> why not? just add a dependency on xpcom/ds and you can use
> nsPersistentProperties

that feels pretty hacky, are you sure bsmedberg or similar is ok way with that sort of local include?

Also while this gets rid of the nsAccUtils stuff which is nice I guess I tend to think we'd get better performance by writing something ourselves that's less generic / isn't ref counted etc.

Comment 5

[alexander :surkov (:asurkov)]

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

> that feels pretty hacky, are you sure bsmedberg or similar is ok way with
> that sort of local include?

I don't see a reason why xpcom module owner would be against of this.

> Also while this gets rid of the nsAccUtils stuff which is nice I guess I
> tend to think we'd get better performance by writing something ourselves
> that's less generic / isn't ref counted etc.

more complicated but we can do that, but it means double work on XPCOM side since nsIPersistentProperties looks a good option for JS.

Internally we could use simple
typedef nsDataHashtable<nsPtrHashKey<const nsIAtom>, nsString> Attributes;

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> 
> > that feels pretty hacky, are you sure bsmedberg or similar is ok way with
> > that sort of local include?
> 
> I don't see a reason why xpcom module owner would be against of this.
> 
> > Also while this gets rid of the nsAccUtils stuff which is nice I guess I
> > tend to think we'd get better performance by writing something ourselves
> > that's less generic / isn't ref counted etc.
> 
> more complicated but we can do that, but it means double work on XPCOM side
> since nsIPersistentProperties looks a good option for JS.

not sure about double, but certainly a little more complicated.

> Internally we could use simple
> typedef nsDataHashtable<nsPtrHashKey<const nsIAtom>, nsString> Attributes;

Do we actually need to be able to very quickly look up a particular attribute?  I've been menaing to look into that, but never had time.  I'dd happily do this, but it seems a linked list would be even better if we don't need lookup.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > more complicated but we can do that, but it means double work on XPCOM side
> > since nsIPersistentProperties looks a good option for JS.
> 
> not sure about double, but certainly a little more complicated.

we should create a structure for internals and another structure for XPCOM side (if we stay on nsIPersistentProperties approach).

> > Internally we could use simple
> > typedef nsDataHashtable<nsPtrHashKey<const nsIAtom>, nsString> Attributes;
> 
> Do we actually need to be able to very quickly look up a particular
> attribute?  I've been menaing to look into that, but never had time.  I'dd
> happily do this, but it seems a linked list would be even better if we don't
> need lookup.

yes, we have couple cases when we look whether the attribute is presented (see nsAccUtils::GetAccAttr and GetStringProperty). These are rather exceptions though.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > > more complicated but we can do that, but it means double work on XPCOM side
> > > since nsIPersistentProperties looks a good option for JS.
> > 
> > not sure about double, but certainly a little more complicated.
> 
> we should create a structure for internals and another structure for XPCOM
> side (if we stay on nsIPersistentProperties approach).

true, I guess I was considering that an internal structure that was unrelated to nsIPersistantProperties would probably be nicer for internal use for example when we need to convert to platform attribute set not using nsIEnumerator seems nice.

> > > Internally we could use simple
> > > typedef nsDataHashtable<nsPtrHashKey<const nsIAtom>, nsString> Attributes;
> > 
> > Do we actually need to be able to very quickly look up a particular
> > attribute?  I've been menaing to look into that, but never had time.  I'dd
> > happily do this, but it seems a linked list would be even better if we don't
> > need lookup.
> 
> yes, we have couple cases when we look whether the attribute is presented
> (see nsAccUtils::GetAccAttr and GetStringProperty). These are rather
> exceptions though.

Ok, I'll try and look, but perhaps we should just not worry about it and use an array, and if profiling shows this is important we can consider the linked list idea.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> Ok, I'll try and look, but perhaps we should just not worry about it and use
> an array, and if profiling shows this is important we can consider the
> linked list idea.

alternatively we could get rid logic by replacing it on something else.

ok, can you give a code snippet that's good enough for good first bug please?

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> > Ok, I'll try and look, but perhaps we should just not worry about it and use
> > an array, and if profiling shows this is important we can consider the
> > linked list idea.
> 
> alternatively we could get rid logic by replacing it on something else.

so, there seems to be two places we need to do this one is the OuterDocAccessible stuff which iirc we thought didn't make much sense in bug 740764, and setting the live attribute if it isn't already set in nsAccessible.cpp.

> ok, can you give a code snippet that's good enough for good first bug please?

ok, here's a first idea, I don't think its perfect, but is someplace to start.

class Attributes
{
  void SetAttribute(nsIAtom* aAttr, nsAString& aValue)
  { mAttrs.put(aAttr, aValue); }

  bool IsAttrSet(nsIAtom& aAttr)
    { return mAttrs.Get(aAttr, nsnull); }

#ifdef MOZ_ATK
  AtkAttributeSet& AsATKAttributeSet();
#elif WIN
  BSTR& AsIA2AttributeSet();
#endif

private:
  nsClassHashtable<const nsIAtom, nsAString> mAttrs;
};

Attributes attrs;
accWrap->GetAttriubtes(&ttrs);
AtkAttributeSet& atkAttrs = attrs.AsATKAttributeSet():

nsAccessible::GetAttributes(Attributes& aAttrs)
{
  aAttrs.AddAttr(nsGkAtoms::tag, tag);

  if (!aAttrs->IsAttrSet(nsGkAtoms::live))
    aAttrs->AddAttr(nsGkAtoms::live, live);

Comment 11

[alexander :surkov (:asurkov)]

nsClassHashtable owns the value object, nsDataHastable seems to be more suitable. But any reason to not just typedef the hastable and keep all platform specific attributes mapping on platform layer? (removing good first bug until we figure out the approach)

Comment 12

[alexander :surkov (:asurkov)]

Trevor, let's figure out an approach.

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> Trevor, let's figure out an approach.

I'm fine with comment 11, but I haven't really thought about the type of hash table to use yet.

Comment 14

[alexander :surkov (:asurkov)]

Mark, look if you want to take it

Comment 15

[Mark Capella [:capella]]

I'm happy to look ...

I haven't seen Nickc (Ye Kaiqi) on IRC for about a couple months now, and the last time he commented on bug 740764 was about 5 weeks ago. Is that still a blocker here as per comment 1 or was it just something to watch out for?

Have you decided on the approach / scope of this change? Starting with comment 1 seemed straight-forward, but then Trevor and you got into some fine-point discussions I didn't follow ... 

If this is still a moving target, I'll wait a bit for it to finalize.

Comment 16

[Trevor Saunders (:tbsaunde)]

> I haven't seen Nickc (Ye Kaiqi) on IRC for about a couple months now, and
> the last time he commented on bug 740764 was about 5 weeks ago. Is that
> still a blocker here as per comment 1 or was it just something to watch out
> for?

having that bug done would make your life simpler, but I think you could do this first.

> Have you decided on the approach / scope of this change? Starting with
> comment 1 seemed straight-forward, but then Trevor and you got into some
> fine-point discussions I didn't follow ... 

I think we agree on the approach in comment 11

Comment 17

[James Teh [:Jamie]]

AccAttributes replaces persistent properties as of bug 1714390.