[chromium] WebCompositorOutputSurface software API
Created attachment 164269 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
This small API change is blocking the rest of the software compositor from landing, so let's get it submitted first.
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
Created attachment 164427 [details] Patch
(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.
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.
Created attachment 164434 [details] Patch
OK, as discussed offline, going with WebCompositorSoftwareOutputDevice.
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.
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.
Comment on attachment 164434 [details] Patch Clearing flags on attachment: 164434 Committed r128803: <http://trac.webkit.org/changeset/128803>
All reviewed patches have been landed. Closing bug.