Closed Bug 1340082 Opened 3 years ago Closed 3 years ago

Convert nsDisplayOutline to WebRenderDisplayItemLayer

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

Details

Attachments

(1 file)

I'll try to convert nsDisplayOutline to WebRenderDisplayItemLayer.
nsDisplayOutline is also using OpDPPushBorder. I keep a Maybe<nsCSSBorderRenderer> in nsDisplayOutline when doing 'GetLayerState'. Should I use UniquePtr here? Then I don't need to include 'nsCSSRenderingBorders.h' in 'nsDisplayList.h'. But currently we seems to use Maybe<> for every renderer.
Attachment #8839833 - Flags: review?(matt.woodrow)
Comment on attachment 8839833 [details] [diff] [review]
Convert nsDisplayOutline to WebRenderDisplayItemLayer

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

I don't think UniquePtr is worth it as that'll result in allocations being on the heap (making it part of the display item puts it in the display list builder arena).

Maybe the best option would be to teach nsCSSRendering how to allocate things into the arena, but that's more complex.

::: layout/painting/nsDisplayList.cpp
@@ +4096,5 @@
> +                                LayerManager* aManager,
> +                                const ContainerLayerParameters& aParameters)
> +{
> +  if (!gfxPrefs::LayersAllowOutlineLayers()) {
> +    return LAYER_INACTIVE;

This (and the one below) should both be LAYER_NONE right?

Going through the overhead of building an inactive layer doesn't seem worth it when it's going to be painted the same anyway.
Attachment #8839833 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> Comment on attachment 8839833 [details] [diff] [review]
> Convert nsDisplayOutline to WebRenderDisplayItemLayer
> 
> Review of attachment 8839833 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't think UniquePtr is worth it as that'll result in allocations being
> on the heap (making it part of the display item puts it in the display list
> builder arena).
> 
> Maybe the best option would be to teach nsCSSRendering how to allocate
> things into the arena, but that's more complex.
> 
> ::: layout/painting/nsDisplayList.cpp
> @@ +4096,5 @@
> > +                                LayerManager* aManager,
> > +                                const ContainerLayerParameters& aParameters)
> > +{
> > +  if (!gfxPrefs::LayersAllowOutlineLayers()) {
> > +    return LAYER_INACTIVE;
> 
> This (and the one below) should both be LAYER_NONE right?
> 
> Going through the overhead of building an inactive layer doesn't seem worth
> it when it's going to be painted the same anyway.

Oh...you are right, should be LAYER_NONE.
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6f04636eaa
Convert nsDisplayOutline to WebRenderDisplayItemLayer. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/dd6f04636eaa
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.