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

RESOLVED FIXED in Firefox 26

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(firefox25 unaffected, firefox26+ fixed, firefox27 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 805628 [details] [diff] [review]
We should not store the NormalPositionProperty on all frames, only relatively positioned ones.
Attachment #805628 - Flags: review?(dholbert)
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 <dbaron@dbaron.org>
># 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+
(Assignee)

Comment 3

5 years ago
Oops, yes, I'll undo that.  I originally used it, before I noticed mistake #2 in addition to mistake #1.
(Assignee)

Comment 7

5 years ago
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?
https://hg.mozilla.org/mozilla-central/rev/71ece6d2f3f8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
status-firefox25: --- → unaffected
status-firefox26: --- → affected
status-firefox27: --- → fixed
tracking-firefox26: ? → +
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.