Summary: | Redesign extension mechanism in GraphicsContext3D | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Marrin <cmarrin> | ||||||
Component: | WebGL | Assignee: | Kenneth Russell <kbr> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | amarinichev, enne, jamesr, senorblanco, vangelis, zmo | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Chris Marrin
2010-09-30 06:45:20 PDT
Created attachment 72785 [details]
Patch
From the ChangeLog:
Upon request, factored out extension support from GraphicsContext3D into a new Extensions3D class. (The plural was chosen because the class and subclasses hold multiple extensions.)
Unlike GraphicsContext3D, Extensions3D contains only pure virtual methods. This was done because Extensions3D's inheritance diagram and usage pattern is very different from that of GraphicsContext3D, and the concrete subclasses need to decide how to implement the various entry points. Requiring them to be placed at the Extensions3D level will cause implementation details to leak into the base class, which is highly undesirable. Any virtual call overhead to these entry points will be negligible.
Changed call sites utilizing these extensions to call through the Extensions3D object or its subclasses.
Tested:
- Chromium on Linux with accelerated 2D canvas and HTML5 video
- Chromium on Mac OS X with WebGL and CSS 3D content
- Safari on Mac OS X with WebGL and CSS 3D content
No new tests. Covered by existing tests.
Comment on attachment 72785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72785&action=review > WebCore/platform/graphics/chromium/Extensions3DChromium.h:62 > + Extensions3DChromium(GraphicsContext3DInternal*); nit: explicit This looks good to me. Do you have any thoughts about this approach, Chris? Created attachment 72872 [details]
Patch
Fixed style error in previous patch. Addressed review feedback.
View in context: https://bugs.webkit.org/attachment.cgi?id=72785&action=review Looks good except for some small comments. > WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:73 > + , m_fbo(0) Is this unrelated? If so, it should be in a separate patch > WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:116 > + static_cast<Extensions3DChromium*>(m_context->getExtensions())->copyTextureToParentTextureCHROMIUM(m_internal->offscreenColorTexture, parentTexture); So it looks here like you're using the extensons mechanism as a way to implement cross-platform extensions in a platform specific way, and to add platform specific extensions to the interface. That's fine, but you should document it in Extensions3D.h and the ChangeLog. You do have a sentence that refers to this, but it should be made more clear. > WebKit/chromium/src/Extensions3DChromium.cpp:1 > +/* Really odd that this in not in WebCore/platform/graphics/chromium Committed r71272: <http://trac.webkit.org/changeset/71272> (In reply to comment #5) > View in context: https://bugs.webkit.org/attachment.cgi?id=72785&action=review > > Looks good except for some small comments. Sorry, I looked at the wrong email indicating the review had been granted and didn't realize you wanted changes made. I've already landed this but will make changes in a follow up bug if you want. > > WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:73 > > + , m_fbo(0) > > Is this unrelated? If so, it should be in a separate patch It's related. The FBO is now only allocated if the extension which DrawingBufferChromium needs is available. > > WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:116 > > + static_cast<Extensions3DChromium*>(m_context->getExtensions())->copyTextureToParentTextureCHROMIUM(m_internal->offscreenColorTexture, parentTexture); > > So it looks here like you're using the extensons mechanism as a way to implement cross-platform extensions in a platform specific way, and to add platform specific extensions to the interface. That's fine, but you should document it in Extensions3D.h and the ChangeLog. You do have a sentence that refers to this, but it should be made more clear. I thought this was self-explanatory from your own request of how the refactoring was to work. I suspect that anyone looking at the code will be able to figure it out. If you feel strongly then file a bug and I'll update the documentation. > > WebKit/chromium/src/Extensions3DChromium.cpp:1 > > +/* > > Really odd that this in not in WebCore/platform/graphics/chromium It can not be placed in WebCore/platform/graphics/chromium due to how it's implemented. The implementation reaches into files that live in WebKit/chromium/src/. This is part of the reason for decoupling the interface and implementation at the Extensions3D level. |