Closed
Bug 475522
Opened 15 years ago
Closed 15 years ago
walk subtree once for all text attributes
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
80.97 KB,
patch
|
neil
:
superreview+
|
Details | Diff | Splinter Review |
3.79 KB,
patch
|
ginnchen+exoracle
:
review+
surkov
:
review+
|
Details | Diff | Splinter Review |
It's performance related bug, also see bug 445677, bug 453605
Assignee | ||
Updated•15 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 1•15 years ago
|
||
All tests that will be introduced in bug 475525 must pass.
Depends on: 475525
Assignee | ||
Comment 2•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #360277 -
Flags: review?(david.bolter)
Assignee | ||
Updated•15 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•15 years ago
|
||
- "background-color": "rgb(255, 255, 255)", we don't expose background-color text attribute any more if it has default value (bug 445511).
Updated•15 years ago
|
Attachment #360277 -
Flags: review?(marco.zehe) → review+
Comment 4•15 years ago
|
||
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 5•15 years ago
|
||
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?
Comment 6•15 years ago
|
||
I should have put a comma "template code, and some style"
Assignee | ||
Comment 7•15 years ago
|
||
(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.
Assignee | ||
Comment 8•15 years ago
|
||
(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.
Assignee | ||
Comment 9•15 years ago
|
||
(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).
Comment 10•15 years ago
|
||
Alex, where is the screen reader supposed to get the default value from if not from a11y APIs?
Assignee | ||
Comment 11•15 years ago
|
||
(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.
Comment 12•15 years ago
|
||
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?
Comment 13•15 years ago
|
||
(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.
Comment 14•15 years ago
|
||
(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 15•15 years ago
|
||
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)
Comment 16•15 years ago
|
||
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.
Comment 17•15 years ago
|
||
Heh, that was obviously meant for a different bug - bug 475006.
Assignee | ||
Comment 18•15 years ago
|
||
1) expose default background color 2) keep local text attributes array
Attachment #360277 -
Attachment is obsolete: true
Attachment #360654 -
Flags: review?(david.bolter)
Updated•15 years ago
|
Attachment #360654 -
Flags: review?(david.bolter) → review+
Assignee | ||
Updated•15 years ago
|
Attachment #360654 -
Flags: superreview?(neil)
Attachment #360654 -
Flags: review?(aaronleventhal)
Comment 19•15 years ago
|
||
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)
Assignee | ||
Comment 20•15 years ago
|
||
(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.
Updated•15 years ago
|
Attachment #360654 -
Flags: superreview-
Comment 21•15 years ago
|
||
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.
Assignee | ||
Comment 22•15 years ago
|
||
(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?
Comment 23•15 years ago
|
||
(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;
Assignee | ||
Comment 24•15 years ago
|
||
(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).
Assignee | ||
Comment 25•15 years ago
|
||
(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?
Assignee | ||
Comment 26•15 years ago
|
||
(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?
Comment 27•15 years ago
|
||
(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++
Assignee | ||
Comment 28•15 years ago
|
||
Attachment #360654 -
Attachment is obsolete: true
Attachment #361112 -
Flags: superreview?(neil)
Attachment #361112 -
Flags: review?(aaronleventhal)
Attachment #360654 -
Flags: review?(aaronleventhal)
Comment 29•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #361112 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 30•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4767c92771e6
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 31•15 years ago
|
||
Had to back this out because of build bustage.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 32•15 years ago
|
||
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 ago → 15 years ago
Resolution: --- → FIXED
Comment 33•15 years ago
|
||
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.
Assignee | ||
Comment 34•15 years ago
|
||
Ginn, any idea how to fix?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 35•15 years ago
|
||
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)
Assignee | ||
Comment 36•15 years ago
|
||
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 37•15 years ago
|
||
Comment on attachment 362552 [details] [diff] [review] Fix VC7.1 bustage It works!
Attachment #362552 -
Flags: review?(ginn.chen) → review+
Comment 38•15 years ago
|
||
(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!
Assignee | ||
Comment 39•15 years ago
|
||
(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?
Comment 40•15 years ago
|
||
I will as soon as I feel the tree is green enough.
Comment 41•15 years ago
|
||
Pushed changeset 6c630bbb4233 to mozilla-central.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•