Bug 126056 – Get rid of USE(IOSURFACE_CANVAS_BACKING_STORE)

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Tim Horton]

PLATFORM(MAC) should use ENABLE(ACCELERATED_2D_CANVAS) to indicate that it will use an accelerated backing store for Canvas2D, just like all the other platforms.

It should *not* have its own separate and oddly named/placed USE().

I have a patch.

[Tim Horton]

<rdar://problem/15705228>

[Tim Horton]

Created attachment 219736 [details]
patch

I would like a review specifically from Simon for this one since he was just working in this area today and it's slightly wacky.

[Tim Horton]

Comment on attachment 219736 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219736&action=review

> Source/WebCore/html/HTMLCanvasElement.cpp:375
> +    auto compositingStrategy = canvasCompositingStrategy(*renderer());

This probably needs a USE(ACCELERATED_COMPOSITING) check and a renderer null check of some sort.

Also I wonder if canvasCompositingStrategy should be checking renderBox()->hasAcceleratedCompositing() in the !PLATFORM(MAC) case, because I'm not sure the non-Mac ports can have accelerated canvas if the renderer doesn't have a compositing layer (which Mac can do).

[Simon Fraser (smfr)]

Comment on attachment 219736 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219736&action=review

What about the WebKits prefs?

Please don't land this yet. I need to think about some parts when more awake.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1374
> +#if ENABLE(ACCELERATED_2D_CANVAS) && !PLATFORM(MAC) // mac doesn't need this because.... ?

Yeah, tricky. Would have to look at ImageBufferCG(Data) to see what's going on here.

[Myles C. Maxfield]

This has repercussions for https://bugs.webkit.org/show_bug.cgi?id=125477 (in-flight) as well

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 219736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219736&action=review
> 
> What about the WebKits prefs?

What do the WebKit prefs have to do with what's compiled in? Anyway, they already have the right name.

> Please don't land this yet. I need to think about some parts when more awake.

Yup.

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1374
> > +#if ENABLE(ACCELERATED_2D_CANVAS) && !PLATFORM(MAC) // mac doesn't need this because.... ?
> 
> Yeah, tricky. Would have to look at ImageBufferCG(Data) to see what's going on here.

Yeah. We'll think about this more.

[Tim Horton]

(In reply to comment #5)
> This has repercussions for https://bugs.webkit.org/show_bug.cgi?id=125477 (in-flight) as well

Besides merging repercussions, I don't think so.

[Tim Horton]

(In reply to comment #4)
> (From update of attachment 219736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219736&action=review
> 
> What about the WebKits prefs?

Simon points out canvasUsesAcceleratedDrawing and accelerated2dCanvasEnabled both exist in WebKit1. Since there's no reason historically that anyone would have used accelerated2dCanvasEnabled but canvasUsesAcceleratedDrawing is more likely, we should get rid of accelerated2dCanvasEnabled.

This is somewhat tangential since we choose the right preference in HTMLCanvasElement::shouldAccelerate, in this patch.

[Dirk Schulze]

(In reply to comment #8)
> (In reply to comment #4)
> > (From update of attachment 219736 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=219736&action=review
> > 
> > What about the WebKits prefs?
> 
> Simon points out canvasUsesAcceleratedDrawing and accelerated2dCanvasEnabled both exist in WebKit1. Since there's no reason historically that anyone would have used accelerated2dCanvasEnabled but canvasUsesAcceleratedDrawing is more likely, we should get rid of accelerated2dCanvasEnabled.
> 
> This is somewhat tangential since we choose the right preference in HTMLCanvasElement::shouldAccelerate, in this patch.

Any update on this patch?

[Tim Horton]

(In reply to comment #9)
> Any update on this patch?

No, IIRC it got confusing and I had other things to do :) You're welcome to pick it up but there is a high likelihood of breaking something even with care taken.

[Anne van Kesteren]

Pull request: https://github.com/WebKit/WebKit/pull/33108

[EWS]

Committed 283201@main (b45e213003d3): <https://commits.webkit.org/283201@main>

Reviewed commits have been landed. Closing PR #33108 and removing active labels.