Last Comment Bug 734566 - optimize layout of TextAttrsMgr
: optimize layout of TextAttrsMgr
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86_64 All
: -- normal (vote)
: mozilla14
Assigned To: Michał Frontczak [:fxa90id] ♥
:
Mentors:
Depends on: 523304
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2012-03-09 22:32 PST by Trevor Saunders (:tbsaunde)
Modified: 2012-04-05 10:45 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
switched (1006 bytes, patch)
2012-03-18 10:48 PDT, Michał Frontczak [:fxa90id] ♥
tbsaunde+mozbugs: review-
Details | Diff | Splinter Review
changes (11.23 KB, patch)
2012-03-29 14:37 PDT, Michał Frontczak [:fxa90id] ♥
no flags Details | Diff | Splinter Review
new (792 bytes, patch)
2012-04-03 09:01 PDT, Michał Frontczak [:fxa90id] ♥
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-03-09 22:32:27 PST
currently we have members in this order
foo*
bool
bar*
PRInt32

Since this is stack allocated class no big deal, but kind of silly, the layout should be

foo*
bar*
PRInt32
bool
Comment 1 Michał Frontczak [:fxa90id] ♥ 2012-03-18 10:48:16 PDT
Created attachment 606986 [details] [diff] [review]
switched
Comment 2 alexander :surkov 2012-03-20 00:24:42 PDT
(In reply to Michał Frontczak :fxa90id from comment #1)
> Created attachment 606986 [details] [diff] [review]
> switched

nah, you should change the ordering of members
Comment 3 alexander :surkov 2012-03-20 00:25:33 PDT
also, please make sure to ask somebody for feedback (or review), you can use mentor for that when you push the patch
Comment 4 Michał Frontczak [:fxa90id] ♥ 2012-03-20 15:41:36 PDT
ok thx for response
Comment 5 Trevor Saunders (:tbsaunde) 2012-03-22 17:34:55 PDT
Comment on attachment 606986 [details] [diff] [review]
switched

>diff --git a/accessible/src/base/TextAttrs.h b/accessible/src/base/TextAttrs.h
>--- a/accessible/src/base/TextAttrs.h
>+++ b/accessible/src/base/TextAttrs.h
>@@ -76,15 +76,15 @@ public:
>    *                          text attributes
>    * @param oOffsetAcc       [optional] offset an accessible the text attributes
>    *                          should be calculated for
>    * @param oOffsetAccIdx    [optional] index in parent of offset accessible
>    */
>   TextAttrsMgr(nsHyperTextAccessible* aHyperTextAcc,
>-               bool aIncludeDefAttrs,
>                nsAccessible* aOffsetAcc,
>-               PRInt32 aOffsetAccIdx) :
>+               PRInt32 aOffsetAccIdx,
>+               bool aIncludeDefAttrs) :

um, that isn't the members, comment 2 wasn't addressed.
Comment 6 Michał Frontczak [:fxa90id] ♥ 2012-03-29 14:37:29 PDT
Created attachment 610698 [details] [diff] [review]
changes

I switched all members except variables
Comment 7 Trevor Saunders (:tbsaunde) 2012-03-29 15:16:27 PDT
Comment on attachment 610698 [details] [diff] [review]
changes

this appears to be a huge number of uneeded whitespace changes to some ancient version of the file, PRBool was killed 6 months ago nsTPtrArray before that and the file was renamed a couple weeks ago.  please resubmit the patch based off a recent revision.
Comment 8 alexander :surkov 2012-03-29 20:51:34 PDT
That's right. It seems you do a diff against some old Firefox code. Not sure how it happened please let us know if you need a help.
Comment 9 Michał Frontczak [:fxa90id] ♥ 2012-03-29 21:27:00 PDT
(In reply to alexander :surkov from comment #8)
> That's right. It seems you do a diff against some old Firefox code. Not sure
> how it happened please let us know if you need a help.

I got new fresh copy, it really seems to be diffrent, there is no Format function I think this is fine and Im not gonna mess something :)
Comment 10 Michał Frontczak [:fxa90id] ♥ 2012-04-03 09:01:30 PDT
Created attachment 611834 [details] [diff] [review]
new
Comment 12 :Ehsan Akhgari (away Aug 1-5) 2012-04-05 10:45:21 PDT
Thanks, Michał!

https://hg.mozilla.org/mozilla-central/rev/7f363400344b

Note You need to log in before you can comment on or make changes to this bug.