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.
<rdar://problem/15705228>
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.
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).
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.
This has repercussions for https://bugs.webkit.org/show_bug.cgi?id=125477 (in-flight) as well
(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.
(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.
(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.
(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?
(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.