Closed Bug 1276834 Opened 8 years ago Closed 8 years ago

Draw nothing when any mask-image refer to an unresolvable URL

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1258510

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1276834 - Part 1. Change return type of GenerateMaskSurface.
58 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1276834 - Part 2. Draw nothing when any mask-image refer to an unresolvable URL
58 bytes, text/x-review-board-request
Details 
Hint from mstange,
https://bugzilla.mozilla.org/show_bug.cgi?id=1275450#c10

Go back to this bug after finish bug 1272859.
Summary: Draw nothing when all mask-image refer to an unresolvable URL → Draw nothing when any mask-image refer to an unresolvable URL
Attachment #8759098 - Attachment is obsolete: true
Attachment #8759114 - Flags: review?(mstange)
Attachment #8759115 - Flags: review?(mstange)
Comment on attachment 8759114 [details]
Bug 1276834 - Part 1. Change return type of GenerateMaskSurface.

https://reviewboard.mozilla.org/r/57174/#review56580
Attachment #8759114 - Flags: review?(mstange) → review+
Comment on attachment 8759115 [details]
Bug 1276834 - Part 2. Draw nothing when any mask-image refer to an unresolvable URL

https://reviewboard.mozilla.org/r/57176/#review56582

::: layout/svg/nsSVGIntegrationUtils.cpp:525
(Diff revision 3)
>  
> +static bool
> +ShouldCreateMaskSurface(const nsIFrame* aFrame,
> +                        const nsStyleSVGReset* aSVGReset,
> +                        const nsTArray<nsSVGMaskFrame *>& maskFrames,
> +                        bool& aHasUnresovlableMaskLayer)

typo (should be aHasUnresolvableMaskLayer)

::: layout/svg/nsSVGIntegrationUtils.cpp:657
(Diff revision 3)
>  
>    gfxMatrix cssPxToDevPxMatrix = GetCSSPxToDevPxMatrix(frame);
>  
>    const nsStyleSVGReset *svgReset = firstFrame->StyleSVGReset();
>    nsTArray<nsSVGMaskFrame *> maskFrames = effectProperties.GetMaskFrames();
> -  // For a HTML doc:
> +  bool shouldCreateMaskSurface = false, hasUnresovlableMaskLayer = false;

Separate lines please. Also same typo in unresolvable.

::: layout/svg/nsSVGIntegrationUtils.cpp:658
(Diff revision 3)
>    gfxMatrix cssPxToDevPxMatrix = GetCSSPxToDevPxMatrix(frame);
>  
>    const nsStyleSVGReset *svgReset = firstFrame->StyleSVGReset();
>    nsTArray<nsSVGMaskFrame *> maskFrames = effectProperties.GetMaskFrames();
> -  // For a HTML doc:
> -  //   According to css-masking spec, always create a mask surface when we
> +  bool shouldCreateMaskSurface = false, hasUnresovlableMaskLayer = false;
> +  shouldCreateMaskSurface = ShouldCreateMaskSurface(frame, svgReset,

Actually, just declare this variable in this line. If that causes the line to exceed the line length limit, just break after the equals sign.

::: layout/svg/nsSVGIntegrationUtils.cpp:670
(Diff revision 3)
> +    gfxContextAutoSaveRestore saver(&context);
> +
> +    context.SetColor(Color(0.f, 0.f, 0.f, 0.f));
> +    context.Paint();

How is this different from just returning?
Attachment #8759115 - Flags: review?(mstange)
https://reviewboard.mozilla.org/r/57176/#review56582

> How is this different from just returning?

Then it looks like no mask effect at all, you can still see whole background image, which is not we want.
Status: NEW → ASSIGNED
Depends on: mask-ship
Sorry, I still don't really understand. I could try out the patch but I'd rather ask for an explanation one more time.

Is this patch just an optimization, so that we don't create an unnecessary surface? Or does it change behavior? I'm assuming the former, otherwise I'd expect tests to be added.

And I still don't understand why these lines would have any effect:

    context.SetColor(Color(0.f, 0.f, 0.f, 0.f));
    context.Paint();

Are we not using operator OVER here?
(In reply to Markus Stange [:mstange] from comment #12)
> Sorry, I still don't really understand. I could try out the patch but I'd
> rather ask for an explanation one more time.
> 
> Is this patch just an optimization, so that we don't create an unnecessary
> surface? Or does it change behavior? I'm assuming the former, otherwise I'd
> expect tests to be added.
Yes, the former
> And I still don't understand why these lines would have any effect:
> 
>     context.SetColor(Color(0.f, 0.f, 0.f, 0.f));
>     context.Paint();
> 
> Are we not using operator OVER here?
Yes, it need to be OVER. I should call SetOp(OVER) before Paint(). In my test, it works fine since default CompOp is OVER.
And we already have a test case to verify this scenario, which is mask-image-4a.html
You still haven't answered the question. If the operator is OVER, then how is painting "nothing" on top of the existing contents different from skipping those two lines?
(In reply to Markus Stange [:mstange] from comment #15)
> You still haven't answered the question. If the operator is OVER, then how
> is painting "nothing" on top of the existing contents different from
> skipping those two lines?
I read nsSVGIntegrationUtils::PaintFramesWithEffects again and know I make a mistake here.
context is empty unless we paint on it. I used to think that "context" contains content of background-image, which is wrong. I am rewriting this patch
Comment on attachment 8759115 [details]
Bug 1276834 - Part 2. Draw nothing when any mask-image refer to an unresolvable URL

https://reviewboard.mozilla.org/r/57176/#review60808

::: layout/svg/nsSVGIntegrationUtils.cpp:760
(Diff revision 5)
>                                          maskFrames, offsetToUserSpace,
>                                          maskTransform);
> -      if (!maskSurface) {
> -        // Entire surface is clipped out.
> +      if (!maskSurface || hasUnresolvableMaskLayer) {
> +        // Entire surface is clipped out or we have non-resolvable masks.
> +        // Check hasUnresolvableMaskLayer after calling GenerateMaskSurface
> +        // to ensure we get invalidated for loads of the mask.

How does this work? Why are we not getting invalidated for loads of the mask if we don't call GenerateMaskSurface?
Attachment #8759115 - Flags: review?(mstange)
Patch in Bug 1258510 already did what we need here. 
Early return without push any mask:
https://dxr.mozilla.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp?q=PaintFramesWithEffects&redirect_type=direct#584
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: