Closed Bug 1314536 Opened 9 years ago Closed 9 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: