nsHTMLReflowState's second constructor has redundant initialization for mFlags.mIsTopOfPage from parent reflow state

RESOLVED FIXED in mozilla10

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
nsHTMLReflowState's second constructor looks like this:
> 126 nsHTMLReflowState::nsHTMLReflowState(nsPresContext*           aPresContext,
[...]
> 132                                      bool                     aInit)
> 133   : nsCSSOffsetState(aFrame, aParentReflowState.rendContext)
[...]
> 136   , mFlags(aParentReflowState.mFlags)
> 137 {
[...]
> 166   mFlags.mIsTopOfPage = aParentReflowState.mFlags.mIsTopOfPage;
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#126

The mFlags.mIsTopOfPage assignment on line 166 is redundant, since mFlags is already initialized off of aParentReflowState's mFlags.

This line was last modified in the patch[1] for bug 113424, which tweaked the boolean member-variable "nsHTMLReflowState::isTopOfPage" to be part of mFlags.

Before that patch, this line looked like:
> isTopOfPage = aParentReflowState.isTopOfPage;
...and it was necessary because isTopOfPage wasn't initialized elsewhere.  But now it gets initialized as part of mFlags, so this line is redundant.

[1] http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&file=nsHTMLReflowState.cpp&rev2=1.129&rev1=1.128
(Assignee)

Comment 1

6 years ago
Created attachment 566333 [details] [diff] [review]
fix

Patch attached.  Including 30 lines of context (back to the mFlags initialization), to demonstrate the redundancy.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #566333 - Flags: review?(dbaron)
Comment on attachment 566333 [details] [diff] [review]
fix

r=dbaron, but maybe add a comment near the other flag initializations pointing out that mFlags is initialized above by copy-construction so that we only need to initialize those flags that need a value other than what we get by copying the parent.
Attachment #566333 - Flags: review?(dbaron) → review+
(Assignee)

Comment 3

6 years ago
Added a message as suggested (and added a blank line to set apart the mFlags.* assignments from the other member-variable initializations), and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c09b6f933f76
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla10

Comment 4

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