Summary: | Add ShareableSurface class | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Anders Carlsson <andersca> | ||||
Component: | New Bugs | Assignee: | Anders Carlsson <andersca> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Other | ||||||
OS: | OS X 10.5 | ||||||
Attachments: |
|
Description
Anders Carlsson
2011-01-28 10:56:23 PST
Created attachment 80466 [details]
Patch
Attachment 80466 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/Shared/mac/ShareableSurface.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Source/WebKit2/Shared/mac/ShareableSurface.cpp:139: cgl_ctx is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebKit2/Shared/mac/ShareableSurface.cpp:164: cgl_ctx is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebKit2/Shared/mac/ShareableSurface.cpp:181: cgl_ctx is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebKit2/Shared/mac/ShareableSurface.cpp:189: cgl_ctx is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/WebKit2/Shared/mac/ShareableSurface.h:39: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 6 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 80466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=80466&action=review > Source/WebKit2/Shared/mac/ShareableSurface.cpp:53 > + encoder->encode(CoreIPC::MachPort(m_port, MACH_MSG_TYPE_COPY_SEND)); MOVE_SEND! > Source/WebKit2/Shared/mac/ShareableSurface.cpp:74 > + unsigned pixelFormat = 'BGRA'; > + unsigned bytesPerElement = 4; > + int width = size.width(); > + int height = size.height(); I'd put these closer to where they're used. > Source/WebKit2/Shared/mac/ShareableSurface.cpp:97 > + values[4] = CFNumberCreate(0, kCFNumberLongType, &bytesPerRow); > + keys[5] = kIOSurfaceAllocSize; > + values[5] = CFNumberCreate(0, kCFNumberLongType, &allocSize); Should you use an unsigned long type? > Source/WebKit2/Shared/mac/ShareableSurface.cpp:100 > + RetainPtr<CFDictionaryRef> dictionary(AdoptCF, CFDictionaryCreate(0, keys, values, 6, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks)); > + for (unsigned i = 0; i < 6; i++) It's too bad we have to say "6" in 4 places overall. > Source/WebKit2/Shared/mac/ShareableSurface.cpp:117 > + ASSERT(handle.m_port != MACH_PORT_NULL); Could use ASSERT_ARG. > Source/WebKit2/Shared/mac/ShareableSurface.cpp:128 > +ShareableSurface::ShareableSurface(CGLContextObj cglContextObj, const IntSize& size, RetainPtr<IOSurfaceRef> ioSurface) You can just take a bare IOSurfaceRef. > Source/WebKit2/Shared/mac/ShareableSurface.cpp:139 > + CGLContextObj cgl_ctx = m_cglContextObj; Please add a comment about why this is needed. Maybe you can get away with a single comment somewhere high up in the file. Or do the magic needed to make it use m_cglContextObj directly. Or add a SET_CGLCONTEXT(m_cglContextObj) macro (or similar). > Source/WebKit2/Shared/mac/ShareableSurface.cpp:152 > + ASSERT(handle.m_port == MACH_PORT_NULL); ASSERT_ARG > Source/WebKit2/Shared/mac/ShareableSurface.cpp:168 > + glGenFramebuffersEXT(1, &m_frameBufferObjectID); Do we need to check whether this succeeded? > Source/WebKit2/Shared/mac/ShareableSurface.cpp:176 > + glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, 0); > + } > + > + glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_frameBufferObjectID); You could skip the unbind when we create the FBO, but it doesn't matter very much. > Source/WebKit2/Shared/mac/ShareableSurface.cpp:188 > + if (!m_textureID) { Early return! > Source/WebKit2/Shared/mac/ShareableSurface.cpp:192 > + glGenTextures(1, &m_textureID); Can this fail? > Source/WebKit2/Shared/mac/ShareableSurface.h:47 > + class Handle { Seems like we could share this with SharedMemory, at least on Mac. Committed r76974: <http://trac.webkit.org/changeset/76974> |