Closed Bug 1368802 Opened 5 years ago Closed 5 years ago

nsFrameIterator::GetPlaceholderFrame is doing a slow hashtable lookup (GetPlaceholderFrameFor) on all frames

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: perf, Whiteboard: [qf:p1])

Attachments

(1 file)

In bug 1368547 I added an assertion in GetPlaceholderFrameFor that
asserted that the passed in frame has the NS_FRAME_OUT_OF_FLOW bit,
because otherwise the call is pointless.  It caught this bug.
http://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/layout/base/nsFrameTraversal.cpp#485

I'm intentionally allowing for the same behavior as before for aFrame == null,
i.e. returning null, because this code is old and crufty... :-)

I'm removing this bit though because I don't think it's ever done
anything useful:
if (result != aFrame)
    result = GetPlaceholderFrame(result);

|result| is initialized to |aFrame|, so the only way it can be different
is if it got assigned to a placeholder frame from GetPlaceholderFrameFor.
Recursing with a aFrame == "a placeholder" can't possibly find another
placeholder though, so the returned result is always the same.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4854cbca5b36c70d55ec5ae0ac3e751abdc2bb8
Attachment #8872784 - Flags: review?(jfkthame)
Comment on attachment 8872784 [details] [diff] [review]
nsFrameIterator::GetPlaceholderFrame should only try to get the placeholder for out-of-flow frames, because in-flow frames never have a placeholder.

Review of attachment 8872784 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense, modulo one question I wondered about...

::: layout/base/nsFrameTraversal.cpp
@@ +493,3 @@
>    }
> +  nsIFrame* placeholder =
> +    aFrame->PresContext()->PresShell()->GetPlaceholderFrameFor(aFrame);

Why get the presContext from aFrame rather than use the frameIterator's mPresContext field? Seems like that involves chasing an additional pointer (as it amounts to aFrame->mStyleContext->mPresContext, IIRC).
Attachment #8872784 - Flags: review?(jfkthame) → review+
Don't worry about it ;-)  I'll remove that in bug 1368547, so this becomes
aFrame->GetPlaceholderFrame() instead.
SGTM.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/422e3c006cf2
nsFrameIterator::GetPlaceholderFrame should only try to get the placeholder for out-of-flow frames, because in-flow frames never have a placeholder.  r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/422e3c006cf2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [qf] → [qf:p1]
You need to log in before you can comment on or make changes to this bug.