optimize layout of TextAttrsMgr

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: fxa90id)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
x86_64
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

Updated

5 years ago
Blocks: 389800
(Assignee)

Comment 1

5 years ago
Created attachment 606986 [details] [diff] [review]
switched

Comment 2

5 years ago
(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

5 years ago
also, please make sure to ask somebody for feedback (or review), you can use mentor for that when you push the patch
(Assignee)

Comment 4

5 years ago
ok thx for response
(Assignee)

Updated

5 years ago
Attachment #606986 - Flags: review?(trev.saunders)
(Reporter)

Comment 5

5 years ago
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)

Updated

5 years ago
Assignee: nobody → michaljev
(Assignee)

Comment 6

5 years ago
Created attachment 610698 [details] [diff] [review]
changes

I switched all members except variables
Attachment #606986 - Attachment is obsolete: true
Attachment #610698 - Flags: review?(trev.saunders)
(Assignee)

Updated

5 years ago
Attachment #610698 - Attachment description: I changed all members in file :) → changes
(Reporter)

Comment 7

5 years ago
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)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
(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
(Assignee)

Comment 10

5 years ago
Created attachment 611834 [details] [diff] [review]
new
Attachment #610698 - Attachment is obsolete: true
Attachment #611834 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #611834 - Flags: review+ → review?(trev.saunders)
(Reporter)

Updated

5 years ago
Attachment #611834 - Flags: review?(trev.saunders) → review+

Comment 11

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

Updated

5 years ago
Target Milestone: --- → mozilla14
Thanks, Michał!

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