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

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: We should not store the NormalPositionProperty on all frames, only relatively positioned ones.

Comment 2

[Daniel Holbert [: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 <[e-mail]>
># 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.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oops, yes, I'll undo that.  I originally used it, before I noticed mistake #2 in addition to mistake #1.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=3577a04b7c3c

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://tbpl.mozilla.org/?tree=Try&rev=c2448e8b0ae5

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71ece6d2f3f8

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71ece6d2f3f8

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d622582bd52

Comment 10

[Tracy Walker [:tracy]]

If there is anything QA can do here to verify, please remove [qa-] from whiteboard and provide details, thanks.