Closed Bug 728907 Opened 8 years ago Closed 8 years ago

don't use GetComputedStyle for text attributes

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(5 files, 1 obsolete file)

see bug 728127 for rationale.

will fix bug 445509
Attached patch patch1:color (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #599193 - Flags: review?(trev.saunders)
Attached patch patch1:color:v2Splinter Review
Attachment #599193 - Attachment is obsolete: true
Attachment #599218 - Flags: review?(trev.saunders)
Attachment #599193 - Flags: review?(trev.saunders)
Depends on: 473576
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.
Attachment #599218 - Flags: review?(trev.saunders) → review+
(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
> > 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?
(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
(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.
(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
(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.
(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?
(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.
(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
Attachment #600890 - Flags: review?(trev.saunders)
(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
Whiteboard: [don't mark fixed]
Attachment #600926 - Flags: review?(trev.saunders)
Attachment #600926 - Flags: review?(trev.saunders) → review+
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.
Attachment #600890 - Flags: review?(trev.saunders) → review+
(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
Attachment #600926 - Attachment description: patch2:header issue:v1 → patch3:header issue:v1
remove static_cast per Trevor's comment
Attachment #601160 - Flags: review?(trev.saunders)
Attachment #601160 - Flags: review?(trev.saunders) → review+
Depends on: 523304
Attachment #601253 - Flags: review?(trev.saunders)
Depends on: 473569
Whiteboard: [don't mark fixed]
(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
Whiteboard: [don't mark fixed]
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?
Attachment #601253 - Flags: review?(trev.saunders) → review+
(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 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/>
(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
https://hg.mozilla.org/mozilla-central/rev/f97ad1f32a98
https://hg.mozilla.org/mozilla-central/rev/85a6457b194e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.