Closed Bug 1235494 Opened 5 years ago Closed 5 years ago

Optimize nsStyleImageLayers::Layer::CalcDifference


(Core :: Layout, defect)

Not set



Tracking Status
firefox50 --- fixed


(Reporter: u459114, Assigned: u459114)


(Blocks 1 open bug)



(1 file)

In bug 686281, comment 339, dbaron wrote:
> > +    NS_UpdateHint(hint, nsChangeHint_UpdateEffects);
> > +    NS_UpdateHint(hint, nsChangeHint_RepaintFrame);
> > +    // Mask changes require that we update the PreEffectsBBoxProperty,
> > +    // which is done during overflow computation.
> > +    NS_UpdateHint(hint, nsChangeHint_UpdateOverflow);
>Are CSS masks implemented differently enough at the rendering level that they >don't need the UpdateEffects or UpdateOverflow hints?  (I think that seems likely, but I'd like to make sure.)

In the case of raster-image-mask/ gradient-mask/ element-mask, we may skip UpdateEffects and UpdateOverflow.
The quoted code is in nsStyleImageLayers::Layer::CalcDifference.  It also interacts with nsStyleImageLayers::CalcDifference in somewhat interesting ways.

The code was introduced in .
If we know these two masks are both image masks, obviously we don't need nsChangeHint_UpdateEffect at all. And we don't need nsChangeHint_UpdateOverflow either, since an image mask is always inflow. 

Unfortunately, in nsStyleImageLayers::Layer::CalcDifference we do not know whether layers, to be compared, are image masks or svg masks, so these two hints are still required.

Here is an example why we can't figure out a layer is associate to a svg mask or an image layer.
mask-image: url("abc.svg#id"); // #id can be an id of a svg mask or a svg viewbox. 
                               // If it refers to an id of a svg mask, this mask-image 
                               // is bound to a svg mask.
                               // If it refers to an id of a svg viewbox, this mask-image
                               // is bound to a svg image.
We might do some optimization here. 
if Layer::mSourceURI contains no fragment, e.g. #mask, this layer must not be associated to a svg mask. In this case, skipping both nsChangeHint_UpdateOverflow & nsChangeHint_UpdateEffect should be fine.
Attachment #8772295 - Flags: review?(dbaron)
Comment on attachment 8772295 [details]
Bug 1235494 - Optimize nsStyleImageLayers::Layer::CalcDifference

::: layout/style/nsStyleStruct.cpp:2659
(Diff revision 2)
>  {
>    nsChangeHint hint = nsChangeHint(0);
>    if (!EqualURIs(mSourceURI, aNewLayer.mSourceURI)) {
> -    hint |= nsChangeHint_UpdateEffects |
> -            nsChangeHint_RepaintFrame;
> +    hint |= nsChangeHint_RepaintFrame;
> +
> +    // If Layer::mSourceURI refers to a SVG mask, it has a fragment. Not vice

"refers to" -> "links to"

::: layout/style/nsStyleStruct.cpp:2668
(Diff revision 2)
> +    //                           // element in a.svg.
> +    //   mask:url(#localMaskID); // The fragment of this URI is an ID of a mask
> +    //                           // element in local document.
> +    //   mask:url(b.svg#viewBoxID); // The fragment of this URI is an ID of a
> +    //                              // viewbox defined in b.svg.
> +    // That is, if mSourceURI has a fragment, it may refer a SVG mask; If not,

"may refer" -> "may link to"

::: layout/style/nsStyleStruct.cpp:2678
(Diff revision 2)
> +    // Do not take nsChangeHint_UpdateEffects & nsChangeHint_UpdateOverflow
> +    // into account if none of mSourceURI and aNewLayer.mSourceURI refers to
> +    // a SVG mask.

Please replace this comment with:

Return nsChangeHint_UpdateEffects and nsChangeHint_UpdateOverflow if either URI might refer to an SVG mask.
Attachment #8772295 - Flags: review?(dbaron) → review+
Comment on attachment 8772295 [details]
Bug 1235494 - Optimize nsStyleImageLayers::Layer::CalcDifference

Review request updated; see interdiff:
Pushed by
Optimize nsStyleImageLayers::Layer::CalcDifference r=dbaron
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.