Closed Bug 917021 Opened 7 years ago Closed 7 years ago

nsIFrame::NormalPositionProperty() should only be set on relatively positioned frames


(Core :: Layout: Block and Inline, defect)

Not set



Tracking Status
firefox25 --- unaffected
firefox26 + fixed
firefox27 --- fixed


(Reporter: dbaron, Assigned: dbaron)



(Whiteboard: [qa-])


(1 file)

While reviewing bug 898794 I missed two mistakes in nsHTMLReflowState::ApplyRelativePositioning:

 (1) nsIFrame::NormalPositionProperty() should only get set on frames that are relatively positioned.  Right now we're setting it on all frames, which as far as layout goes is a serious memory use regression (and probably slight performance rgression).  I'm surprised it didn't show up in tests.

 (2) The failure to use nsIFrame::IsRelativelyPositioned() or nsStyleDisplay::IsRelativelyPosition(nsIFrame*) means the test is wrong for svg:text.

#1 is something we need to get fixed on aurora, and the correct fix for it just happens to fix #2 as well.
Comment on attachment 805628 [details] [diff] [review]
We should not store the NormalPositionProperty on all frames, only relatively positioned ones.

># HG changeset patch
># User L. David Baron <>
># Date 1379371046 25200
>#      Mon Sep 16 15:37:26 2013 -0700
># Node ID 5436f440aaaa0746116a1eaf1b49adb3cbfdf25a
># Parent  b9a5dcf48ee6ac1745337253379c76a9f7eef9b1
>Bug 917021:  We should not store the NormalPositionProperty on all frames, only relatively positioned ones.
>diff --git a/layout/generic/nsHTMLReflowState.cpp b/layout/generic/nsHTMLReflowState.cpp
>--- a/layout/generic/nsHTMLReflowState.cpp
>+++ b/layout/generic/nsHTMLReflowState.cpp
>@@ -844,27 +844,37 @@ nsHTMLReflowState::ComputeRelativeOffset
>   }
> }
> /* static */ void
> nsHTMLReflowState::ApplyRelativePositioning(nsIFrame* aFrame,
>                                             const nsMargin& aComputedOffsets,
>                                             nsPoint* aPosition)
> {
>+  const nsStyleDisplay* display = aFrame->StyleDisplay();
>+  if (!aFrame->IsRelativelyPositioned()) {
>+    NS_ASSERTION(!aFrame->Properties().Get(nsIFrame::NormalPositionProperty()),
>+                 "We assume that changing the 'position' property causes "
>+                 "frame reconstruction.  If that ever changes, this code "
>+                 "should call "
>+                 "props.Delete(nsIFrame::NormalPositionProperty())");
>+    return;
>+  }
>   // Store the normal position
>-  const nsStyleDisplay* display = aFrame->StyleDisplay();
>   if (NS_STYLE_POSITION_RELATIVE == display->mPosition) {

You don't technically need to move the "display" declaration, do you? (It's not used until the end of this method, where it's currently declared; this patch moves it up higher, for no apparent reason)

So maybe leave that line as-is; but otherwise r=me.
Attachment #805628 - Flags: review?(dholbert) → review+
Oops, yes, I'll undo that.  I originally used it, before I noticed mistake #2 in addition to mistake #1.
Comment on attachment 805628 [details] [diff] [review]
We should not store the NormalPositionProperty on all frames, only relatively positioned ones.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 898794
User impact if declined: reduced speed and increased memory use
Testing completed (on m-c, etc.): on mozilla-inbound
Risk to taking this patch (and alternatives if risky): low risk, though main risks relate to position:relative and position:sticky (latter is preffed off).  I think we have reasonably good test coverage for position:sticky and position:relative.  It's also changing code that was changed only a few weeks ago, so it's not adding much risk.
String or IDL/UUID changes made by this patch: none
Attachment #805628 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attachment #805628 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
If there is anything QA can do here to verify, please remove [qa-] from whiteboard and provide details, thanks.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.