Bug 96851 – [chromium] WebCompositorOutputSurface software API

• Component: New Bugs
• Status: RESOLVED FIXED

[Alexandre Elias]

[chromium] WebCompositorOutputSurface software API

[Alexandre Elias]

Created attachment 164269 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Alexandre Elias]

This small API change is blocking the rest of the software compositor from landing, so let's get it submitted first.

[James Robinson]

Comment on attachment 164269 [details]
Patch

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

> Source/Platform/chromium/public/WebCompositorOutputSurfaceSoftware.h:34
> +class WebCompositorOutputSurfaceSoftware {

could you add some brief class-level comments about what this is?

from the name i'm expecting this to be a subclass of WebCompositorOutputSurface

> Source/Platform/chromium/public/WebCompositorOutputSurfaceSoftware.h:36
> +    virtual ~WebCompositorOutputSurfaceSoftware() { }

public, or protected? who has ownership of this - the WebCompositorOuputSurface?

> Source/Platform/chromium/public/WebCompositorOutputSurfaceSoftware.h:41
> +    virtual WebImage* lockForWrite() = 0;
> +    virtual void unlockForWrite() = 0;
> +    virtual WebImage* lockForRead() = 0;
> +    virtual void unlockForRead() = 0;

can you document the requirements on this? in particular, can you lock a surface for both read and write at the same time?

> Source/Platform/chromium/public/WebCompositorOutputSurfaceSoftware.h:43
> +    virtual void viewportChanged(const WebSize& viewportSize) = 0;

i somewhat prefer passing WebSize by value - it's 2 ints, so on 64-bit platforms it's the same size as a reference and less generated code

[Alexandre Elias]

Created attachment 164427 [details]
Patch

[Alexandre Elias]

(In reply to comment #4)
> (From update of attachment 164269 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164269&action=review
> 
> > Source/Platform/chromium/public/WebCompositorOutputSurfaceSoftware.h:34
> > +class WebCompositorOutputSurfaceSoftware {
> 
> could you add some brief class-level comments about what this is?
> 
> from the name i'm expecting this to be a subclass of WebCompositorOutputSurface

OK, to clarify this, renamed to WebCompositorOutputContextSoftware by analogy with GraphicsContext3D.

> 
> > Source/Platform/chromium/public/WebCompositorOutputSurfaceSoftware.h:36
> > +    virtual ~WebCompositorOutputSurfaceSoftware() { }
> 
> public, or protected? who has ownership of this - the WebCompositorOuputSurface?

The ownership behavior isn't specified by the API base classes.  The shell implementations I wrote have it owned by the WebCompositorOutputSurface.

> 
> > Source/Platform/chromium/public/WebCompositorOutputSurfaceSoftware.h:41
> > +    virtual WebImage* lockForWrite() = 0;
> > +    virtual void unlockForWrite() = 0;
> > +    virtual WebImage* lockForRead() = 0;
> > +    virtual void unlockForRead() = 0;
> 
> can you document the requirements on this? in particular, can you lock a surface for both read and write at the same time?

Changed to single function WebImage* lock(bool forWrite) to clarify it should only be locked once.

> 
> > Source/Platform/chromium/public/WebCompositorOutputSurfaceSoftware.h:43
> > +    virtual void viewportChanged(const WebSize& viewportSize) = 0;
> 
> i somewhat prefer passing WebSize by value - it's 2 ints, so on 64-bit platforms it's the same size as a reference and less generated code

Done.

[Nat Duca]

Comment on attachment 164427 [details]
Patch

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

WebCompositorSoftwareOutputSurface plz?

> Source/Platform/chromium/public/WebCompositorOutputContextSoftware.h:37
> +class WebCompositorOutputContextSoftware {

Backwards this class name feels, yes?

> Source/Platform/chromium/public/WebCompositorOutputContextSoftware.h:47
> +    virtual void viewportChanged(WebSize viewportSize) = 0;

I'm not sure if we do this elsewhere, but sometimes I like using will/did to clarify when the function is called.

[Alexandre Elias]

Created attachment 164434 [details]
Patch

[Alexandre Elias]

OK, as discussed offline, going with WebCompositorSoftwareOutputDevice.

[James Robinson]

Comment on attachment 164434 [details]
Patch

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

> Source/Platform/chromium/public/WebCompositorSoftwareOutputDevice.h:44
> +    virtual WebImage* lock(bool forWrite) = 0;

It might be worth making this a 2-state enum - think about what the callsites will look like. It's up to you.

[Alexandre Elias]

Comment on attachment 164434 [details]
Patch

I'm not a big fan of two-state enums, and this is similar to an idiom in Skia, so committing as is.

[WebKit Review Bot]

Comment on attachment 164434 [details]
Patch

Clearing flags on attachment: 164434

Committed r128803: <http://trac.webkit.org/changeset/128803>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.