RESOLVED FIXED 96851
[chromium] WebCompositorOutputSurface software API
https://bugs.webkit.org/show_bug.cgi?id=96851
Summary [chromium] WebCompositorOutputSurface software API
Alexandre Elias
Reported 2012-09-14 20:10:23 PDT
[chromium] WebCompositorOutputSurface software API
Attachments
Patch (5.24 KB, patch)
2012-09-14 20:14 PDT, Alexandre Elias
no flags
Patch (5.71 KB, patch)
2012-09-17 11:45 PDT, Alexandre Elias
no flags
Patch (5.76 KB, patch)
2012-09-17 12:07 PDT, Alexandre Elias
no flags
Alexandre Elias
Comment 1 2012-09-14 20:14:47 PDT
WebKit Review Bot
Comment 2 2012-09-14 20:18:42 PDT
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.
Alexandre Elias
Comment 3 2012-09-14 20:53:00 PDT
This small API change is blocking the rest of the software compositor from landing, so let's get it submitted first.
James Robinson
Comment 4 2012-09-16 21:56:17 PDT
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
Comment 5 2012-09-17 11:45:26 PDT
Alexandre Elias
Comment 6 2012-09-17 11:54:06 PDT
(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 7 2012-09-17 11:54:46 PDT
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
Comment 8 2012-09-17 12:07:46 PDT
Alexandre Elias
Comment 9 2012-09-17 12:08:47 PDT
OK, as discussed offline, going with WebCompositorSoftwareOutputDevice.
James Robinson
Comment 10 2012-09-17 13:40:28 PDT
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 11 2012-09-17 13:45:04 PDT
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 12 2012-09-17 14:04:34 PDT
Comment on attachment 164434 [details] Patch Clearing flags on attachment: 164434 Committed r128803: <http://trac.webkit.org/changeset/128803>
WebKit Review Bot
Comment 13 2012-09-17 14:04:39 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.