Return DrawResult from nsSVGMaskFrame::GetMaskForMaskedFrame

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: u459114, Assigned: u459114)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
The caller of nsSVGMaskFrame::GetMaskForMaskedFrame(e.g. GenerateMaskSurface in nsSVGIntegrationUtils.cpp) has no idea how to react when GetMaskForMaskedFrame return a nullptr. Without this information, nsDisplayMask does not know how to update nsDisplayMaskGeometry in this case.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8811145 - Flags: review?(mstange)
Attachment #8811144 - Flags: review?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8812168 - Flags: review?(mstange)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8811145 [details]
Bug 1314536 - Part 1. Implement/use nsSVGMaskFrame::MaskParams and add a test case for nested mask-mode usage in SVG mask.

https://reviewboard.mozilla.org/r/93360/#review94224

::: layout/svg/nsSVGIntegrationUtils.cpp:538
(Diff revision 6)
>      gfxMatrix cssPxToDevPxMatrix =
>      nsSVGIntegrationUtils::GetCSSPxToDevPxMatrix(aParams.frame);
> -
> +    nsSVGMaskFrame::MaskParams params(&ctx, aParams.frame, cssPxToDevPxMatrix,
> +                                      aOpacity, &aOutMaskTransform,
> +                                      svgReset->mMask.mLayers[0].mMaskMode,
> +                                      result);

Hmm, this looks a little confusing. Reading this code I wouldn't expect "result" to be modified.

I search for DrawResult and found ClippedImage::GetFrameInternal, which returns a Pair<DrawResult, RefPtr<SourceSurface>> and then does this on the caller side:

DrawResult result;
RefPtr<SourceSurface> surface;
Tie(result, surface) = GetFrameInternal(...);

I think that's a lot nicer to read.

The only problem with that is that we have another outparam: the mask transform. Maybe that should stay an outparam. So then you'd have:

    nsSVGMaskFrame::MaskParams params(&ctx, aParams.frame, cssPxToDevPxMatrix,
                                      aOpacity,
                                      svgReset->mMask.mLayers[0].mMaskMode);

    Tie(result, aOutMaskSurface) =
      aMaskFrames[0]->GetMaskForMaskedFrame(params, &aOutMaskTransform);
    return result;

How does that sound?

::: layout/svg/nsSVGUtils.cpp:758
(Diff revision 6)
> -      maskSurface =
> -        maskFrame->GetMaskForMaskedFrame(&aContext, aFrame, aTransform,
> +      uint8_t maskMode =
> +        aFrame->StyleSVGReset()->mMask.mLayers[0].mMaskMode;

You weren't passing a mask mode before, so GetMaskForMaskedFrame was using the default value NS_STYLE_MASK_MODE_MATCH_SOURCE. Was that a bug?
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8811145 [details]
Bug 1314536 - Part 1. Implement/use nsSVGMaskFrame::MaskParams and add a test case for nested mask-mode usage in SVG mask.

https://reviewboard.mozilla.org/r/93360/#review94224

> Hmm, this looks a little confusing. Reading this code I wouldn't expect "result" to be modified.
> 
> I search for DrawResult and found ClippedImage::GetFrameInternal, which returns a Pair<DrawResult, RefPtr<SourceSurface>> and then does this on the caller side:
> 
> DrawResult result;
> RefPtr<SourceSurface> surface;
> Tie(result, surface) = GetFrameInternal(...);
> 
> I think that's a lot nicer to read.
> 
> The only problem with that is that we have another outparam: the mask transform. Maybe that should stay an outparam. So then you'd have:
> 
>     nsSVGMaskFrame::MaskParams params(&ctx, aParams.frame, cssPxToDevPxMatrix,
>                                       aOpacity,
>                                       svgReset->mMask.mLayers[0].mMaskMode);
> 
>     Tie(result, aOutMaskSurface) =
>       aMaskFrames[0]->GetMaskForMaskedFrame(params, &aOutMaskTransform);
>     return result;
> 
> How does that sound?

Hmm, use tuple sound great, will do

> You weren't passing a mask mode before, so GetMaskForMaskedFrame was using the default value NS_STYLE_MASK_MODE_MATCH_SOURCE. Was that a bug?

Yes, that's. svg mask need also respect mask-mode. I should create a test case for this - put a mask in a mask, and change mask-mode of that nested mask.

Comment 16

2 years ago
mozreview-review
Comment on attachment 8812168 [details]
Bug 1314536 - Part 3. Correct comment and add a test case for it.

https://reviewboard.mozilla.org/r/94022/#review94238
Attachment #8812168 - Flags: review?(mstange) → review+

Comment 17

2 years ago
mozreview-review
Comment on attachment 8811144 [details]
Bug 1314536 - Part 2. Implement MixModeBlender to simplify nsSVGUtils::PaintFrameWithEffects.

https://reviewboard.mozilla.org/r/93358/#review94248

Looks good. It seems completely unrelated to this bug though :-)
Attachment #8811144 - Flags: review?(mstange) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8811145 [details]
Bug 1314536 - Part 1. Implement/use nsSVGMaskFrame::MaskParams and add a test case for nested mask-mode usage in SVG mask.

https://reviewboard.mozilla.org/r/93360/#review94292
Attachment #8811145 - Flags: review?(mstange) → review+

Comment 25

2 years ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b0aa89236bb2
Part 1. Implement/use nsSVGMaskFrame::MaskParams and add a test case for nested mask-mode usage in SVG mask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/2effc920decf
Part 2. Implement MixModeBlender to simplify nsSVGUtils::PaintFrameWithEffects. r=mstange
https://hg.mozilla.org/integration/autoland/rev/16632b726b19
Part 3. Correct comment and add a test case for it. r=mstange
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

2 years ago
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05ad109be9a9
Part 1. Implement/use nsSVGMaskFrame::MaskParams and add a test case for nested mask-mode usage in SVG mask. r=mstange
https://hg.mozilla.org/integration/autoland/rev/68670da6a10d
Part 2. Implement MixModeBlender to simplify nsSVGUtils::PaintFrameWithEffects. r=mstange
https://hg.mozilla.org/integration/autoland/rev/7ecf2330d1d8
Part 3. Correct comment and add a test case for it. r=mstange

Comment 34

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/05ad109be9a9
https://hg.mozilla.org/mozilla-central/rev/68670da6a10d
https://hg.mozilla.org/mozilla-central/rev/7ecf2330d1d8
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.