Last Comment Bug 693801 - nsHTMLReflowState's second constructor has redundant initialization for mFlags.mIsTopOfPage from parent reflow state
: nsHTMLReflowState's second constructor has redundant initialization for mFlag...
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla10
Assigned To: Daniel Holbert [:dholbert] (vacation, returning 2/27)
: Jet Villegas (:jet)
Depends on: 113424
  Show dependency treegraph
Reported: 2011-10-11 13:58 PDT by Daniel Holbert [:dholbert] (vacation, returning 2/27)
Modified: 2011-10-22 06:06 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

fix (3.08 KB, patch)
2011-10-11 14:02 PDT, Daniel Holbert [:dholbert] (vacation, returning 2/27)
dbaron: review+
Details | Diff | Splinter Review

Description User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-11 13:58:39 PDT
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;

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.

Comment 1 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-11 14:02:42 PDT
Created attachment 566333 [details] [diff] [review]

Patch attached.  Including 30 lines of context (back to the mFlags initialization), to demonstrate the redundancy.
Comment 2 User image David Baron :dbaron: ⌚️UTC-8 2011-10-21 11:45:52 PDT
Comment on attachment 566333 [details] [diff] [review]

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.
Comment 3 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-10-21 13:43:37 PDT
Added a message as suggested (and added a blank line to set apart the mFlags.* assignments from the other member-variable initializations), and pushed:
Comment 4 User image Ed Morley [:emorley] 2011-10-22 06:06:40 PDT

Note You need to log in before you can comment on or make changes to this bug.