1365972 - Convert nsDisplayFilter to a WR display item

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Martin Robinson (email:[e-mail])]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36
Build ID: 20170512091250

Comment 1

[Martin Robinson (email:[e-mail])]

Attachment: Very early WIP for WebRender filters support

Comment 2

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Comment 3

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Comment 4

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8871691 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

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

::: gfx/layers/Layers.h
@@ +1965,5 @@
>    // Store display list log.
>    nsCString mDisplayListLog;
>  
>    StyleAnimationValue mBaseAnimationStyle;
> +  nsTArray<nsStyleFilter> mFilterChain;

I'm uneasy about putting nsStyleStruct code in Layer for a couple of reasons.

The first is that this struct has pointers to other style structs and layers are retained between paints and can be shadow layers living on the compositor where it doesn't make sense to have a style pointer. So I think this could end up with a dangling pointer here. The other reason is that this is only needed for ContainerLayer, so whatever we use for this should be put there.

So I think a solution could be to create a graphics `Filter` struct (maybe in gfx/2d/Types.h? there is already a different FilterType enum in there that is unrelated). This struct could basically be a C++ port of webrender_traits::Filter. Then just convert it to WrFilter like we for nsStyleFilter. It's an extra step in the pipeline, but I think it's cleaner.

::: gfx/layers/wr/StackingContextHelper.cpp
@@ +38,5 @@
> +    case NS_STYLE_FILTER_OPACITY:
> +    case NS_STYLE_FILTER_BLUR:
> +    case NS_STYLE_FILTER_INVERT:
> +    default:
> +      filterOp.filter_type = WrFilterOpType::None;

Let's use MOZ_ASSERT_UNREACHABLE("") here because the code in nsDisplayList should guarantee we will never get here.

::: gfx/webrender_bindings/src/bindings.rs
@@ +61,5 @@
>  
> +#[repr(u32)]
> +#[derive(Copy, Clone)]
> +pub enum WrFilterOpType {
> +  None = 0,

Let's not add a None value here, so we don't need the `unreachable!("")` later.

::: layout/painting/FrameLayerBuilder.h
@@ +279,5 @@
>    bool mInLowPrecisionDisplayPort;
>    bool mForEventsAndPluginsOnly;
>    layers::LayerManager::PaintedLayerCreationHint mLayerCreationHint;
>  
> +  const nsTArray<nsStyleFilter>* mFilter;

It doesn't seem like this is used?

::: layout/painting/nsDisplayList.cpp
@@ +8659,5 @@
> +      return LAYER_SVG_EFFECTS;
> +    }
> +  }
> +
> +  if (mFrame->IsFrameOfType(nsIFrame::eSVG)) {

Maybe put this condition first so SVG frames have an early out?

::: modules/libpref/init/all.js
@@ +5750,5 @@
>  pref("layers.advanced.outline-layers", 2);
>  pref("layers.advanced.solid-color", 2);
>  pref("layers.advanced.table", 2);
>  pref("layers.advanced.text-layers", 2);
> +pref("layers.advanced.filter-layers", 2);

Is this passing all reftests so it can be enabled by default? Does enabling this when WebRender is disabled cause artifacts?

Comment 6

[Martin Robinson (email:[e-mail])]

(In reply to Ryan Hunt [:rhunt] from comment #5)
> Comment on attachment 8871691 [details] [diff] [review]
> Add initial WebRender support for nsDisplayFilter

Thanks for the great review!

> 
> Review of attachment 8871691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/Layers.h
> @@ +1965,5 @@
> >    // Store display list log.
> >    nsCString mDisplayListLog;
> >  
> >    StyleAnimationValue mBaseAnimationStyle;
> > +  nsTArray<nsStyleFilter> mFilterChain;
> 
> I'm uneasy about putting nsStyleStruct code in Layer for a couple of reasons.
> 
> The first is that this struct has pointers to other style structs and layers
> are retained between paints and can be shadow layers living on the
> compositor where it doesn't make sense to have a style pointer. So I think
> this could end up with a dangling pointer here. The other reason is that
> this is only needed for ContainerLayer, so whatever we use for this should
> be put there.
> 
> So I think a solution could be to create a graphics `Filter` struct (maybe
> in gfx/2d/Types.h? there is already a different FilterType enum in there
> that is unrelated). This struct could basically be a C++ port of
> webrender_traits::Filter. Then just convert it to WrFilter like we for
> nsStyleFilter. It's an extra step in the pipeline, but I think it's cleaner.

Okay. I'll implement this in the next version of the patch.

> 
> ::: gfx/layers/wr/StackingContextHelper.cpp
> @@ +38,5 @@
> > +    case NS_STYLE_FILTER_OPACITY:
> > +    case NS_STYLE_FILTER_BLUR:
> > +    case NS_STYLE_FILTER_INVERT:
> > +    default:
> > +      filterOp.filter_type = WrFilterOpType::None;
> 
> Let's use MOZ_ASSERT_UNREACHABLE("") here because the code in nsDisplayList
> should guarantee we will never get here.

I didn't do this because some layers that aren't created for nsDisplayFilter are carrying filter chains as well. My approach isn't very robust though, so I'll make sure that these layers do not include a filter chain at all.

> ::: gfx/webrender_bindings/src/bindings.rs
> @@ +61,5 @@
> >  
> > +#[repr(u32)]
> > +#[derive(Copy, Clone)]
> > +pub enum WrFilterOpType {
> > +  None = 0,
> 
> Let's not add a None value here, so we don't need the `unreachable!("")`
> later.

Okay.

> 
> ::: layout/painting/FrameLayerBuilder.h
> @@ +279,5 @@
> >    bool mInLowPrecisionDisplayPort;
> >    bool mForEventsAndPluginsOnly;
> >    layers::LayerManager::PaintedLayerCreationHint mLayerCreationHint;
> >  
> > +  const nsTArray<nsStyleFilter>* mFilter;
> 
> It doesn't seem like this is used?

I'll remove this.

> 
> ::: layout/painting/nsDisplayList.cpp
> @@ +8659,5 @@
> > +      return LAYER_SVG_EFFECTS;
> > +    }
> > +  }
> > +
> > +  if (mFrame->IsFrameOfType(nsIFrame::eSVG)) {
> 
> Maybe put this condition first so SVG frames have an early out?

Okay.
 
> ::: modules/libpref/init/all.js
> @@ +5750,5 @@
> >  pref("layers.advanced.outline-layers", 2);
> >  pref("layers.advanced.solid-color", 2);
> >  pref("layers.advanced.table", 2);
> >  pref("layers.advanced.text-layers", 2);
> > +pref("layers.advanced.filter-layers", 2);
> 
> Is this passing all reftests so it can be enabled by default? Does enabling
> this when WebRender is disabled cause artifacts?

I don't think so. Here, I wasn't sure exactly what the policy was for enabling WebRender features.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8871691 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

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

::: gfx/thebes/gfxPrefs.h
@@ +508,5 @@
>    DECL_OVERRIDE_PREF(Live, "layers.advanced.outline-layers",          LayersAllowOutlineLayers, gfxPrefs::OverrideBase_WebRender());
>    DECL_OVERRIDE_PREF(Live, "layers.advanced.solid-color",             LayersAllowSolidColorLayers, gfxPrefs::OverrideBase_WebRender());
>    DECL_OVERRIDE_PREF(Live, "layers.advanced.table",                   LayersAllowTable, gfxPrefs::OverrideBase_WebRendest());
>    DECL_OVERRIDE_PREF(Live, "layers.advanced.text-layers",             LayersAllowTextLayers, gfxPrefs::OverrideBase_WebRendest());
> +  DECL_OVERRIDE_PREF(Live, "layers.advanced.filter-layers",           LayersAllowFilterLayers, gfxPrefs::OverrideBase_WebRender());

Drive-by comment: please make sure this is added in alphabetical order by pref name. Same in all.js (for the block of advanced layers prefs anyway).

Comment 8

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Comment 9

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8872521 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

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

This looks good, just some more code organization things.

::: gfx/2d/Types.h
@@ +127,5 @@
>    PREMULTIPLY,
>    UNPREMULTIPLY
>  };
>  
> +enum class CSSFilterType : int8_t {

Ah, so this isn't the best place for this. Moz2D is technically a standalone library that shouldn't have types for things it doesn't use.

This can go into layers/LayersTypes.h. I should have thought more about that.

@@ +360,5 @@
>    Float offset;
>    Color color;
>  };
>  
> +struct CSSFilter {

Same as CSSFilterType.

::: gfx/layers/Layers.h
@@ +1862,5 @@
>       mExtraDumpInfo.Clear();
>  #endif
>    }
>  
> +  void SetFilterChain(const nsTArray<mozilla::gfx::CSSFilter>& aFilterChain)

Let's accept a nsTArray<mozilla::gfx::CSSFilter>&& to prevent a copy here.

@@ +1964,5 @@
>    // Store display list log.
>    nsCString mDisplayListLog;
>  
>    StyleAnimationValue mBaseAnimationStyle;
> +  nsTArray<mozilla::gfx::CSSFilter> mFilterChain;

Let's move this into ContainerLayer.

::: gfx/layers/wr/StackingContextHelper.cpp
@@ +11,5 @@
>  
>  namespace mozilla {
>  namespace layers {
>  
> +static inline WrFilterOpType ToWrFilterOpType(const mozilla::gfx::CSSFilterType type)

This can be moved into webrender_bindings/WebRenderTypes.h

@@ +44,5 @@
> +  if (!layer) {
> +    return nsTArray<WrFilterOp>();
> +  }
> +
> +  nsTArray<WrFilterOp> filters = nsTArray<WrFilterOp>();

nit: you can omit `= nsTArray<WrFilterOp>();`

@@ +46,5 @@
> +  }
> +
> +  nsTArray<WrFilterOp> filters = nsTArray<WrFilterOp>();
> +  for (const CSSFilter& filter : layer->GetFilterChain()) {
> +    WrFilterOp webRenderFilter {

Can we add a ToWrFilterOp function for this?

It would be super trivial, but it matches what we do for everything else. It can go with ToWrFilterOpType in webrender_bindings/WebRenderTypes.h

::: gfx/thebes/gfxPrefs.h
@@ +503,5 @@
>    DECL_OVERRIDE_PREF(Live, "layers.advanced.canvas-background-color", LayersAllowCanvasBackgroundColorLayers, gfxPrefs::OverrideBase_WebRendest());
>    DECL_OVERRIDE_PREF(Live, "layers.advanced.caret-layers",            LayersAllowCaretLayers, gfxPrefs::OverrideBase_WebRender());
>    DECL_OVERRIDE_PREF(Live, "layers.advanced.columnRule-layers",       LayersAllowColumnRuleLayers, gfxPrefs::OverrideBase_WebRendest());
>    DECL_OVERRIDE_PREF(Live, "layers.advanced.displaybuttonborder-layers", LayersAllowDisplayButtonBorder, gfxPrefs::OverrideBase_WebRender());
> +  DECL_OVERRIDE_PREF(Live, "layers.advanced.filter-layers",           LayersAllowFilterLayers, gfxPrefs::OverrideBase_WebRender());

The default value should be gfxPrefs::OverrideBase_WebRendest() if this isn't passing all of the tests.

::: layout/painting/FrameLayerBuilder.cpp
@@ +115,5 @@
>  
>    return gMaskLayerImageCache;
>  }
>  
> +static inline Maybe<gfx::CSSFilter> ToCSSFilter(const nsStyleFilter& filter)

Let's not return Maybe<> here because the only case we return Nothing() should be statically prevented.

And also, can we move this into layers/LayersTypes.h along with CSSFilter, CSSFilterType?

That'll keep all these things in the same location, and not pollute FLB with conversion functions.

@@ +141,5 @@
> +    case NS_STYLE_FILTER_SEPIA:
> +    case NS_STYLE_FILTER_OPACITY:
> +    case NS_STYLE_FILTER_BLUR:
> +    case NS_STYLE_FILTER_INVERT:
> +    default:

nit: the extra cases above this default are unnecessary

@@ +5665,5 @@
>                                    containerLayer, state, scaleParameters)) {
>      return nullptr;
>    }
>  
> +  if (aFlags & PASS_FILTERS_TO_LAYER && state != LAYER_SVG_EFFECTS) {

nit: the precedence for these expressions is correct, but it includes a bitwise operator so I think it's a good idea to parenthesize it.

::: layout/painting/nsDisplayList.cpp
@@ +8661,5 @@
> +      return LAYER_SVG_EFFECTS;
> +    }
> +  }
> +
> +  if (ForceActiveLayers() || ShouldUseAdvancedLayer(aManager, gfxPrefs::LayersAllowFilterLayers)) {

Do we want to create filter layers when only ForceActiveLayers() is enabled? It seems like this is only supported with gfxPrefs::LayersAllowFilterLayers()

@@ +8662,5 @@
> +    }
> +  }
> +
> +  if (ForceActiveLayers() || ShouldUseAdvancedLayer(aManager, gfxPrefs::LayersAllowFilterLayers)) {
> +    return LAYER_ACTIVE_FORCE;;

nit: double semicolon

Comment 10

[Ryan Hunt [:rhunt] (on leave until early May)]

You should do a try run with this enabled to see what the test failures look like. You can use the try syntax from the wiki [1]

[1] https://wiki.mozilla.org/Platform/GFX/Quantum_Render

Comment 11

[Martin Robinson (email:[e-mail])]

(In reply to Ryan Hunt [:rhunt] from comment #10)
> You should do a try run with this enabled to see what the test failures look
> like. You can use the try syntax from the wiki [1]
> 
> [1] https://wiki.mozilla.org/Platform/GFX/Quantum_Render

Thanks for the review. I'll try to get the patch ready ASAP. Try results for the previous patch are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dae21649c14d5c0e1059491ca00c562fa5a4854a. I will rerun try when I make the changes you requested as well.

Comment 12

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Comment 13

[Martin Robinson (email:[e-mail])]

I posted a new patch based on your comments. I ended up having to put ToCSSFilter into LayersTypes.cpp because nsStyleStruct defines a type called Position that is defined in the geolocation code as well. Putting the code into the source file allowed me to isolate the two definitions. 

> Do we want to create filter layers when only ForceActiveLayers() is enabled? It seems like this is only supported with gfxPrefs::LayersAllowFilterLayers()

I'm not sure what the correct configuration is here, as I just copied the same pattern from a similar WebRender feature. Perhaps someone else can give help me to understand if this is the correct thing. All the tests are passing, if that affects the decision. Here are the treeherder results for the current version of the patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5507de9a4b9c771a2cdf9c9bd61e059271747c80

Comment 14

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8872957 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

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

This looks good to me, just a few more nits.

::: gfx/layers/Layers.h
@@ +2247,5 @@
>    EventRegionsOverride GetEventRegionsOverride() const {
>      return mEventRegionsOverride;
>    }
>  
> +  void SetFilterChain(const nsTArray<CSSFilter>&& aFilterChain) {

Get rid of the const here as it prevents move assignment from occurring, which will prevent a copy from happening.

@@ +2248,5 @@
>      return mEventRegionsOverride;
>    }
>  
> +  void SetFilterChain(const nsTArray<CSSFilter>&& aFilterChain) {
> +    mFilterChain = nsTArray<CSSFilter>(aFilterChain);

nit: you can omit the `nsTArray<CSSFilter>(..)`

::: gfx/layers/LayersTypes.cpp
@@ +9,5 @@
> +
> +namespace mozilla {
> +namespace layers {
> +
> +CSSFilter ToCSSFilter(const nsStyleFilter& filter) {

nit: move the open brace to the next line

::: gfx/layers/LayersTypes.h
@@ +39,1 @@
>  namespace mozilla {

nit: get rid of this extra blank line

::: gfx/layers/wr/StackingContextHelper.cpp
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/layers/StackingContextHelper.h"
>  
> +#include "mozilla/gfx/Types.h"

Did you mean to include this?

::: gfx/webrender_bindings/WebRenderTypes.h
@@ +15,5 @@
>  #include "mozilla/Range.h"
>  #include "Units.h"
>  #include "nsStyleConsts.h"
>  
> +

nit: remove this extra blank line

@@ +552,5 @@
>    VecU8 dl;
>    WrBuiltDisplayListDescriptor dl_desc;
>  };
>  
> +static inline WrFilterOpType ToWrFilterOpType(const mozilla::layers::CSSFilterType type) {

nit: you should be able to get rid of mozilla:: and just use layers:: throughout these functions

::: gfx/webrender_bindings/webrender_ffi.h
@@ +6,5 @@
>  
>  #ifndef WR_h
>  #define WR_h
>  
> +

nit: get rid of this extra blank line

::: layout/painting/FrameLayerBuilder.cpp
@@ +53,5 @@
>  
>  #include <algorithm>
>  #include <functional>
>  
> +

nit: get rid of this extra blank line

@@ +116,5 @@
>  
>    return gMaskLayerImageCache;
>  }
>  
> +static inline nsTArray<layers::CSSFilter> GetCSSFilters(const nsTArray<nsStyleFilter>& filters)

Either inline this where it is used or move this to LayersTypes.h, I'd rather not pollute this file with a helper function

Comment 15

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Martin Robinson (mrobinson) from comment #13)
> I posted a new patch based on your comments. I ended up having to put
> ToCSSFilter into LayersTypes.cpp because nsStyleStruct defines a type called
> Position that is defined in the geolocation code as well. Putting the code
> into the source file allowed me to isolate the two definitions. 
> 
> > Do we want to create filter layers when only ForceActiveLayers() is enabled? It seems like this is only supported with gfxPrefs::LayersAllowFilterLayers()
> 
> I'm not sure what the correct configuration is here, as I just copied the
> same pattern from a similar WebRender feature. Perhaps someone else can give
> help me to understand if this is the correct thing. All the tests are
> passing, if that affects the decision. Here are the treeherder results for
> the current version of the patch:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5507de9a4b9c771a2cdf9c9bd61e059271747c80

I'd say it's fine to leave it as is then. My only concern is that it will lead to incorrect rendering when force-active is enabled but webrender is not, but force-active usually causes issues anyways.

Comment 16

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8872957 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

Matt, can you take a quick glance at the FLB changes?

There's not much and it's only about piping some information for ContainerLayers when WR is enabled.

Comment 17

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Uploading the final version of the patch.

Comment 18

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8873337 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

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

::: layout/painting/FrameLayerBuilder.cpp
@@ +5622,5 @@
>      return nullptr;
>    }
>  
> +  if ((aFlags & PASS_FILTERS_TO_LAYER) && state != LAYER_SVG_EFFECTS) {
> +    nsTArray<layers::CSSFilter> cssFilters = nsTArray<layers::CSSFilter>();

nit: you can omit the `= nsTArray<layers::CSSFilter>()`

Or even better, `aContainerFrame->StyleEffects()->mFilters` has a known size so you can use `= nsTArray<..>(filterCount)` to reserve room for the new elements. That could prevent some extra allocations.

Comment 19

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 8873337 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

Matt, can you take a quick glance at the FLB changes?

There's not much and it's only about piping some information for ContainerLayers when WR is enabled.

Comment 20

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8872957 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

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

::: layout/painting/FrameLayerBuilder.cpp
@@ +5632,5 @@
>      return nullptr;
>    }
>  
> +  if ((aFlags & PASS_FILTERS_TO_LAYER) && state != LAYER_SVG_EFFECTS) {
> +    containerLayer->SetFilterChain(GetCSSFilters(aContainerFrame->StyleEffects()->mFilters));

Why can't we do this from nsDisplayFilter::BuildLayer?

::: layout/painting/nsDisplayList.cpp
@@ +8662,5 @@
> +    }
> +  }
> +
> +  if (ForceActiveLayers() || ShouldUseAdvancedLayer(aManager, gfxPrefs::LayersAllowFilterLayers)) {
> +    return LAYER_ACTIVE_FORCE;

I don't think we want to do this for ForceActiveLayers, since no backends except WR will actually be able to render this.

Do filters ever change the size of the output?

LAYER_SVG_EFFECTS has handling for invalidation etc when the effect changes the size, you'll be missing out on that by creating an active layer.

Comment 22

[Martin Robinson (email:[e-mail])]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> Comment on attachment 8872957 [details] [diff] [review]
> Add initial WebRender support for nsDisplayFilter
> 
> Review of attachment 8872957 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/FrameLayerBuilder.cpp
> @@ +5632,5 @@
> >      return nullptr;
> >    }
> >  
> > +  if ((aFlags & PASS_FILTERS_TO_LAYER) && state != LAYER_SVG_EFFECTS) {
> > +    containerLayer->SetFilterChain(GetCSSFilters(aContainerFrame->StyleEffects()->mFilters));
> 
> Why can't we do this from nsDisplayFilter::BuildLayer?

No important reason in particular. I added it there because that is where the other layer parameters are initialized. I'll move it out of FrameLayerBuilder. That will make things much simpler anyway.

> 
> ::: layout/painting/nsDisplayList.cpp
> @@ +8662,5 @@
> > +    }
> > +  }
> > +
> > +  if (ForceActiveLayers() || ShouldUseAdvancedLayer(aManager, gfxPrefs::LayersAllowFilterLayers)) {
> > +    return LAYER_ACTIVE_FORCE;
> 
> I don't think we want to do this for ForceActiveLayers, since no backends
> except WR will actually be able to render this.

Okay. I'll only activate layers if ShouldUseAdvancedLayer(aManager, gfxPrefs::LayersAllowFilterLayers) is true.
 
> Do filters ever change the size of the output?
> 
> LAYER_SVG_EFFECTS has handling for invalidation etc when the effect changes
> the size, you'll be missing out on that by creating an active layer.

None of the currently active filters change the size of the output. I'm not entirely sure this matters in WebRender anyhow, since it doesn't have an internal concept of layers. 

Thanks for the review. New patch incoming.

Comment 23

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Comment 24

[Martin Robinson (email:[e-mail])]

Treeherder results for current patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=245b3349209e686d1906415b99b3ca4778ed38fc

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8873759 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

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

I think you can drop the FLB changes entirely now.

Comment 26

[Martin Robinson (email:[e-mail])]

Attachment: Add initial WebRender support for nsDisplayFilter

Comment 27

[Martin Robinson (email:[e-mail])]

Comment on attachment 8874831 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

I updated the patch to include the latest comments, to generate the ffi file with the same version of cbindgen, and rebased on the latest changes.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8874831 [details] [diff] [review]
Add initial WebRender support for nsDisplayFilter

For future reference, you can just use the checkin-needed bug keyword when your patch is ready to land. It plays more nicely with the automated bug marking tools we use.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f781899dd025
Add initial WebRender support for nsDisplayFilter. r=rhunt, r=mattwoodrow

Comment 30

[Martin Robinson (email:[e-mail])]

@RyanVM Thanks! As this is my first go-around, I was confused between checkin flag and the checkin-needed keyword.

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f781899dd025