734566 - optimize layout of TextAttrsMgr

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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] ♥]

Attachment: switched

Comment 2

[alexander :surkov (:asurkov)]

(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 (:asurkov)]

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] ♥]

ok thx for response

Comment 5

[Trevor Saunders (:tbsaunde)]

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] ♥]

Attachment: changes

I switched all members except variables

Comment 7

[Trevor Saunders (:tbsaunde)]

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 (:asurkov)]

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] ♥]

(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] ♥]

Attachment: new

Comment 11

[alexander :surkov (:asurkov)]

landed https://hg.mozilla.org/integration/mozilla-inbound/rev/7f363400344b

Comment 12

[(no longer active)]

Thanks, Michał!

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