WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(5.71 KB, patch)
2012-09-17 11:45 PDT
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Patch
(5.76 KB, patch)
2012-09-17 12:07 PDT
,
Alexandre Elias
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Alexandre Elias
Comment 1
2012-09-14 20:14:47 PDT
Created
attachment 164269
[details]
Patch
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
Created
attachment 164427
[details]
Patch
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
Created
attachment 164434
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug