The default bug view has changed. See this FAQ.

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
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.