Bug 64214 – [chromium] Delete the unused legacy accelerated canvas 2d code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Delete the unused legacy accelerated canvas 2d code

[James Robinson]

Created attachment 100182 [details]
Patch

[James Robinson]

Note that I've removed the LoopBlinn code from all the project files, but I haven't deleted the files yet since the skia guys still may want to reference it.  I filed https://bugs.webkit.org/show_bug.cgi?id=64216 to cover deleting that code from the repo.

Diffstat for this patch:
 36 files changed, 256 insertions(+), 2930 deletions(-)

[James Robinson]

Created attachment 100184 [details]
merged up to ToT

[Stephen White]

Comment on attachment 100182 [details]
Patch

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

Looks good for the most part.  I'd like to get an ok from other ports on the GraphicsContext3D changes before I give it an r+.

> Source/WebCore/page/Page.h:269
> +        GraphicsContext3D* sharedGraphicsContext3D();

Should be renamed graphicsContext3D().

Also, since the calls are no longer going through GraphicsContext3D in Chrome, and I doubt they will on any other platform either, I'd prefer that we create and own a GrContext (or another wrapper class unrelated to GraphicsCOntext3D) directly here, via a platform-specific typedef.  That could be done in a later patch, though.

> Source/WebCore/page/Page.h:328
> +        RefPtr<GraphicsContext3D> m_sharedGraphicsContext3D;

s/sharedG/g/

> Source/WebCore/platform/graphics/GraphicsContext.cpp:710
> +void GraphicsContext::setSharedGraphicsContext3D(GraphicsContext3D*, DrawingBuffer*, const IntSize&)

s/Shared//

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:1240
> +void GraphicsContext::syncSoftwareCanvas()

Should we rename this function too?  Maybe willDraw() or something?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:691
> +void PlatformContextSkia::setSharedGraphicsContext3D(GraphicsContext3D* context, DrawingBuffer* drawingBuffer, const WebCore::IntSize& size)

s/Shared//

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:187
> +    void setSharedGraphicsContext3D(GraphicsContext3D*, DrawingBuffer*, const IntSize&);

s/Shared//

[James Robinson]

(In reply to comment #4)
> (From update of attachment 100182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100182&action=review
> 
> Looks good for the most part.  I'd like to get an ok from other ports on the GraphicsContext3D changes before I give it an r+.
> 
> > Source/WebCore/page/Page.h:269
> > +        GraphicsContext3D* sharedGraphicsContext3D();
> 
> Should be renamed graphicsContext3D().

Sure

> 
> Also, since the calls are no longer going through GraphicsContext3D in Chrome, and I doubt they will on any other platform either, I'd prefer that we create and own a GrContext (or another wrapper class unrelated to GraphicsCOntext3D) directly here, via a platform-specific typedef.  That could be done in a later patch, though.

Yeah, it'd be nice to clean this up.  One difficulty is that a GrContext doesn't actually hold a GL context, it needs to live on top of one.  I agree that we should clean this up in a follow-up patch.

> > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:1240
> > +void GraphicsContext::syncSoftwareCanvas()
> 
> Should we rename this function too?  Maybe willDraw() or something?

That could work.  What this really does it make the GraphicsContext3D associated with the GrContext current, which arguably should be done by the implementation of GraphicsContext, not by the caller.  I'll see if I can reasonably just get rid of this call completely.

[James Robinson]

Created attachment 100195 [details]
Patch

[James Robinson]

Turns out syncSoftwareCanvas() can go away completely if I set the context to current in ImageBufferSkia's draw() calls.  The only changes to GraphicsContext in this patch are removing now-unnecessary calls and renaming setSharedGraphicsContext3D()->setGraphicsContext3D(), so it should be pretty uncontroversial.

I removed the 'Shared' from interface names everywhere except for on Page because I think it's helpful to indicate that the context is supposed to be shared by everyone using that Page.  We can definitely rename it to something better at the same time that we change the type to something better, but for now I'd rather leave it alone.

[Stephen White]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 100182 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=100182&action=review
> > 
> > Looks good for the most part.  I'd like to get an ok from other ports on the GraphicsContext3D changes before I give it an r+.
> > 
> > > Source/WebCore/page/Page.h:269
> > > +        GraphicsContext3D* sharedGraphicsContext3D();
> > 
> > Should be renamed graphicsContext3D().
> 
> Sure
> 
> > 
> > Also, since the calls are no longer going through GraphicsContext3D in Chrome, and I doubt they will on any other platform either, I'd prefer that we create and own a GrContext (or another wrapper class unrelated to GraphicsCOntext3D) directly here, via a platform-specific typedef.  That could be done in a later patch, though.
> 
> Yeah, it'd be nice to clean this up.  One difficulty is that a GrContext doesn't actually hold a GL context, it needs to live on top of one.  I agree that we should clean this up in a follow-up patch.

What we really need is some kind of bare-bones class to represent the context used for the command buffer stubs, since that's what Ganesh is calling.  I was going to suggest we go back to using GLES2Context, but it seems that it was removed from the WebKit API long ago.

> > > Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:1240
> > > +void GraphicsContext::syncSoftwareCanvas()
> > 
> > Should we rename this function too?  Maybe willDraw() or something?
> 
> That could work.  What this really does it make the GraphicsContext3D associated with the GrContext current, which arguably should be done by the implementation of GraphicsContext, not by the caller.  I'll see if I can reasonably just get rid of this call completely.

[Stephen White]

Comment on attachment 100195 [details]
Patch

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

Not your fault, but IWBN if SkGpuDevice could handle all the makeCurrent()'s internally instead of GraphicsContextSkia.cpp having to do it.

Looks ok.  r=me

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:119
> +    context->platformContext()->makeGrContextCurrent();

Are you sure this one is still needed?  useGPU() is always false for Ganesh, so this whole clause should probably go away.

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:127
> +    context->platformContext()->makeGrContextCurrent();

This wasn't here before.. was this a bug?  Or is the lower level no longer doing a makeGrContextCurrent?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:74
>          SkiaGPU

Nit:  could be demoted to a bool, or perhaps even removed (m_gpuContext != NULL could be used, I think).

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:183
>      bool canAccelerate() const;

Do we still need this?

[Stephen White]

(In reply to comment #9)
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:127
> > +    context->platformContext()->makeGrContextCurrent();
> 
> This wasn't here before.. was this a bug?  Or is the lower level no longer doing a makeGrContextCurrent?

Ignore this.. I see this is related to your comment that "Turns out syncSoftwareCanvas() can go away completely if I set the context to current in ImageBufferSkia's draw() calls."

[James Robinson]

(In reply to comment #9)
> (From update of attachment 100195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100195&action=review
> 
> Not your fault, but IWBN if SkGpuDevice could handle all the makeCurrent()'s internally instead of GraphicsContextSkia.cpp having to do it.

YES!  The way this was explained to me many moons ago was that SkGpuDevice had no notion of what a GL context was, so it was the responsibility of the caller to set the right context current before making any skia calls that might make GL calls.  I think it's a big pain in the butt.

> 
> Looks ok.  r=me
> 
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:119
> > +    context->platformContext()->makeGrContextCurrent();
> 
> Are you sure this one is still needed?  useGPU() is always false for Ganesh, so this whole clause should probably go away.
> 
> > Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:127
> > +    context->platformContext()->makeGrContextCurrent();
> 
> This wasn't here before.. was this a bug?  Or is the lower level no longer doing a makeGrContextCurrent?

It was essentially a bug, the syncSoftwareCanvas() call was setting the correct context previously.

> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:74
> >          SkiaGPU
> 
> Nit:  could be demoted to a bool, or perhaps even removed (m_gpuContext != NULL could be used, I think).

Removed the enum, changed useSkiaGPU() to just null-test m_gpuContext.

> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:183
> >      bool canAccelerate() const;
> 
> Do we still need this?

Nope, nuking.

[James Robinson]

Committed r90872: <http://trac.webkit.org/changeset/90872>