523304 - expose text-underline-color and text-line-through-color text attributes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

name: text-underline-color and text-line-through-color
value: <color>
default: rgb(0,0,0)
comments: <color> as rgb(n,n,n) where n is 0-255 
reference:  Specified in the ODF spec at

15.4.9 Line-Through Color - http://docs.oasis-open.org/office/v1.1/OS/OpenDocument-v1.1-html/OpenDocument-v1.1.html#outline:15.4.9.Line-Through_Color
15.4.31 Underline Color - http://docs.oasis-open.org/office/v1.1/OS/OpenDocument-v1.1-html/OpenDocument-v1.1.html#outline:15.4.31.Underline_Color

Comment 1

[alexander :surkov (:asurkov)]

Example:

<span style="text-decoration: underline; color: red"><span style="color: black">text</span></span>

Black text is underlined by red line (see http://www.w3.org/TR/2008/REC-CSS2-20080411/text.html#lining-striking-props)

Comment 2

[alexander :surkov (:asurkov)]

Attachment: cleanup:v1

fit text attr interface and do cleanup

Comment 3

[alexander :surkov (:asurkov)]

Attachment: fix

expose attributes

Comment 4

[Trevor Saunders (:tbsaunde)]

just commenting on resulting files since they're easier to review than the patch.


class nsHyperTextAccessible;


don't forward decls usually get put after includes.


namespace mozilla {
namespace a11y {

/**
 * Used to expose text attributes for the hyper text accessible (see
 * nsHyperTextAccessible class). It is indended for the work with 'language' and
 * CSS based text attributes.

typeo intended and you might want to generally update this comment.

 *
 * @note "invalid: spelling" text attrbiute is implemented entirerly in

typeo

 *       nsHyperTextAccessible class.
 */
class TextAttrsMgr
{
public:
  /**
   * Constructor. If instance of the class is intended to expose default text
   * attributes then 'aIncludeDefAttrs' and 'aOffsetNode' argument must be
   * skiped.

typeo

   * @param oOffsetNode      [optional] DOM node represents hyper text offset

aOffsetNode

  TextAttrsMgr(nsHyperTextAccessible* aHyperTextAcc,
               bool aIncludeDefAttrs = true,
               nsAccessible* aOffsetAcc = nsnull,
               PRInt32 aOffsetAccIdx = -1);

I think I'd rather have two seperate constructors, and a common function if that makes things cleaner.

  /*
   * Return text attributes and hyper text offsets where these attributes are
   * applied. Offsets are calculated in the case of non default attributes.
   *
   * @note In the case of default attributes pointers on hyper text offsets
   *       must be skiped.

typeo skipped

  void GetAttributes(nsIPersistentProperties* aAttributes,
                     PRInt32* aStartHTOffset = nsnull,
                     PRInt32* aEndHTOffset = nsnull);

again two versions might be nicer.

carrying the state of if you can get default attrs or not from constructor to this function feels weird to me.  Is there a reason this design is cleaner than one or two static methods?

  nsRefPtr<nsHyperTextAccessible> mHyperTextAcc;

  bool mIncludeDefAttrs;

  nsRefPtr<nsAccessible> mOffsetAcc;

do we still need strong refs ever?

  class ITextAttr

name makes me think its a windows COM interface, we have nsARefreshObserver, so maybe ATextAttr for abstract, or I think I like GenericTextAttr though its a little longer.

     * @param aAttributes           [in] the given attribute set
     * @param aIncludeDefAttrValue  [in] if true then attribute is exposed event

even

      bool isDefined = mIsDefined;
      T* nativeValue = &mNativeValue;

      if (!isDefined) {
        if (aIncludeDefAttrValue) {
          isDefined = mIsRootDefined;
          nativeValue = &mRootNativeValue;
        }
      } else if (!aIncludeDefAttrValue) {
        isDefined = mRootNativeValue != mNativeValue;
      }

      if (isDefined)
        ExposeValue(aAttributes, *nativeValue);

I think getting rid of isDefined and explicitly testing the state and calling ExposeValue() in each case would be simpler and easier to understand.

  // Embedded objects are combined into own range with empty attributes set.
  if (mOffsetAcc && nsAccUtils::IsEmbeddedObject(mOffsetAcc)) {
    for (PRInt32 childIdx = mOffsetAccIdx - 1; childIdx >= 0; childIdx--) {
      nsAccessible *currAcc = mHyperTextAcc->GetChildAt(childIdx);
      if (!nsAccUtils::IsEmbeddedObject(currAcc))
        break;

      (*aStartHTOffset)--;
    }

    PRInt32 childCount = mHyperTextAcc->GetChildCount();
    for (PRInt32 childIdx = mOffsetAccIdx + 1; childIdx < childCount;
         childIdx++) {
      nsAccessible *currAcc = mHyperTextAcc->GetChildAt(childIdx);
      if (!nsAccUtils::IsEmbeddedObject(currAcc))
        break;

      (*aEndHTOffset)++;
    }

    return;
  }

make it its own function?

  textAttrArray.Clear();

sort of useless since the array is about to go away with the stack frame.

    *(aStartHTOffset) -= nsAccUtils::TextLength(currAcc);

Doesn't nsHypertextAccessible cache this info?

on the other hand if we don't have to go back far in the kids and the cache was recently cleared this might hurt more than it would help.

TextAttrsMgr::LangTextAttr::
  ExposeValue(nsIPersistentProperties* aAttributes, const nsString& aValue)

kind of weird formatting, but ok


I'm not sure I like the offsets computation stuff, api, but since atk and ia2 both have the same thing I gues we should go with it.  I would prefer that each attribute has an associated range, but I gues that might be slower per call although it might allow fewer calls.

I wonder if we could do something a bit nicer or atleast with less virtual functions using http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern discussed at http://blog.cdleary.com/2012/01/c-generic-wrappers-and-crtp-oh-mi/

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> just commenting on resulting files since they're easier to review than the
> patch.

ok, more deep clean up then :)

>   TextAttrsMgr(nsHyperTextAccessible* aHyperTextAcc,
>                bool aIncludeDefAttrs = true,
>                nsAccessible* aOffsetAcc = nsnull,
>                PRInt32 aOffsetAccIdx = -1);
> 
> I think I'd rather have two seperate constructors, and a common function if
> that makes things cleaner.

fine with me

>   void GetAttributes(nsIPersistentProperties* aAttributes,
>                      PRInt32* aStartHTOffset = nsnull,
>                      PRInt32* aEndHTOffset = nsnull);
> 
> again two versions might be nicer.
>
> carrying the state of if you can get default attrs or not from constructor
> to this function feels weird to me.  Is there a reason this design is
> cleaner than one or two static methods?

I'd say this is what the class for, to carry states. However if we would have two versions of GetAttributes then your point made sense I think.

>   nsRefPtr<nsHyperTextAccessible> mHyperTextAcc;
> 
>   bool mIncludeDefAttrs;
> 
>   nsRefPtr<nsAccessible> mOffsetAcc;
> 
> do we still need strong refs ever?

nah, when 728907 is fixed but unstrongrefing them now wouldn't add more problems than we could have.

>   class ITextAttr
> 
> name makes me think its a windows COM interface, we have nsARefreshObserver,
> so maybe ATextAttr for abstract, or I think I like GenericTextAttr though
> its a little longer.

I changed ITextAttr -> TextAttr, TextAttr -> TTextAttr

>       if (isDefined)
>         ExposeValue(aAttributes, *nativeValue);
> 
> I think getting rid of isDefined and explicitly testing the state and
> calling ExposeValue() in each case would be simpler and easier to understand.

this is flexible since it allows values comparable to 0, otherwise having a value we can't say whether we should expose it

> make it its own function?

maybe not so big to have own function for it

>   textAttrArray.Clear();
> 
> sort of useless since the array is about to go away with the stack frame.

ok

>     *(aStartHTOffset) -= nsAccUtils::TextLength(currAcc);
> 
> Doesn't nsHypertextAccessible cache this info?
> 
> on the other hand if we don't have to go back far in the kids and the cache
> was recently cleared this might hurt more than it would help.

it is, and 2nd it should less perfromant

> TextAttrsMgr::LangTextAttr::
>   ExposeValue(nsIPersistentProperties* aAttributes, const nsString& aValue)
> 
> kind of weird formatting, but ok

didn't find anything nicer

> I'm not sure I like the offsets computation stuff, api, but since atk and
> ia2 both have the same thing I gues we should go with it.  I would prefer
> that each attribute has an associated range, but I gues that might be slower
> per call although it might allow fewer calls.

it might be not comfortable to ATs, anyway as you said it's not something we could trade by

> I wonder if we could do something a bit nicer or atleast with less virtual
> functions using
> http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern discussed
> at http://blog.cdleary.com/2012/01/c-generic-wrappers-and-crtp-oh-mi/

sort of crazy thing but I like it. Not sure however if we can make the code nicer. Sounds like strange workaround of virtual functions. But maybe I need more time to get agree on this :)

Comment 6

[Trevor Saunders (:tbsaunde)]

> I changed ITextAttr -> TextAttr, TextAttr -> TTextAttr

ok,  sounds fine

> >       if (isDefined)
> >         ExposeValue(aAttributes, *nativeValue);
> > 
> > I think getting rid of isDefined and explicitly testing the state and
> > calling ExposeValue() in each case would be simpler and easier to understand.
> 
> this is flexible since it allows values comparable to 0, otherwise having a
> value we can't say whether we should expose it

I'm not sure I follow, I'll try and look again and see if I can see what you mean here.

> > make it its own function?
> 
> maybe not so big to have own function for it

I was thinking more the function its in is big so breaking self contained bits off might be nice, but if you like to keep together ok

> >   textAttrArray.Clear();
> > 
> > sort of useless since the array is about to go away with the stack frame.
> 
> ok

kind of pointless given your later patches, sorry.

> >     *(aStartHTOffset) -= nsAccUtils::TextLength(currAcc);
> > 
> > Doesn't nsHypertextAccessible cache this info?
> > 
> > on the other hand if we don't have to go back far in the kids and the cache
> > was recently cleared this might hurt more than it would help.
> 
> it is, and 2nd it should less perfromant

I'm not sure I understand what your saying.

> sort of crazy thing but I like it. Not sure however if we can make the code
> nicer. Sounds like strange workaround of virtual functions. But maybe I need
> more time to get agree on this :)

sure, I'm not sure if its workable or not either, but in some sense not having virtual functions makes me think its cleaner, especially when there's only one impl :)

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > >       if (isDefined)
> > >         ExposeValue(aAttributes, *nativeValue);
> > > 
> > > I think getting rid of isDefined and explicitly testing the state and
> > > calling ExposeValue() in each case would be simpler and easier to understand.
> > 
> > this is flexible since it allows values comparable to 0, otherwise having a
> > value we can't say whether we should expose it
> 
> I'm not sure I follow, I'll try and look again and see if I can see what you
> mean here.

not real case but illustration. say you the text attribute value is provided by integer, but sometimes we should expose the attribute and sometimes we don't. Having a integer only we can't do that decision, that's what isDefined stuffs are supposed for.

> > > make it its own function?
> > 
> > maybe not so big to have own function for it
> 
> I was thinking more the function its in is big so breaking self contained
> bits off might be nice, but if you like to keep together ok

I'm not big fun of big functions but splitting them into parts just to make big function smaller is not always the idea I follow. I think in this case I would leave it as is.

> > >     *(aStartHTOffset) -= nsAccUtils::TextLength(currAcc);
> > > 
> > > Doesn't nsHypertextAccessible cache this info?
> > > 
> > > on the other hand if we don't have to go back far in the kids and the cache
> > > was recently cleared this might hurt more than it would help.
> > 
> > it is, and 2nd it should less perfromant
> 
> I'm not sure I understand what your saying.

I meant AccIterator iter; while((iter->Next()) { } is very nice syntax but it does some extra stuffs like dynamic memory allocation. Maybe I wouldn't use it here. I'd say we need to rethink AccIterator implementation to make it nicer.

> > sort of crazy thing but I like it. Not sure however if we can make the code
> > nicer. Sounds like strange workaround of virtual functions. But maybe I need
> > more time to get agree on this :)
> 
> sure, I'm not sure if its workable or not either, but in some sense not
> having virtual functions makes me think its cleaner, especially when there's
> only one impl :)

yeah, but having abstract TextAttr (not templated) we can't stop virtual functions usage. But I admit it was interesting to read and that's something we(I) should keep in mind.

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > > >       if (isDefined)
> > > >         ExposeValue(aAttributes, *nativeValue);
> > > > 
> > > > I think getting rid of isDefined and explicitly testing the state and
> > > > calling ExposeValue() in each case would be simpler and easier to understand.
> > > 
> > > this is flexible since it allows values comparable to 0, otherwise having a
> > > value we can't say whether we should expose it
> > 
> > I'm not sure I follow, I'll try and look again and see if I can see what you
> > mean here.
> 
> not real case but illustration. say you the text attribute value is provided
> by integer, but sometimes we should expose the attribute and sometimes we
> don't. Having a integer only we can't do that decision, that's what
> isDefined stuffs are supposed for.

I'm not suggesting change the idea, just how the logic is layed out to something like this

      if (!mIsDefined && aIncludeDefAttrValue && mIsRootDefined)
        ExposeValue(aAttributes, mRootnativeValue);
      else if ((!aIncludeDefAttrValue && mNativeValue != mRootNativeValue) ||
                mIsDefined) {
        ExposeValue(aAttributes, mNativeValue);
      }

> > > > make it its own function?
> > > 
> > > maybe not so big to have own function for it
> > 
> > I was thinking more the function its in is big so breaking self contained
> > bits off might be nice, but if you like to keep together ok
> 
> I'm not big fun of big functions but splitting them into parts just to make
> big function smaller is not always the idea I follow. I think in this case I
> would leave it as is.

ok

> > > >     *(aStartHTOffset) -= nsAccUtils::TextLength(currAcc);
> > > > 
> > > > Doesn't nsHypertextAccessible cache this info?
> > > > 
> > > > on the other hand if we don't have to go back far in the kids and the cache
> > > > was recently cleared this might hurt more than it would help.
> > > 
> > > it is, and 2nd it should less perfromant
> > 
> > I'm not sure I understand what your saying.
> 
> I meant AccIterator iter; while((iter->Next()) { } is very nice syntax but
> it does some extra stuffs like dynamic memory allocation. Maybe I wouldn't
> use it here. I'd say we need to rethink AccIterator implementation to make
> it nicer.

yeah, no good idea off hand, but just for i = 0; i < kidCount; i++) { ... } isn't that bd so...

> > > sort of crazy thing but I like it. Not sure however if we can make the code
> > > nicer. Sounds like strange workaround of virtual functions. But maybe I need
> > > more time to get agree on this :)
> > 
> > sure, I'm not sure if its workable or not either, but in some sense not
> > having virtual functions makes me think its cleaner, especially when there's
> > only one impl :)
> 
> yeah, but having abstract TextAttr (not templated) we can't stop virtual
> functions usage. But I admit it was interesting to read and that's something
> we(I) should keep in mind.

yeah, I'd sort of been thinking about a larger rethink of how this works.  but I think we can keep things the way they are for now.

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 601217 [details] [diff] [review]
fix

>+  IsDefined() const
>+{
>+  return mLine & (NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE |
>+    NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH);

implement it using IsLineThrough() and is IsUnderLine()?

>+  TextDecorTextAttr(nsIFrame* aRootFrame, nsIFrame* aFrame) :
>+  TextAttr<TextDecorValue>(!aFrame)

does const TextDecoreValue work?

>+  ExposeValue(nsIPersistentProperties* aAttributes, const TextDecorValue& aValue)
>+{
>+  if (aValue.IsUnderline()) {
>+    nsAutoString formattedStyle;
>+    StyleInfo::FormatTextDecorationStyle(aValue.Style(), formattedStyle);
>+    nsAccUtils::SetAccAttr(aAttributes,
>+                           nsGkAtoms::textUnderlineStyle,
>+                           formattedStyle);
>+
>+    nsAutoString formattedColor;
>+    StyleInfo::FormatColor(aValue.Color(), formattedColor);
>+    nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::textUnderlineColor,
>+                           formattedColor);
>+    return;
>+  }
>+
>+  if (aValue.IsLineThrough()) {
>+    nsAutoString formattedStyle;
>+    StyleInfo::FormatTextDecorationStyle(aValue.Style(), formattedStyle);
>+    nsAccUtils::SetAccAttr(aAttributes,
>+                           nsGkAtoms::textLineThroughStyle,
>+                           formattedStyle);
>+
>+    nsAutoString formattedColor;
>+    StyleInfo::FormatColor(aValue.Color(), formattedColor);
>+    nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::textLineThroughColor,
>+                           formattedColor);
>+  }

what about text that is both line through and under lined? kind of weir but legal no?

>+    TextDecorValue() { }

why do you have this? not having a frme will end badly no?

>+    TextDecorValue(nsIFrame* aFrame);
>+
>+    nscolor Color() const { return mColor; }
>+    PRUint8 Style() const { return mStyle; }
>+
>+    bool IsDefined() const;
>+    bool IsUnderline() const;
>+    bool IsLineThrough() const;
>+
>+    bool operator ==(const TextDecorValue& aValue)
>+    {
>+      return mColor == aValue.mColor && mLine == aValue.mLine &&
>+        mStyle == aValue.mStyle;
>+    }
>+    bool operator !=(const TextDecorValue& aValue)
>+      { return !(*this == aValue); }
>+
>+  private:
>+    nscolor mColor;
>+    PRUint8 mLine;
>+    PRUint8 mStyle;

since they aren't changed after construction you could make them public and const and drom the accessors.

Comment 10

[Trevor Saunders (:tbsaunde)]

> > yeah, but having abstract TextAttr (not templated) we can't stop virtual
> > functions usage. But I admit it was interesting to read and that's something
> > we(I) should keep in mind.
> 
> yeah, I'd sort of been thinking about a larger rethink of how this works. 
> but I think we can keep things the way they are for now.

actually, it occurs to me that I think we could make GetValueFor() and ExposeValue() non virtual now using this if we wanted to.

Comment 11

[alexander :surkov (:asurkov)]

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

> I'm not suggesting change the idea, just how the logic is layed out to
> something like this
> 
>       if (!mIsDefined && aIncludeDefAttrValue && mIsRootDefined)
>         ExposeValue(aAttributes, mRootnativeValue);
>       else if ((!aIncludeDefAttrValue && mNativeValue != mRootNativeValue) ||
>                 mIsDefined) {
>         ExposeValue(aAttributes, mNativeValue);
>       }

I'll tweak it

> yeah, no good idea off hand, but just for i = 0; i < kidCount; i++) { ... }
> isn't that bd so...

I'm not sure I follow, what is bd?

(In reply to Trevor Saunders (:tbsaunde) from comment #10)
> > > yeah, but having abstract TextAttr (not templated) we can't stop virtual
> > > functions usage. But I admit it was interesting to read and that's something
> > > we(I) should keep in mind.
> > 
> > yeah, I'd sort of been thinking about a larger rethink of how this works. 
> > but I think we can keep things the way they are for now.
> 
> actually, it occurs to me that I think we could make GetValueFor() and
> ExposeValue() non virtual now using this if we wanted to.

We get unusual syntax and get rid two virtual functions which should be nice but not sure how big the win is. If you like then I file follow up bug to investigate it.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: cleanup:v2

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 604598 [details] [diff] [review]
cleanup:v2

I saw another minor thing or two we could make better here, but we should really ix all this for 13 so lets go with what we have.

Comment 14

[Trevor Saunders (:tbsaunde)]

> > yeah, no good idea off hand, but just for i = 0; i < kidCount; i++) { ... }
> > isn't that bd so...
> 
> I'm not sure I follow, what is bd?

bad

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> Comment on attachment 604598 [details] [diff] [review]
> cleanup:v2
> 
> I saw another minor thing or two we could make better here, but we should
> really ix all this for 13 so lets go with what we have.

Nah, I don't think we can take all of them into Firefox 13 (3 days left). So you could stay nitpicky.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #15)
> (In reply to Trevor Saunders (:tbsaunde) from comment #13)
> > Comment on attachment 604598 [details] [diff] [review]
> > cleanup:v2
> > 
> > I saw another minor thing or two we could make better here, but we should
> > really ix all this for 13 so lets go with what we have.
> 
> Nah, I don't think we can take all of them into Firefox 13 (3 days left). So

well, I'd probably try especially since GetComputedStyle can cuse us greif, but of course up to you.

> you could stay nitpicky.

well, this still blocks a bunch of stuff in your queue right?  The only big thing I noticed tht should definitely be fixed but isn't really important since its a stack class is tht the members of TextAttrMgr are layed out pretty poorly alignment wise, we have sizeof(void*) object followed by  bool followed by sizeof(void*)-1 bytes of padding then sizeof(void*) object then sizeof(PRUint32) object.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #16)
> well, I'd probably try especially since GetComputedStyle can cuse us greif,
> but of course up to you.

well, it is but pushing everything last day is not what I'd like to do. changes are really big, crashes are not so often.

> > you could stay nitpicky.
> 
> well, this still blocks a bunch of stuff in your queue right?

yeah, these days my queue is about 10 patches.

>  The only big
> thing I noticed tht should definitely be fixed but isn't really important
> since its a stack class is tht the members of TextAttrMgr are layed out
> pretty poorly alignment wise, we have sizeof(void*) object followed by  bool
> followed by sizeof(void*)-1 bytes of padding then sizeof(void*) object then
> sizeof(PRUint32) object.

may I ask you to file good-first-bug for this?

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> Comment on attachment 601217 [details] [diff] [review]
> fix
> 
> >+  IsDefined() const
> >+{
> >+  return mLine & (NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE |
> >+    NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH);
> 
> implement it using IsLineThrough() and is IsUnderLine()?

if they were inline. so I didn't do that.

> >+  TextDecorTextAttr(nsIFrame* aRootFrame, nsIFrame* aFrame) :
> >+  TextAttr<TextDecorValue>(!aFrame)
> 
> does const TextDecoreValue work?

what's for? doing this I'd say to keep const T in base class instead.

> what about text that is both line through and under lined? kind of weir but
> legal no?

iirc, these styles are not inherited and provided by the same CSS style so no way to have them both

> >+    TextDecorValue() { }
> 
> why do you have this? not having a frme will end badly no?

probably I don't need it

> since they aren't changed after construction you could make them public and
> const and drom the accessors.

well, it's felt as a class so I wouldn't like to keep member variables public. Also keeping them a const you can't change them in constructor (you can initialize them but not assign a value)

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > Comment on attachment 601217 [details] [diff] [review]
> > fix
> > 
> > >+  IsDefined() const
> > >+{
> > >+  return mLine & (NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE |
> > >+    NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH);
> > 
> > implement it using IsLineThrough() and is IsUnderLine()?
> 
> if they were inline. so I didn't do that.

yeah, it might be reasonable to make them inline since the ttribute class uses them, but I'd sort of expect that if it actually makes sense the compiler is smart enough to just do this for you in the case the use and the method definition are in the same translation unit.  If you care its probably not that hard to see if that's actually true...

> > >+  TextDecorTextAttr(nsIFrame* aRootFrame, nsIFrame* aFrame) :
> > >+  TextAttr<TextDecorValue>(!aFrame)
> > 
> > does const TextDecoreValue work?
> 
> what's for? doing this I'd say to keep const T in base class instead.

yeah, it would end up being the same  for this class, but I don't think it'd work so never mind.

> > what about text that is both line through and under lined? kind of weir but
> > legal no?
> 
> iirc, these styles are not inherited and provided by the same CSS style so
> no way to have them both

ok

> > >+    TextDecorValue() { }
> > 
> > why do you have this? not having a frme will end badly no?
> 
> probably I don't need it
> 
> > since they aren't changed after construction you could make them public and
> > const and drom the accessors.
> 
> well, it's felt as a class so I wouldn't like to keep member variables
> public. Also keeping them a const you can't change them in constructor (you
> can initialize them but not assign a value)

I thought you you could assign to them once if you didn't provide an initializer, but ok.

Comment 20

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #17)
> (In reply to Trevor Saunders (:tbsaunde) from comment #16)
> > well, I'd probably try especially since GetComputedStyle can cuse us greif,
> > but of course up to you.
> 
> well, it is but pushing everything last day is not what I'd like to do.
> changes are really big, crashes are not so often.

true

> >  The only big
> > thing I noticed tht should definitely be fixed but isn't really important
> > since its a stack class is tht the members of TextAttrMgr are layed out
> > pretty poorly alignment wise, we have sizeof(void*) object followed by  bool
> > followed by sizeof(void*)-1 bytes of padding then sizeof(void*) object then
> > sizeof(PRUint32) object.
> 
> may I ask you to file good-first-bug for this?

bug 734566

Comment 21

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > > >+  IsDefined() const
> > > >+{
> > > >+  return mLine & (NS_STYLE_TEXT_DECORATION_LINE_UNDERLINE |
> > > >+    NS_STYLE_TEXT_DECORATION_LINE_LINE_THROUGH);
> > > 
> > > implement it using IsLineThrough() and is IsUnderLine()?
> > 
> > if they were inline. so I didn't do that.
> 
> yeah, it might be reasonable to make them inline since the ttribute class
> uses them, but I'd sort of expect that if it actually makes sense the
> compiler is smart enough to just do this for you in the case the use and the
> method definition are in the same translation unit.  If you care its
> probably not that hard to see if that's actually true...

well, since TextAttrs.h is included into cpp files then I think I can add nsStyleConsts.h dependency into TextAttrs.h

Comment 22

[alexander :surkov (:asurkov)]

cleanup:v2 landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3fc01c33bc

Comment 23

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> >+    TextDecorValue() { }
> 
> why do you have this? not having a frme will end badly no?

TTextAttr creates has an instance of T type, empty object is created.

Comment 24

[alexander :surkov (:asurkov)]

fix landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c29f828d1ce4

Comment 25

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/mozilla-central/rev/9e3fc01c33bc

Comment 26

[Daniel Holbert [:dholbert]]

and:
 https://hg.mozilla.org/mozilla-central/rev/c29f828d1ce4

Comment 27

[alexander :surkov (:asurkov)]

the doc here: https://developer.mozilla.org/en/Accessibility/AT-APIs/Gecko/TextAttrs