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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

No description provided.
Depends on: 1368802
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.
Attachment #8872970 - Flags: review?(jfkthame)
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.
Attachment #8872973 - Flags: review?(jfkthame)
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 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?
Attachment #8872970 - Flags: review?(jfkthame) → review+
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!
Attachment #8872973 - Flags: review?(jfkthame) → review+
> ... other properties that hold frame pointers we generally use NS_DECLARE_FRAME_PROPERTY_WITHOUT_DTOR

OK, fixed.
Pushed by mpalmgren@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ddcdb354143a
https://hg.mozilla.org/mozilla-central/rev/45409ba9a985
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.