Closed Bug 932500 Opened 6 years ago Closed 6 years ago

PathD2D::TransformedCopyToBuilder ignores even odd fill

Categories

(Core :: Graphics, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: cabanier, Assigned: cabanier)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I noticed that TransformedCopyToBuilder with even odd winding returns a path that is still using nzo winding.
This looks like a small oversight in the TransformedCopyToBuilder function.
Attached patch Add case for even odd winding (obsolete) — Splinter Review
Assignee: nobody → cabanier
Attachment #824268 - Flags: review?(bas)
OS: Mac OS X → Windows 7
Blocks: 931587
Attachment #824268 - Flags: review?(bas) → review?(jmuizelaar)
Comment on attachment 824268 [details] [diff] [review]
Add case for even odd winding

Review of attachment 824268 [details] [diff] [review]:
-----------------------------------------------------------------

The default is ALTERNATE. Does this patch have a behaviour change?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3)
> Comment on attachment 824268 [details] [diff] [review]
> Add case for even odd winding
> 
> Review of attachment 824268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The default is ALTERNATE. Does this patch have a behaviour change?

Well, it fixes when you go from D2D1_FILL_MODE_ALTERNATE to D2D1_FILL_MODE_WINDING which was ignored before.
I could add an assert that checks if the result of TransformedCopyToBuilder doesn't give the correct winding back.
Attachment #824268 - Flags: review?(jmuizelaar) → review?(bas)
Comment on attachment 824268 [details] [diff] [review]
Add case for even odd winding

Review of attachment 824268 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/PathD2D.cpp
@@ +268,5 @@
>  
>    if (aFillRule == FILL_WINDING) {
>      sink->SetFillMode(D2D1_FILL_MODE_WINDING);
> +  } else {
> +    sink->SetFillMode(D2D1_FILL_MODE_ALTERNATE);

This is unneeded as ALTERNATE is the default for a new sink. We should probably just remove this line.

@@ +282,5 @@
>                          D2DMatrix(aTransform),
>                          sink);
>    }
>  
> +  RefPtr<PathBuilderD2D> pathBuilder = new PathBuilderD2D(sink, path, aFillRule);

Good catch.
Attachment #824268 - Flags: review?(bas) → review+
Attachment #824268 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0308e23bc0ab
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.