Closed
Bug 523304
Opened 15 years ago
Closed 13 years ago
expose text-underline-color and text-line-through-color text attributes
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access, dev-doc-needed)
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•15 years ago
|
Summary: expose ext-underline-color and text-line-through-color text attributes → expose text-underline-color and text-line-through-color text attributes
Assignee | ||
Comment 1•15 years ago
|
||
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)
Assignee | ||
Comment 2•13 years ago
|
||
fit text attr interface and do cleanup
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #601214 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 3•13 years ago
|
||
expose attributes
Attachment #601217 -
Flags: review?(trev.saunders)
Comment 4•13 years ago
|
||
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/
Assignee | ||
Comment 5•13 years ago
|
||
(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•13 years ago
|
||
> 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 :)
Assignee | ||
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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.
Attachment #601217 -
Flags: review?(trev.saunders)
Comment 10•13 years ago
|
||
> > 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.
Assignee | ||
Comment 11•13 years ago
|
||
(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.
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #601214 -
Attachment is obsolete: true
Attachment #604598 -
Flags: review?(trev.saunders)
Attachment #601214 -
Flags: review?(trev.saunders)
Comment 13•13 years ago
|
||
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.
Attachment #604598 -
Flags: review?(trev.saunders) → review+
Comment 14•13 years ago
|
||
> > 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
Assignee | ||
Comment 15•13 years ago
|
||
(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•13 years ago
|
||
(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.
Assignee | ||
Comment 17•13 years ago
|
||
(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?
Assignee | ||
Comment 18•13 years ago
|
||
(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•13 years ago
|
||
(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.
Updated•13 years ago
|
Attachment #601217 -
Flags: review+
Comment 20•13 years ago
|
||
(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
Assignee | ||
Comment 21•13 years ago
|
||
(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
Assignee | ||
Comment 22•13 years ago
|
||
cleanup:v2 landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e3fc01c33bc
Whiteboard: [don't mark fixed]
Assignee | ||
Comment 23•13 years ago
|
||
(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.
Assignee | ||
Comment 24•13 years ago
|
||
Whiteboard: [don't mark fixed]
Comment 25•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Version: unspecified → Trunk
Comment 26•13 years ago
|
||
Assignee | ||
Comment 27•13 years ago
|
||
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•