902651 - CanvasRenderingContext2D::Demote should preserve DrawTarget state

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The DrawTarget's transform and clip need to be copied to the new DrawTarget.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

An interesting case is when we push a few clips, Demote(), then pop a clip or two.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1)
> An interesting case is when we push a few clips, Demote(), then pop a clip
> or two.

I kinda wonder if we should add a mozDemote() method or something so we can explicitly demote in order to test this thing.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Restore the DrawTarget transform and clip when demoting

This also includes a mochitest and some machinery for demoting canvases from there

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

I don't really like the event hack. I think I'm going to add a method to CanvasRenderingContext2D.idl instead.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: better patch

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 793222 [details] [diff] [review]
better patch

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

Looks good, please find someone else to review the .webidl change though.

We still the actual copying of state patch too, right?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Restore the DrawTarget transform and clip when demoting

Indeed I forgot the part to actually restore the clip/transform was in a different patch. Fixed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 793489 [details] [diff] [review]
Restore the DrawTarget transform and clip when demoting

>+#if USE_SKIA_GPU

ifdef, right?

r=me on the binding part, assuming that was what you wanted me to review.  If it wasn't, please say what you _did_ want me to review.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 793489 [details] [diff] [review]
> Restore the DrawTarget transform and clip when demoting
> 
> >+#if USE_SKIA_GPU
> 
> ifdef, right?

Yup, good catch.

> 
> r=me on the binding part, assuming that was what you wanted me to review. 
> If it wasn't, please say what you _did_ want me to review.

Yes, that was the bit I wanted you to review. Thanks!

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 793489 [details] [diff] [review]
Restore the DrawTarget transform and clip when demoting

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

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +567,5 @@
>  
>      return CurrentState().font;
>    }
>  
>  #if USE_SKIA_GPU

#ifdef here too

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d76df67ac3b1

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(I fixed up all the s/if/ifdef/ stuff, thanks)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d76df67ac3b1