Summary: | Move TextureMapper-specific logic out of GraphicsContext3DPrivate | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||||||
Component: | New Bugs | Assignee: | Zan Dobersek <zan> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, gyuyoung.kim, mcatanzaro, ossy, pvollan | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Zan Dobersek
2017-01-16 10:40:10 PST
Created attachment 298971 [details]
WIP
Attachment 298971 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #1) > Created attachment 298971 [details] > WIP Based on proposal from https://bugs.webkit.org/show_bug.cgi?id=166968#c4. Alex, I need some input -- this basically moves the GraphicsContext3DPrivate logic into a TextureMapper-specific class, leaving the former to only work with a GLContext object on non-TextureMapper ports that use GraphicsContext3DCairo. Is this the desirable way of getting PlatformLayer interface out of the GraphicsContext3DPrivate class? Can the GraphicsContext3DPrivate class be potentially removed? I think this is a step in the right direction. I think GraphicsContext3DPrivate is an unnecessary and excessive layer of abstraction. Created attachment 299350 [details]
Patch
(In reply to comment #5) > Created attachment 299350 [details] > Patch I'm uploading this up for review, but it might still need some additional tweaking for the EFL port. (In reply to comment #4) > I think this is a step in the right direction. I think > GraphicsContext3DPrivate is an unnecessary and excessive layer of > abstraction. Do you want to do the formal review? Comment on attachment 299350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=299350&action=review > Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:103 > +#if USE(TEXTURE_MAPPER) > + m_texmapLayer = std::make_unique<TextureMapperGC3DPlatformLayer>(*this, renderStyle); > +#else > + m_private = std::make_unique<GraphicsContext3DPrivate>(this, renderStyle); > +#endif Are there platforms that use Cairo but not TEXTURE_MAPPER? Maybe we could remove even more code. (In reply to comment #8) > Comment on attachment 299350 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=299350&action=review > > > Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:103 > > +#if USE(TEXTURE_MAPPER) > > + m_texmapLayer = std::make_unique<TextureMapperGC3DPlatformLayer>(*this, renderStyle); > > +#else > > + m_private = std::make_unique<GraphicsContext3DPrivate>(this, renderStyle); > > +#endif > > Are there platforms that use Cairo but not TEXTURE_MAPPER? Maybe we could > remove even more code. I would gladly remove it, but the EFL port still uses m_private and even implements a separate GraphicsContext3DPrivate class. I can polish this patch enough that it builds for the EFL port, and land it, but I won't have time to modify anything further on the EFL side of things. Ok, but I think it's probably a bad idea to wait for people to move EFL code to where it ought to be. Nobody is working on architecture improvements on EFL. Created attachment 300605 [details]
Patch for landing
Created attachment 300606 [details]
Patch for landing
(In reply to comment #12) > Created attachment 300606 [details] > Patch for landing I'll be landing this patch tomorrow, now that it builds on all platforms. I didn't go and rip out GraphicsContext3DPrivate completely since it's still used by PLATFORM(WIN) && !USE(CAIRO) code here: https://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp#L376 Mac's GraphicsContext3DPrivate is no-op and could be removed easily. The PLATFORM(WIN) && !USE(CAIRO) configuration will need a replacement. I'm CC-ing EFL people if they want to take a look, but at minimum the m_private member could be made EFL-specific. (In reply to comment #13) > I'm CC-ing EFL people if they want to take a look, but at minimum the > m_private member could be made EFL-specific. Comment on attachment 300606 [details] Patch for landing Clearing flags on attachment: 300606 Committed r211681: <http://trac.webkit.org/changeset/211681> All reviewed patches have been landed. Closing bug. (In reply to comment #15) > Comment on attachment 300606 [details] > Patch for landing > > Clearing flags on attachment: 300606 > > Committed r211681: <http://trac.webkit.org/changeset/211681> FYI, it broke the WinCairo build: ..\..\Source\WebCore\platform\graphics\texmap\TextureMapperGC3DPlatformLayer.cpp(110): error C2039: 'glBindFramebuffer': is not a member of '`global namespace'' ..\..\Source\WebCore\platform\graphics\texmap\TextureMapperGC3DPlatformLayer.cpp(110): error C3861: 'glBindFramebuffer': identifier not found ninja: build stopped: subcommand failed. |