Closed Bug 475522 Opened 15 years ago Closed 15 years ago

walk subtree once for all text attributes

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

It's performance related bug, also see bug 445677, bug 453605
OS: Mac OS X → All
Hardware: x86 → All
All tests that will be introduced in bug 475525 must pass.
Depends on: 475525
Attached patch patch (obsolete) — Splinter Review
1) tests from bug 475525 are added
2) it makes faster text attributes calculation in 2 times almost (testcase from bug 453605) because we walk subtree once only
3) all text attribute code is enclosed in nsTextAttrs files
4) new code organization should allow developer to add support of new attributes more easy - all he need is to write small class like nsBGColorTextAttr and add it to nsTextAttrsMgr class.
5) I didn't introduce any interface changes at this point
Attachment #360277 - Flags: review?(marco.zehe)
Attachment #360277 - Flags: review?(david.bolter)
Status: NEW → ASSIGNED
- "background-color": "rgb(255, 255, 255)",

we don't expose background-color text attribute any more if it has default value (bug 445511).
Attachment #360277 - Flags: review?(marco.zehe) → review+
Comment on attachment 360277 [details] [diff] [review]
patch

r=me for the test part. The other part looks OK, too. But what's this about the default background color? Why was this removed, or is this a problem you still need to address?
Comment on attachment 360277 [details] [diff] [review]
patch

You are keeping me busy -- which is great!

I need some help understanding the patch:

>+  // "color" text attribute
>+  nsCSSTextAttr colorTextAttr(0, hyperTextElm, offsetElm);
>+  mTextAttrArray.AppendElement(&colorTextAttr);
>+
>+  // "font-family" text attribute
>+  nsCSSTextAttr fontFamilyTextAttr(1, hyperTextElm, offsetElm);
>+  mTextAttrArray.AppendElement(&fontFamilyTextAttr);
>+
>+  // "font-style" text attribute
>+  nsCSSTextAttr fontStyleTextAttr(2, hyperTextElm, offsetElm);
>+  mTextAttrArray.AppendElement(&fontStyleTextAttr);

I think the nsCSSTextAttr constructor expects a PRBool as the first argument. I'm not sure how you are using the numbers here.


>+
>+  nsresult rv = NS_OK;
>+
>+  // Expose text attributes range where they are applied if applicable.
>+  if (mOffsetNode)
>+    rv = GetRange(aStartHTOffset, aEndHTOffset);
>+
>+  mTextAttrArray.Clear();

Why are you prefixing variable mTextAttrArray with m, if it is temporary? Maybe you could change the name and pass it into the methods where it is used (GetRange)?

>-
>-PRBool
>-nsFontSizeTextAttr::Get(nsAString& aValue)
>+void
>+nsFontSizeTextAttr::Format(const nscoord& aValue, nsAString& aFormattedValue)

Nice.

>+
>+/**
>+ * 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.
>+ *
>+ * @note "invalid: spelling" text attrbiute is implemented entirerly in
>+ *       nsHyperTextAccessible class.
>+ */
>+class nsTextAttrsMgr

Is this named this way to indicate this is a manager type of class?

                         PRInt32 *aEndHTOffset = nsnull);
>+
>+protected:
>+
>+  /**
>+   * Calculates range (start and end offsets) of text where the text attributes
>+   * are stretched. New offsets may be smaller if one of text attributes changes

What does 'stretched' mean here?

Great stuff overall... I saw the template code and some style things geared towards performance. How is the performance BTW?
I should have put a comma "template code, and some style"
(In reply to comment #4)
> (From update of attachment 360277 [details] [diff] [review])
> r=me for the test part. The other part looks OK, too. But what's this about the
> default background color? Why was this removed, or is this a problem you still
> need to address?

It was intentional (see comment #3), it's just demo how we can fix the bug 445511 - mIsRootDefined (member of nsTextAttr class) should be PR_FALSE.
(In reply to comment #5)
> (From update of attachment 360277 [details] [diff] [review])
> You are keeping me busy -- which is great!

I'm trying :)

> >+  // "font-style" text attribute
> >+  nsCSSTextAttr fontStyleTextAttr(2, hyperTextElm, offsetElm);
> >+  mTextAttrArray.AppendElement(&fontStyleTextAttr);
> 
> I think the nsCSSTextAttr constructor expects a PRBool as the first argument.
> I'm not sure how you are using the numbers here.

No, nsCSSTextAttr was changed, now it expects the index in gCSSTextAttrsMap as first argument and it works with one CSS based text attribute only. Previously nsCSSTextAttr worked with all CSS based text attributes. I did this change to create an array of text attr objects which allows to work with these text attr objects in the same way (because they expose the same nsITextAttr interface).

> >+
> >+  nsresult rv = NS_OK;
> >+
> >+  // Expose text attributes range where they are applied if applicable.
> >+  if (mOffsetNode)
> >+    rv = GetRange(aStartHTOffset, aEndHTOffset);
> >+
> >+  mTextAttrArray.Clear();
> 
> Why are you prefixing variable mTextAttrArray with m, if it is temporary? Maybe
> you could change the name and pass it into the methods where it is used
> (GetRange)?

'm' because it's member but you're right it's used locally only. If you prefer then I can create it locally (in GetAttributes method) and pass to all methods called from GetAttributes(). Do you?

> >-PRBool
> >-nsFontSizeTextAttr::Get(nsAString& aValue)
> >+void
> >+nsFontSizeTextAttr::Format(const nscoord& aValue, nsAString& aFormattedValue)
> 
> Nice.

Yes, this is advantage of this approach I believe - it allows to keep text attr classes simple.

> >+class nsTextAttrsMgr
> 
> Is this named this way to indicate this is a manager type of class?

yes. Initially I thought to call it nsTextAttrs but this name if very similar to nsTextAtrr.

> >+  /**
> >+   * Calculates range (start and end offsets) of text where the text attributes
> >+   * are stretched. New offsets may be smaller if one of text attributes changes
> 
> What does 'stretched' mean here?

it's copy/paste from nsHyperTextAccessible, "stretched" means do not change their value. For example
<div><span lang="en">hello</span><span lang="de">Guten Tag</span></div>. Here "language: en" text attribute is stretched from 0 to 6, "language: de" text attribute is stretched from 6 to the end.

> Great stuff overall... I saw the template code and some style things geared
> towards performance. How is the performance BTW?

there were two words in comment #2. More details: I tried to load "War and Piece" by Lev Tolstoy (example from bug 453605), but my bad inet connection didn't allow to load this work of literature entirely (though possibly it's not so bad, iirc I spent few months to read it in school ;)). So I tested only the part I loaded. I called getTextAttributes(0, offset1, offset2) on document accessible, the time was 2 sec, when I applied the patch time was 1.2 sec.

Sure this performance win is good but it's not enough to say bug 453605 is fixed. But I tend to consider this patch not only as performance related bug, I tend to consider as code organization improvement of text attributes support.
(In reply to comment #6)
> I should have put a comma "template code, and some style"

I didn't find anything better than to introduce templates. One thing I 100% wanted is every text attribute object shouldn't implement logic how to expose and compare text attr values because it's easy to add wrong code when you introduce new text attribute or change the old one (like it was for background-color and font-size text attributes).
Alex, where is the screen reader supposed to get the default value from if not from a11y APIs?
(In reply to comment #10)
> Alex, where is the screen reader supposed to get the default value from if not
> from a11y APIs?

I don't know, it's possibly they don't need default value. Btw, getDefaultTextAttributes is used in ATK only. IA2 hasn't way to get pure default attribute values.

Marco, ok, I can easy get back to old behaviour and save this problem for bug 445511 if you like.
We should ask Aaron, but my inclination is to say "yes, please do". I don't know if this is thought through to the end to say "well just use what you think the default value is". With different themings etc., this could be *anything*. So I would say we should provide the default color we get from Layout for the most accurate results.

Aaron, was there any specific explanation given by AT vendors as to where they think they will get the default background color value from?
(In reply to comment #8)
> (In reply to comment #5)
> > (From update of attachment 360277 [details] [diff] [review] [details])

Thanks Surkov:

> > >+  mTextAttrArray.Clear();
> > 
> > Why are you prefixing variable mTextAttrArray with m, if it is temporary? Maybe
> > you could change the name and pass it into the methods where it is used
> > (GetRange)?
> 
> 'm' because it's member but you're right it's used locally only. If you prefer
> then I can create it locally (in GetAttributes method) and pass to all methods
> called from GetAttributes(). Do you?

I would prefer it created locally and passed :)

> Sure this performance win is good but it's not enough to say bug 453605 is
> fixed. But I tend to consider this patch not only as performance related bug, I
> tend to consider as code organization improvement of text attributes support.

Agreed. This work is good in both respects.
(In reply to comment #9)
> (In reply to comment #6)
> > I should have put a comma "template code, and some style"
> 
> I didn't find anything better than to introduce templates. One thing I 100%
> wanted is every text attribute object shouldn't implement logic how to expose
> and compare text attr values because it's easy to add wrong code when you
> introduce new text attribute or change the old one (like it was for
> background-color and font-size text attributes).

Exellent.
Comment on attachment 360277 [details] [diff] [review]
patch

Cancelled review. I can give the new patch a once over :)
Attachment #360277 - Flags: review?(david.bolter)
It's really silly to try to optimize this. How many nodes are there that even have a role attribute at all? A binary search is definitely fast enough.
Heh, that was obviously meant for a different bug - bug 475006.
Attached patch patch2 (obsolete) — Splinter Review
1) expose default background color
2) keep local text attributes array
Attachment #360277 - Attachment is obsolete: true
Attachment #360654 - Flags: review?(david.bolter)
Attachment #360654 - Flags: review?(david.bolter) → review+
Attachment #360654 - Flags: superreview?(neil)
Attachment #360654 - Flags: review?(aaronleventhal)
Comment on attachment 360654 [details] [diff] [review]
patch2

I looked twice but couldn't find anything worth commenting on. Did you have anything in mind?
Attachment #360654 - Flags: superreview?(neil)
(In reply to comment #19)
> (From update of attachment 360654 [details] [diff] [review])
> I looked twice but couldn't find anything worth commenting on. Did you have
> anything in mind?

Mainly I kept in mind template usage.
Attachment #360654 - Flags: superreview-
Comment on attachment 360654 [details] [diff] [review]
patch2

>+    T& nativeValue = mNativeValue;
This doesn't do what you think. It just means that "nativeValue" is another name for "mNativeValue". In particular, assigning to it changes mNativeValue.

>+    if (mIsDefined)
>+      return mNativeValue == mRootNativeValue;
How does comparing your values tell you that you are equal to something else?

>+  // Return native value for the given DOM element.
>+  virtual PRBool GetValueFor(nsIDOMElement *aElm, T *aValue) = 0;
>+
>+  // Format native value to text attribute value.
>+  virtual void Format(const T& aValue, nsAString& aFormattedValue) = 0;
I'm not sure making these virtual is ideal, but I don't know enough about specialisation of template classes to know if there is a better way.
(In reply to comment #21)
> (From update of attachment 360654 [details] [diff] [review])
> >+    T& nativeValue = mNativeValue;
> This doesn't do what you think. It just means that "nativeValue" is another
> name for "mNativeValue". In particular, assigning to it changes mNativeValue.

Here I wanted to have another name for mNativeValue but I didn't want to change mNativeValue. So if I have
T& nativeValue = mNativeValue;
nativeValue = mRootNativeValue;

then does it mean mNativeValue == mRootNativeValue or does it mean nativeValue is another name for mRootNativeValue?
(In reply to comment #22)
>(In reply to comment #21)
>>(From update of attachment 360654 [details] [diff] [review])
>>>+    T& nativeValue = mNativeValue;
>>This doesn't do what you think. It just means that "nativeValue" is another
>>name for "mNativeValue". In particular, assigning to it changes mNativeValue.
>Here I wanted to have another name for mNativeValue but I didn't want to change
>mNativeValue. So if I have
>T& nativeValue = mNativeValue;
>nativeValue = mRootNativeValue;
>then does it mean mNativeValue == mRootNativeValue or does it mean nativeValue
>is another name for mRootNativeValue?
It means the same as mNativeValue = mRootNativeValue;
(In reply to comment #21)

> >+    if (mIsDefined)
> >+      return mNativeValue == mRootNativeValue;
> How does comparing your values tell you that you are equal to something else?

If the value is undefined then it is defined by root value. Here the given
value is undefined, original value is defined. Therefore given value is equal
to original when original value is equal to root value (because given value is
defined by root value).
(In reply to comment #23)
> (In reply to comment #22)
> >(In reply to comment #21)
> >>(From update of attachment 360654 [details] [diff] [review])
> >>>+    T& nativeValue = mNativeValue;
> >>This doesn't do what you think. It just means that "nativeValue" is another
> >>name for "mNativeValue". In particular, assigning to it changes mNativeValue.
> >Here I wanted to have another name for mNativeValue but I didn't want to change
> >mNativeValue. So if I have
> >T& nativeValue = mNativeValue;
> >nativeValue = mRootNativeValue;
> >then does it mean mNativeValue == mRootNativeValue or does it mean nativeValue
> >is another name for mRootNativeValue?
> It means the same as mNativeValue = mRootNativeValue;

Ok, I wanted to avoid copy of native values. So does it work if I'll use pointers instead of references?
(In reply to comment #21)

> >+  // Format native value to text attribute value.
> >+  virtual void Format(const T& aValue, nsAString& aFormattedValue) = 0;
> I'm not sure making these virtual is ideal, but I don't know enough about
> specialisation of template classes to know if there is a better way.

Sorry, do you mean you need additional details about nsTextAttr classes?
(In reply to comment #25)
> Ok, I wanted to avoid copy of native values. So does it work if I'll use
> pointers instead of references?
Pointers would work, yes.

(In reply to comment #26)
> (In reply to comment #21)
> > >+  // Format native value to text attribute value.
> > >+  virtual void Format(const T& aValue, nsAString& aFormattedValue) = 0;
> > I'm not sure making these virtual is ideal, but I don't know enough about
> > specialisation of template classes to know if there is a better way.
> Sorry, do you mean you need additional details about nsTextAttr classes?
No, I mean I need to learn C++
Attached patch patch3Splinter Review
Attachment #360654 - Attachment is obsolete: true
Attachment #361112 - Flags: superreview?(neil)
Attachment #361112 - Flags: review?(aaronleventhal)
Attachment #360654 - Flags: review?(aaronleventhal)
Comment on attachment 361112 [details] [diff] [review]
patch3

I think I have to stop doing large reviews like this right now, at least during my job search. It's just too much.
Attachment #361112 - Flags: review?(aaronleventhal)
Attachment #361112 - Flags: superreview?(neil) → superreview+
http://hg.mozilla.org/mozilla-central/rev/4767c92771e6
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Had to back this out because of build bustage.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed on Alexander Surkov's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/bd35ca5e194a
Same patch as attached here, but with proper templating as Linux requires.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Failed to compile with Sun Studio compiler on Solaris:

"../../../../accessible/src/base/nsTextAttrs.cpp", line 403: Error: nsTextAttr may not have a type qualifier.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 404: Error: nsTextAttr<T> cannot be initialized in a constructor.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 404: Error: Could not find nsTextAttr<nsAutoString>::nsTextAttr() to initialize base class.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 441: Error: nsTextAttr may not have a type qualifier.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 441: Error: nsTextAttr<T> cannot be initialized in a constructor.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 442: Error: Could not find nsTextAttr<nsAutoString>::nsTextAttr() to initialize base class.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 491: Error: nsTextAttr may not have a type qualifier.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 491: Error: nsTextAttr<T> cannot be initialized in a constructor.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 492: Error: Could not find nsTextAttr<unsigned>::nsTextAttr() to initialize base class.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 553: Error: nsTextAttr may not have a type qualifier.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 554: Error: nsTextAttr<T> cannot be initialized in a constructor.
"../../../../accessible/src/base/nsTextAttrs.cpp", line 554: Error: Could not find nsTextAttr<int>::nsTextAttr() to initialize base class.
12 Error(s) detected.
Ginn, any idea how to fix?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As VC7.1 complains about the AppendElement calls too for some reason, I'm asking for surkov's review too to ensure that my fix there is acceptable.
Attachment #362552 - Flags: review?(ginn.chen)
Comment on attachment 362552 [details] [diff] [review]
Fix VC7.1 bustage

r=me for code logic, thanks for the patch, Neil.
Attachment #362552 - Flags: review+
Comment on attachment 362552 [details] [diff] [review]
Fix VC7.1 bustage

It works!
Attachment #362552 - Flags: review?(ginn.chen) → review+
(In reply to comment #35)
> As VC7.1 complains about the AppendElement calls too for some reason, I'm
> asking for surkov's review too to ensure that my fix there is acceptable.
At least, I meant to, but I mistyped his email address :-( Thanks anyway!
(In reply to comment #38)
> (In reply to comment #35)
> > As VC7.1 complains about the AppendElement calls too for some reason, I'm
> > asking for surkov's review too to ensure that my fix there is acceptable.
> At least, I meant to, but I mistyped his email address :-( Thanks anyway!

Usually I read comments to bugs I am assigned to. So I didn't miss your review request :) Btw, are going to checkin the patch or should someone of us do this?
I will as soon as I feel the tree is green enough.
Pushed changeset 6c630bbb4233 to mozilla-central.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: