Make SVG clipPath clipping use a Moz2D Path object directly

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Assignee

Description

5 years ago
No description provided.
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.
Assignee

Comment 3

5 years ago
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.
Assignee

Updated

5 years ago
Attachment #8496579 - Attachment description: patch → part 1 - Make simple SVG clipPath clipping use a Moz2D Path object directly
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 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.
Attachment #8496579 - Flags: review?(longsonr) → review+
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.
Attachment #8496587 - Flags: review?(longsonr) → review+
Assignee

Comment 8

5 years ago
(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.
Assignee

Updated

5 years ago
Attachment #8496587 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/22acc74cf08d
https://hg.mozilla.org/mozilla-central/rev/3e31ac0e3997
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1501057
You need to log in before you can comment on or make changes to this bug.