1073974 - Make SVG clipPath clipping use a Moz2D Path object directly

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Make simple SVG clipPath clipping use a Moz2D Path object directly

Comment 2

[Robert Longson [:longsonr]]

The trouble with NudgeToIntegers is that we end up with things like bug 1071534 	where there's a parent with some extreme scaling and a child with values close to zero but which are significant when scaled.

Comment 3

[Jonathan Watt [:jwatt]]

This is again maintaining the current behavior where currently under nsSVGPathGeometryFrame::GeneratePath this is what we do.

Also note that this is not going to cause bugs like bug 1071534, since this is operating on the _entire_ transform all the way back to device space (note the gfx->CurrentMatrix() call which we're prepending to, then nudging). In other words it's the initial translation (before all other transforms all the way down to the painting child) that is being nudged. We're not nudging intermediate transforms, which would indeed carry the risk you mention.

Comment 4

[Robert Longson [:longsonr]]

Comment on attachment 8496579 [details] [diff] [review]
part 1 - Make simple SVG clipPath clipping use a Moz2D Path object directly

>   gfxContext *gfx = aContext->ThebesContext();
> 
>   nsISVGChildFrame *singleClipPathChild = nullptr;
> 
...

>+    if (singleClipPathChild) {
>+      nsSVGPathGeometryFrame* pathFrame = do_QueryFrame(singleClipPathChild);

Other * are right hugging (see above). We should be consistent at least within
a single method. Can you make them all look the same please, ideally consistent with
what the majority of the existing code in this file does.

>     FillRule oldFillRull = gfx->CurrentFillRule();

Spelling! oldFillRule. I know this is existing code but it needs fixing.


>+    gfx->SetColor(gfxRGBA(1.0f, 1.0f, 1.0f, 1.0f));
>+    gfx->Fill();
>+    gfx->SetFillRule(oldFillRull); // restore, but only for CLIP_MASK

the comment is no longer true. Probably best just ditched.

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 8496579 [details] [diff] [review]
part 1 - Make simple SVG clipPath clipping use a Moz2D Path object directly

r=longsonr assuming review comments are addressed.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Make complex SVG clipPath clipping use a Moz2D Path object directly

Comment 7

[Robert Longson [:longsonr]]

Comment on attachment 8496587 [details] [diff] [review]
part 2 - Make complex SVG clipPath clipping use a Moz2D Path object directly

>+    RefPtr<PathBuilder> builder = aContext->GetDrawTarget()->
>+      CreatePathBuilder(nsSVGUtils::ToFillRule(StyleSVG()->mClipRule));

create a variable to hold the fill rule

>-
>-    if (StyleSVG()->mClipRule == NS_STYLE_FILL_RULE_EVENODD)
>-      gfx->SetFillRule(FillRule::FILL_EVEN_ODD);
>-    else
>-      gfx->SetFillRule(FillRule::FILL_WINDING);
>+    gfx->SetFillRule(nsSVGUtils::ToFillRule(StyleSVG()->mClipRule));

and use it here too.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #4)
> Other * are right hugging (see above). We should be consistent at least
> within
> a single method. Can you make them all look the same please, ideally
> consistent with
> what the majority of the existing code in this file does.

I personally prefer the existing variable name-hugging convention, but we're supposed to use type-hugging in new code we write:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

I'll change the local instances to do that.

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 8496579 [details] [diff] [review]
part 1 - Make simple SVG clipPath clipping use a Moz2D Path object directly

https://hg.mozilla.org/integration/mozilla-inbound/rev/22acc74cf08d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e31ac0e3997

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22acc74cf08d
https://hg.mozilla.org/mozilla-central/rev/3e31ac0e3997