Bug 179967 – [CoordGraphics] Replace CoordinatedSurface, ThreadSafeCoordinatedSurface with CoordinatedBuffer

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

[Zan Dobersek]

[CoordGraphics] Replace CoordinatedSurface, ThreadSafeCoordinatedSurface with CoordinatedBuffer

[Zan Dobersek]

Created attachment 327485 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 327485 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +
> +        
> +

Remove extra empty lines here, please.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedBuffer.cpp:53
> +void CoordinatedBuffer::paintToSurface(const IntRect& rect, Client& client)

I'm confused about this, what surface is this painting to? Maybe we could rename it to paintToBuffer now, or simply paint() since buffer is this, no?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedBuffer.cpp:59
> +    client.paintToSurfaceContext(context);

I would also rename this to paintToContext()

> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/UpdateAtlas.cpp:105
>      UpdateAtlasSurfaceClient surfaceClient(client, size, supportsAlpha());
> -    m_surface->paintToSurface(rect, surfaceClient);
> +    m_buffer->paintToSurface(rect, surfaceClient);

Wow, this CoordinatedBuffer::Client is a bit messy. Here we have UpdateAtlasSurfaceClient which is a CoordinatedBuffer::Client but receives another CoordinatedBuffer::Client as parameter. Could we avoid this client mess by simply using lambdas to render into the context? CoordinatedBuffer::paint() could receive a Function <void (GraphicsContext&)>. I'm not suggesting to do that in this patch, just wondering if that would be possible.

[Zan Dobersek]

Comment on attachment 327485 [details]
Patch

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

>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedBuffer.cpp:53
>> +void CoordinatedBuffer::paintToSurface(const IntRect& rect, Client& client)
> 
> I'm confused about this, what surface is this painting to? Maybe we could rename it to paintToBuffer now, or simply paint() since buffer is this, no?

I have this and other "paint to surface" methods removed in the stashed work. That's why I didn't bother renaming them.

>> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/UpdateAtlas.cpp:105
>> +    m_buffer->paintToSurface(rect, surfaceClient);
> 
> Wow, this CoordinatedBuffer::Client is a bit messy. Here we have UpdateAtlasSurfaceClient which is a CoordinatedBuffer::Client but receives another CoordinatedBuffer::Client as parameter. Could we avoid this client mess by simply using lambdas to render into the context? CoordinatedBuffer::paint() could receive a Function <void (GraphicsContext&)>. I'm not suggesting to do that in this patch, just wondering if that would be possible.

This will also be simplified to the point where CoordinatedBuffer::Client will be possible to remove.

[Zan Dobersek]

Committed r225108: <https://trac.webkit.org/changeset/225108>

[Radar WebKit Bug Importer]

<rdar://problem/35674166>

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #3)
> Comment on attachment 327485 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327485&action=review
> 
> >> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedBuffer.cpp:53
> >> +void CoordinatedBuffer::paintToSurface(const IntRect& rect, Client& client)
> > 
> > I'm confused about this, what surface is this painting to? Maybe we could rename it to paintToBuffer now, or simply paint() since buffer is this, no?
> 
> I have this and other "paint to surface" methods removed in the stashed
> work. That's why I didn't bother renaming them.
> 
> >> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/UpdateAtlas.cpp:105
> >> +    m_buffer->paintToSurface(rect, surfaceClient);
> > 
> > Wow, this CoordinatedBuffer::Client is a bit messy. Here we have UpdateAtlasSurfaceClient which is a CoordinatedBuffer::Client but receives another CoordinatedBuffer::Client as parameter. Could we avoid this client mess by simply using lambdas to render into the context? CoordinatedBuffer::paint() could receive a Function <void (GraphicsContext&)>. I'm not suggesting to do that in this patch, just wondering if that would be possible.
> 
> This will also be simplified to the point where CoordinatedBuffer::Client
> will be possible to remove.

\o/