1368547 - Investigate removing nsFrameManagerBase::mPlaceholderMap and instead store the placeholder on a frame property on the out-of-flow

Status: RESOLVED FIXED

(Core :: Layout: Positioned, enhancement)

Comment 1

[Mats Palmgren (inactive)]

Attachment: part 1 - Remove nsFrameManagerBase::mPlaceholderMap and store the placeholder on a frame property on the out-of-flow instead

This first part does the semantic changes: removes the mPlaceholderMap hashtable
and stores the placeholder in a frame property of the corresponding abs.pos.
There's also a part 2 that does the more mechanical changes, but I figured
it'd be easier to review them separately.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 2 - Remove nsFrameManager/PresShell methods dealing with placeholders and introduce a nsIFrame::GetPlaceholderFrame() convenience method

This changes all calls to nsFrameManager/PresShell methods for getting
the placeholder from frameManager->GetPlaceholderFrameFor(aFrame) to
aFrame->GetPlaceholderFrame(), and then removes those methods.

The DeleteProperty call isn't needed since that happens inside
the nsFrame::DestroyFrom so DeleteAllProperties() takes care of that.

RegisterPlaceholderFrame is replaced by a SetProperty call.
That happens in one place only so figured it's not necessary
to introduce a method for doing that.

Comment 3

[Mats Palmgren (inactive)]

Fwiw, in a Linux64 Opt build this improves the page load time by 0.3s
for the testcase in bug 1359341 with N=2000, which shows that these
hashtable lookups are indeed rather slow.

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8872970 [details] [diff] [review]
part 1 - Remove nsFrameManagerBase::mPlaceholderMap and store the placeholder on a frame property on the out-of-flow instead

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

::: layout/generic/nsIFrame.h
@@ +1186,5 @@
>                                        DestroyContentArray)
>  
>    NS_DECLARE_FRAME_PROPERTY_SMALL_VALUE(BidiDataProperty, mozilla::FrameBidiData)
>  
> +  NS_DECLARE_FRAME_PROPERTY_SMALL_VALUE(PlaceholderFrameProperty, nsPlaceholderFrame*)

AFAICS, it should work to use NS_DECLARE_FRAME_PROPERTY_SMALL_VALUE like this, but for other properties that hold frame pointers we generally use NS_DECLARE_FRAME_PROPERTY_WITHOUT_DTOR directly (e.g. IBSplitSibling); might be nice to be consistent with that usage?

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8872973 [details] [diff] [review]
part 2 - Remove nsFrameManager/PresShell methods dealing with placeholders and introduce a nsIFrame::GetPlaceholderFrame() convenience method

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

Nice!

Comment 6

[Mats Palmgren (inactive)]

> ... other properties that hold frame pointers we generally use NS_DECLARE_FRAME_PROPERTY_WITHOUT_DTOR

OK, fixed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddcdb354143a
part 1 - Remove nsFrameManagerBase::mPlaceholderMap and store the placeholder on a frame property on the out-of-flow instead.  r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/45409ba9a985
part 2 - Remove nsFrameManager/PresShell methods dealing with placeholders and introduce a nsIFrame::GetPlaceholderFrame() convenience method.  r=jfkthame

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ddcdb354143a
https://hg.mozilla.org/mozilla-central/rev/45409ba9a985