Store normal frame position before applying relative positioning

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: coyotebush, Assigned: coyotebush)

Tracking

Trunk
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

nsHTMLReflowState::ApplyRelativePositioning, before changing the position given, should store it in a frame property. There are already a number of calculations that need this "normal" position and get it by using |GetPosition() - GetRelativeOffset()|. Storing it explicitly is cleaner and will work when sticky positioning changes the frame's position. This stored position will also be used in sticky positioning calculations.
Is this instead of the offset or in addition to the offset?
It looks like ComputedOffsetProperty will then be unnecessary, yes. I only notice one use that doesn't directly follow the pattern I've noted, but of course that can be reworked using GetPosition and GetNormalPosition.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#3952
ok, sounds good.

I'm not sure if GetNormalPosition() is the best name, though I don't have a better one off the top of my head.
Maybe GetStaticPosition(), though I'm not crazy about that, either?
ReflowPosition (or FlowPosition) might also work, though I still favor NormalPosition.
Depends on: 898797
That worked out well enough. Most of the changes here are of the form |GetPosition() - GetRelativeOffset()| => |GetNormalPosition()|, but a few are slightly more involved, and ApplyRelativePositioning had to be changed to take the whole frame as a parameter.

A try push with this and the patches from bug 898797:
https://tbpl.mozilla.org/?tree=Try&rev=49ea49b2d4f2
Attachment #783213 - Flags: review?(dbaron)
Oh, plus this similar bit. (Still not sure I could safely store computed sticky offsets in mComputedOffsets, but those I might want to only store in a frame property anyway.)
Attachment #783249 - Flags: review?(dbaron)
(I'll fold the patches together before checking in.)
Folded patches and rebased onto m-c (with the nsTextFrameThebes move).
Attachment #785820 - Flags: review?(dbaron)
Attachment #783213 - Attachment is obsolete: true
Attachment #783213 - Flags: review?(dbaron)
Attachment #783249 - Attachment is obsolete: true
Attachment #783249 - Flags: review?(dbaron)
Comment on attachment 785820 [details] [diff] [review]
Store normal frame position before applying relative positioning.

nsFlexContainerFrame.cpp:

Looks ok to me, but maybe run it by dholbert?

nsFloatManager.cpp:

Probably best to initialize as:
  nsRect region(aFloat->GetNormalPosition(), aFloat->GetSize());


nsFrame.cpp:

Given the code that was there before, I suspect it might be faster
for GetNormalPosition to check IsRelativelyPositioned() before getting 
the frame property, although maybe frame property access is faster now.
Maybe at least leave a comment suggesting that it might be faster?


r=dbaron with that

(though I'm still wondering about the name, but ok with it)
Attachment #785820 - Flags: review?(dbaron) → review+
Attachment #785820 - Flags: feedback?(dholbert)
Comment on attachment 785820 [details] [diff] [review]
Store normal frame position before applying relative positioning.

>+++ b/layout/generic/nsFlexContainerFrame.cpp
>@@ -2408,20 +2408,20 @@ nsFlexContainerFrame::Reflow(nsPresConte
>-      // (We subtract mComputedOffsets.top because we don't want relative
>-      // positioning on the child to affect the baseline that we read from it).
>-      flexContainerAscent = physicalPosn.y + childDesiredSize.ascent -
>-        childReflowState.mComputedOffsets.top;
>+      // (We don't want relative positioning on the child to affect the
>+      // baseline that we read from it).
>+      flexContainerAscent = curItem.Frame()->GetNormalPosition().y +
>+        childDesiredSize.ascent;

Just one nit -- could you tweak the comment to say:
 // We use GetNormalPosition() instead of GetRect() because we don't want
instead of just "We don't want"? (or something to that effect)

Otherwise it's not really clear what part of the code the comment is talking about, unless the reader knows what GetNormalPosition() is. And most readers won't recognize that at first, since GetNormalPosition is all-new :)
Attachment #785820 - Flags: feedback?(dholbert) → feedback+
Version: unspecified → Trunk
Done.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10)
> nsFrame.cpp:
> 
> Given the code that was there before, I suspect it might be faster
> for GetNormalPosition to check IsRelativelyPositioned() before getting 
> the frame property, although maybe frame property access is faster now.
> Maybe at least leave a comment suggesting that it might be faster?

Given that most of the current callers of GetRelativeOffset didn't bother passing aDisplay, I'd guess the performance impact would be pretty minimal. But I added a comment, and see also bug 404215 comment 42.
Attachment #785820 - Attachment is obsolete: true
Attachment #787865 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e0293081301
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 911786
You need to log in before you can comment on or make changes to this bug.