Bug 167096 – Move TextureMapper-specific logic out of GraphicsContext3DPrivate

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

Move TextureMapper-specific logic out of GraphicsContext3DPrivate

[Zan Dobersek]

Created attachment 298971 [details]
WIP

[WebKit Commit Bot]

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.

[Zan Dobersek]

(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?

[Alex Christensen]

I think this is a step in the right direction.  I think GraphicsContext3DPrivate is an unnecessary and excessive layer of abstraction.

[Zan Dobersek]

Created attachment 299350 [details]
Patch

[Zan Dobersek]

(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.

[Michael Catanzaro]

(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?

[Alex Christensen]

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.

[Zan Dobersek]

(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.

[Alex Christensen]

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.

[Zan Dobersek]

Created attachment 300605 [details]
Patch for landing

[Zan Dobersek]

Created attachment 300606 [details]
Patch for landing

[Zan Dobersek]

(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.

[Zan Dobersek]

(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.

[Zan Dobersek]

Comment on attachment 300606 [details]
Patch for landing

Clearing flags on attachment: 300606

Committed r211681: <http://trac.webkit.org/changeset/211681>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(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.

[Alex Christensen]

r211718