Closed Bug 865477 Opened 7 years ago Closed 7 years ago

nsBlockFrame::Reflow should use Maybe<> for stack-allocation, instead of nsAutoPtr<> & heap allocation, for lazily-constructed nsHTMLReflowState

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: dholbert, Assigned: dholbert)

References

()

Details

Attachments

(1 file)

Right now, nsBlockFrame::Reflow has a variable 'mutableReflowState' which is heap-allocated because we don't initially know whether we'll need to construct it or not:
> 977   nsAutoPtr<nsHTMLReflowState> mutableReflowState;
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=abc416eac56f#977

This is the *only* place in our code where we have a nsAutoPtr<nsHTMLReflowState>, FWIW.

Maybe<> is much better-suited for this situation. Its header-comment says it's a "Small utility for lazily constructing objects without using dynamic storage".  We also already use it for this same purpose in one other spot -- in nsLineLayout::ReflowFrame:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout.cpp?rev=32503b49921e#769

Plus, reflow states are inherently stack-based objects (they're generally declared as local-variables, used for the duration of a Reflow call, and then discarded), so it's much nicer to have them just live on the stack.

So: Let's definitely switch this to Maybe<>.
Summary: nsBlockFrame::Reflow should use Maybe<> instead of heap for possibly-allocated nsHTMLReflowState → nsBlockFrame::Reflow should use Maybe<> instead of heap for lazily-constructed nsHTMLReflowState
Attached patch fix v1Splinter Review
I haven't tested this vigorously yet, but it compiles and I think it should do the trick.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #741544 - Flags: review?(matspal)
Try run (Linux64-only, debug & opt, reftests & mochitests):
 https://tbpl.mozilla.org/?tree=Try&rev=93312673af54
Summary: nsBlockFrame::Reflow should use Maybe<> instead of heap for lazily-constructed nsHTMLReflowState → nsBlockFrame::Reflow should use Maybe<> for stack-allocation, instead of nsAutoPtr<> & heap allocation, for lazily-constructed nsHTMLReflowState
OS: Linux → All
Hardware: x86_64 → All
Actually, I'll tag bz for review to make sure he's in the loop on this change, since he added this variable in http://hg.mozilla.org/mozilla-central/rev/6f0219923a28#l1.36 for bug 626395.
Depends on: 626395
Attachment #741544 - Flags: review?(matspal) → review?(bzbarsky)
[Try run's green, woot!]
Comment on attachment 741544 [details] [diff] [review]
fix v1

r=me
Attachment #741544 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/6594045f0975
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.