Closed Bug 734566 Opened 8 years ago Closed 8 years ago

optimize layout of TextAttrsMgr

Categories

(Core :: Disability Access APIs, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: tbsaunde, Assigned: fxa90id)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 2 obsolete files)

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
Attached patch switched (obsolete) — Splinter Review
(In reply to Michał Frontczak :fxa90id from comment #1)
> Created attachment 606986 [details] [diff] [review]
> switched

nah, you should change the ordering of members
also, please make sure to ask somebody for feedback (or review), you can use mentor for that when you push the patch
ok thx for response
Attachment #606986 - Flags: review?(trev.saunders)
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.
Attachment #606986 - Flags: review?(trev.saunders) → review-
Assignee: nobody → michaljev
Attached patch changes (obsolete) — Splinter Review
I switched all members except variables
Attachment #606986 - Attachment is obsolete: true
Attachment #610698 - Flags: review?(trev.saunders)
Attachment #610698 - Attachment description: I changed all members in file :) → changes
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.
Attachment #610698 - Flags: review?(trev.saunders)
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.
(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 :)
Status: NEW → ASSIGNED
Attached patch newSplinter Review
Attachment #610698 - Attachment is obsolete: true
Attachment #611834 - Flags: review+
Attachment #611834 - Flags: review+ → review?(trev.saunders)
Attachment #611834 - Flags: review?(trev.saunders) → review+
Target Milestone: --- → mozilla14
Thanks, Michał!

https://hg.mozilla.org/mozilla-central/rev/7f363400344b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.