728907 - don't use GetComputedStyle for text attributes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

see bug 728127 for rationale.

will fix bug 445509

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch1:color

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch1:color:v2

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 599218 [details] [diff] [review]
patch1:color:v2

>+StyleInfo::Format(const nscolor& aValue, nsAString& aFormattedValue)
>+{
>+  // Combine the string like rgb(R, G, B) from nscolor.
>+  nsAutoString value;

I'm not sure nsAutoString wins you anything here iether, but I'd be fine with this method taking a nsString& if that makes things easier.

btw I wonder if anyone outside a11y wants to do this computation.

>-// nsBackgroundTextAttr
>+// nsBGColorTextAttr
> ////////////////////////////////////////////////////////////////////////////////
> 
> nsBGColorTextAttr::nsBGColorTextAttr(nsIFrame *aRootFrame, nsIFrame *aFrame) :

I wonder if you could merge this and the color one somehow? and if it would end up being reasonable of course.

>+  nsTextAttr<nscolor>(aFrame == nsnull)

!!aFrame?

>+ * Class is used for the work with 'color' text attribute in nsTextAttrsMgr
>+ * class.
>+ */
>+class nsColorTextAttr : public nsTextAttr<nscolor>

declare it in nsTextAttrs.cpp? I don't' see any reason to expose more than the manager to consumers really.

Comment 4

[alexander :surkov (:asurkov)]

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

> btw I wonder if anyone outside a11y wants to do this computation.

I'm not aware of this, iirc computed style uses different pattern

> I wonder if you could merge this and the color one somehow? and if it would
> end up being reasonable of course.

I wouldn't do that until they are really common (background color differs from color to handle transparent value) to keep text attributes atomic.

> >+class nsColorTextAttr : public nsTextAttr<nscolor>
> 
> declare it in nsTextAttrs.cpp? I don't' see any reason to expose more than
> the manager to consumers really.

this makes sense, still not sure if I like to deal with it here to not introduce more mess than we have now

Comment 5

[Trevor Saunders (:tbsaunde)]

> > I wonder if you could merge this and the color one somehow? and if it would
> > end up being reasonable of course.
> 
> I wouldn't do that until they are really common (background color differs
> from color to handle transparent value) to keep text attributes atomic.

ok, though I'm not sure what you mean by atomic.

> > >+class nsColorTextAttr : public nsTextAttr<nscolor>
> > 
> > declare it in nsTextAttrs.cpp? I don't' see any reason to expose more than
> > the manager to consumers really.
> 
> this makes sense, still not sure if I like to deal with it here to not
> introduce more mess than we have now

ok, how does a spin off bug to move all of these classes out of the header sound?

Comment 6

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > > I wonder if you could merge this and the color one somehow? and if it would
> > > end up being reasonable of course.
> > 
> > I wouldn't do that until they are really common (background color differs
> > from color to handle transparent value) to keep text attributes atomic.
> 
> ok, though I'm not sure what you mean by atomic.

small class used to calculate specific text attributes, what can be shared between different text attributes if they have the same logic.

> > > >+class nsColorTextAttr : public nsTextAttr<nscolor>
> > > 
> > > declare it in nsTextAttrs.cpp? I don't' see any reason to expose more than
> > > the manager to consumers really.
> > 
> > this makes sense, still not sure if I like to deal with it here to not
> > introduce more mess than we have now
> 
> ok, how does a spin off bug to move all of these classes out of the header
> sound?

sounds like a good first bug

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 599218 [details] [diff] [review]
> patch1:color:v2
> 
> >+StyleInfo::Format(const nscolor& aValue, nsAString& aFormattedValue)
> >+{
> >+  // Combine the string like rgb(R, G, B) from nscolor.
> >+  nsAutoString value;
> 
> I'm not sure nsAutoString wins you anything here iether, but I'd be fine
> with this method taking a nsString& if that makes things easier.

that makes sense, though requires me to use nsAutoString in nsBGColorTextAttr::Format. Ideally we need to fix nsITreeAttr interface.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #6)

> > ok, how does a spin off bug to move all of these classes out of the header
> > sound?
> 
> sounds like a good first bug

btw, iirc vc doesn't allow to keep template classes in source files

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #6)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > > > I wonder if you could merge this and the color one somehow? and if it would
> > > > end up being reasonable of course.
> > > 
> > > I wouldn't do that until they are really common (background color differs
> > > from color to handle transparent value) to keep text attributes atomic.
> > 
> > ok, though I'm not sure what you mean by atomic.
> 
> small class used to calculate specific text attributes, what can be shared
> between different text attributes if they have the same logic.
> 
> > > > >+class nsColorTextAttr : public nsTextAttr<nscolor>
> > > > 
> > > > declare it in nsTextAttrs.cpp? I don't' see any reason to expose more than
> > > > the manager to consumers really.
> > > 
> > > this makes sense, still not sure if I like to deal with it here to not
> > > introduce more mess than we have now
> > 
> > ok, how does a spin off bug to move all of these classes out of the header
> > sound?
> 
> sounds like a good first bug

sounds about right.

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > Comment on attachment 599218 [details] [diff] [review]
> > patch1:color:v2
> > 
> > >+StyleInfo::Format(const nscolor& aValue, nsAString& aFormattedValue)
> > >+{
> > >+  // Combine the string like rgb(R, G, B) from nscolor.
> > >+  nsAutoString value;
> > 
> > I'm not sure nsAutoString wins you anything here iether, but I'd be fine
> > with this method taking a nsString& if that makes things easier.
> 
> that makes sense, though requires me to use nsAutoString in
> nsBGColorTextAttr::Format. Ideally we need to fix nsITreeAttr interface.

that's probably fine, the worst thing that can happen is some  unneeded copies.  fixing the interface seems like the real solution.

(In reply to alexander :surkov from comment #8)
> (In reply to alexander :surkov from comment #6)
> 
> > > ok, how does a spin off bug to move all of these classes out of the header
> > > sound?
> > 
> > sounds like a good first bug
> 
> btw, iirc vc doesn't allow to keep template classes in source files

that would be :( but I have a hard time understanding how that could be since there isn't really any difference other than a header presumably isn't the file the compiler was invoked on.

Comment 10

[alexander :surkov (:asurkov)]

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

> > btw, iirc vc doesn't allow to keep template classes in source files
> 
> that would be :( but I have a hard time understanding how that could be
> since there isn't really any difference other than a header presumably isn't
> the file the compiler was invoked on.

as alternative we can move everything into private section of nsTextAttrsMgr, hidden from others, clear hg history and cpp doesn't get bloat. Sounds ok?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > > btw, iirc vc doesn't allow to keep template classes in source files
> > 
> > that would be :( but I have a hard time understanding how that could be
> > since there isn't really any difference other than a header presumably isn't
> > the file the compiler was invoked on.
> 
> as alternative we can move everything into private section of
> nsTextAttrsMgr, hidden from others, clear hg history and cpp doesn't get
> bloat. Sounds ok?

then we'll have things like nsTextMgr::nsCSSTextAttr::nsCSSTextAttr(nsIFoo* aFoo) { } no?

if not I gues this doesn't hurt much, but it doesn't acomplish my main goal of less for the compiler to look at each time it compiles the header.  On the other hand we could probably easily stop including nsTextAttrs.h in nsHyperTextAccessible.h and then it would only be included twice so it wouldn't matter much.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> > as alternative we can move everything into private section of
> > nsTextAttrsMgr, hidden from others, clear hg history and cpp doesn't get
> > bloat. Sounds ok?
> 
> then we'll have things like nsTextMgr::nsCSSTextAttr::nsCSSTextAttr(nsIFoo*
> aFoo) { } no?

yes but that shouldn't hurt much, right?

> if not I gues this doesn't hurt much, but it doesn't acomplish my main goal
> of less for the compiler to look at each time it compiles the header.  On
> the other hand we could probably easily stop including nsTextAttrs.h in
> nsHyperTextAccessible.h and then it would only be included twice so it
> wouldn't matter much.

I believe it's included into nsHyperTextAccessible.h by mistake

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch2:fontStyle:v1

Comment 14

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #2)
> Created attachment 599218 [details] [diff] [review]
> patch1:color:v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9640938c826

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch3:header issue:v1

Comment 16

[Trevor Saunders (:tbsaunde)]

Comment on attachment 600890 [details] [diff] [review]
patch2:fontStyle:v1

>+  // "font-style" text attribute
>+  FontStyleTextAttr fontStyleTextAttr(rootFrame, frame);
>+  textAttrArray.AppendElement(static_cast<nsITextAttr*>(&fontStyleTextAttr));

is the cast really needed? I'd expect not accept on old anoying compilers that we might well not care about anymore.

btw all of the constructors and GetValueFor()s feel very similar, it would be nice if we didn't need to duplicate that logic.

Comment 17

[alexander :surkov (:asurkov)]

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

> btw all of the constructors and GetValueFor()s feel very similar, it would
> be nice if we didn't need to duplicate that logic.

mostly they are similar but some classes do different things like nsLangTextAttr

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch4:static_cast:v1

remove static_cast per Trevor's comment

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch5:replace nsTArray:v1

Comment 20

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/d9640938c826

Comment 21

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #13)
> Created attachment 600890 [details] [diff] [review]
> patch2:fontStyle:v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/bff447521a33

Comment 22

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #15)
> Created attachment 600926 [details] [diff] [review]
> patch3:header issue:v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c68488743d

Comment 23

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #18)
> Created attachment 601160 [details] [diff] [review]
> patch4:static_cast:v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/91c35e2c18da

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/bff447521a33

Comment 25

[Matt Brubeck (:mbrubeck)]

and https://hg.mozilla.org/mozilla-central/rev/d7c68488743d
and https://hg.mozilla.org/mozilla-central/rev/91c35e2c18da

Comment 26

[Trevor Saunders (:tbsaunde)]

Comment on attachment 601253 [details] [diff] [review]
patch5:replace nsTArray:v1

>diff --git a/accessible/src/base/TextAttrs.cpp b/accessible/src/base/TextAttrs.cpp
>--- a/accessible/src/base/TextAttrs.cpp
>+++ b/accessible/src/base/TextAttrs.cpp
>@@ -143,90 +143,88 @@ TextAttrsMgr::GetAttributes(nsIPersisten
>   nsIContent *offsetNode = nsnull, *offsetElm = nsnull;
>   nsIFrame *frame = nsnull;
>   if (mOffsetAcc) {
>     offsetNode = mOffsetAcc->GetContent();
>     offsetElm = nsCoreUtils::GetDOMElementFor(offsetNode);
>     frame = offsetElm->GetPrimaryFrame();
>   }
> 
>-  nsTArray<ITextAttr*> textAttrArray(9);
>-
>   // "language" text attribute
>   LangTextAttr langTextAttr(mHyperTextAcc, hyperTextElm, offsetNode);
>-  textAttrArray.AppendElement(&langTextAttr);
> 
>   // "text-position" text attribute
>-  CSSTextAttr posTextAttr(0, hyperTextElm, offsetElm);
>-  textAttrArray.AppendElement(&posTextAttr);
>+  CSSTextAttr cssTextAttr(0, hyperTextElm, offsetElm);
> 
>   // "background-color" text attribute
>   BGColorTextAttr bgColorTextAttr(rootFrame, frame);
>-  textAttrArray.AppendElement(&bgColorTextAttr);
> 
>   // "color" text attribute
>   ColorTextAttr colorTextAttr(rootFrame, frame);
>-  textAttrArray.AppendElement(&colorTextAttr);
> 
>   // "font-family" text attribute
>   FontFamilyTextAttr fontFamilyTextAttr(rootFrame, frame);
>-  textAttrArray.AppendElement(&fontFamilyTextAttr);
> 
>   // "font-size" text attribute
>   FontSizeTextAttr fontSizeTextAttr(rootFrame, frame);
>-  textAttrArray.AppendElement(&fontSizeTextAttr);
> 
>   // "font-style" text attribute
>   FontStyleTextAttr fontStyleTextAttr(rootFrame, frame);
>-  textAttrArray.AppendElement(&fontStyleTextAttr);
> 
>   // "font-weight" text attribute
>   FontWeightTextAttr fontWeightTextAttr(rootFrame, frame);
>-  textAttrArray.AppendElement(&fontWeightTextAttr);
> 
>   // "text-underline(line-through)-style(color)" text attributes
>   TextDecorTextAttr textDecorTextAttr(rootFrame, frame);
>-  textAttrArray.AppendElement(&textDecorTextAttr);
>+
>+  ITextAttr* attrArray[] =
>+  {
>+    &langTextAttr,
>+    &cssTextAttr,
>+    &bgColorTextAttr,
>+    &colorTextAttr,
>+    &fontFamilyTextAttr,
>+    &fontSizeTextAttr,
>+    &fontStyleTextAttr,
>+    &fontWeightTextAttr,
>+    &>+  PRUint32 attrCount = sizeof(attrArray) / sizeof(ITextAttr*);

I thought there was a macro / template function for that?

>     bool offsetFound = false;
>-    for (PRUint32 attrIdx = 0; attrIdx < attrLen; attrIdx++) {
>-      ITextAttr* textAttr = aTextAttrArray[attrIdx];
>+    for (PRUint32 attrIdx = 0; attrIdx < aAttrArrayLen; attrIdx++) {

why not just idx?

Comment 27

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #26)
> >+    &>+  PRUint32 attrCount = sizeof(attrArray) / sizeof(ITextAttr*);
> 
> I thought there was a macro / template function for that?

fixed

> >     bool offsetFound = false;
> >-    for (PRUint32 attrIdx = 0; attrIdx < attrLen; attrIdx++) {
> >-      ITextAttr* textAttr = aTextAttrArray[attrIdx];
> >+    for (PRUint32 attrIdx = 0; attrIdx < aAttrArrayLen; attrIdx++) {
> 
> why not just idx?

because nicer? :)

Comment 28

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f97ad1f32a98

Comment 29

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 601253 [details] [diff] [review]
patch5:replace nsTArray:v1

This should use ArrayLength and the template trick for GetRange; see <http://whereswalden.com/2011/10/20/implementing-mozillaarraylength-and-mozillaarrayend-and-some-followup-work/>

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Ms2ger from comment #29)
> Comment on attachment 601253 [details] [diff] [review]
> patch5:replace nsTArray:v1
> 
> This should use ArrayLength and the template trick for GetRange; see
> <http://whereswalden.com/2011/10/20/implementing-mozillaarraylength-and-
> mozillaarrayend-and-some-followup-work/>

https://hg.mozilla.org/integration/mozilla-inbound/rev/85a6457b194e

Comment 31

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/f97ad1f32a98
https://hg.mozilla.org/mozilla-central/rev/85a6457b194e