Closed Bug 1314536 Opened 8 years ago Closed 8 years ago

Return DrawResult from nsSVGMaskFrame::GetMaskForMaskedFrame

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1314536 - Part 2. Implement MixModeBlender to simplify nsSVGUtils::PaintFrameWithEffects.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1314536 - Part 1. Implement/use nsSVGMaskFrame::MaskParams and add a test case for nested mask-mode usage in SVG mask.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1314536 - Part 3. Correct comment and add a test case for it.
58 bytes, text/x-review-board-request
mstange
: review+
Details 
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.
Attachment #8811145 - Flags: review?(mstange)
Attachment #8811144 - Flags: review?(mstange)
Attachment #8812168 - Flags: review?(mstange)
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?
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 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 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 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+
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
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: