User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

Currently Webrender assumes(1,2) a EGL context will use GLES, but that doesn't need to be true. For bug 1474281 we need to differentiate more clearly.

1: https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1113
2: https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1178

Export the GL type (GL or GLES) from in the GL context and use that in webrender.

As I'm new to the firefox contribution workflow (and c++ and rust), please be generous with criticism and feedback. If there's anything, especially formal stuff, were I can improve, please let me know.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Comment on attachment 9075512 [details] [diff] [review]
export_gl_type_from_context.patch

Review of attachment 9075512 [details] [diff] [review]:

This looks sensible to me, but probably graphics people should review it.

Thanks for the fast reply!

In terms of process, the recommended way of submitting patches is via
phabricator:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html

Ah of course, will read into it.

::: gfx/gl/GLContextTypes.h
@@ +15,4 @@

class GLContext;

enum class GLContextType { Unknown, WGL, CGL, GLX, EGL, EAGL };
+enum class GLType { Unknown, GL, GLES };

Is Unknown used anywhere? If not probably should be removed.

I thought it might be common style, therefore blindly copied from above, but there shouldn't be any other cases. Would also allow us so set GL as default without overwriting the func for GLX/WGL/CGL. I'll wait for a full review, though, to not mess up things more.

Removed the gl bits as they duplicated existing functionality. Now it's webrender only.

(In reply to Jeff Gilbert [:jgilbert] from comment #6)

Comment on attachment 9075512 [details] [diff] [review]
export_gl_type_from_context.patch

Review of attachment 9075512 [details] [diff] [review]:

https://searchfox.org/mozilla-central/rev/
11712bd3ce7454923e5931fa92eaf9c01ef35a0a/gfx/gl/GLContext.h#252

Ouch, of course something like that existed already :)
Ok, so now it's only the webrender part. Jeff, could you have another look?

Incorporated suggestion Jeff Gilbert

Unused is_glcontext_egl() should be deleted from WebRenderBridgeParent.cpp and webrender_ffi.h as well, or not(?)
https://searchfox.org/mozilla-central/search?q=is_glcontext_egl&path=

Properly check for the GL type instead of assuming a EGL context implies GLES

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #11)

Unused is_glcontext_egl() should be deleted from WebRenderBridgeParent.cpp and webrender_ffi.h as well, or not(?)
https://searchfox.org/mozilla-central/search?q=is_glcontext_egl&path=

Right, it seems unlikely that it will be needed any more. Fixed in lates version

Thanks! How do we proceed with this? Can I set the 'checkin needed' flag and the patch will get pushed? Or does that only work for phabricator?

Nightly 70 begins on Monday. Let's wait until then, I guess.(?)

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #16)

Nightly 70 begins on Monday. Let's wait until then, I guess.(?)

That sounds right. But do I need to do something then or would you or somebody else push it? Sorry for these nooby questions, it's my first time landing a patch in FF :)

Me or somebody else with L3 access can push it, or you can set the checkin-needed flag on the bug and some sheriff will get to it.

Ah, right, waiting until 70.

Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e90032db12be
Webrender assumes that an EGL context uses GLES (but it can be GL, too). r=jgilbert

https://hg.mozilla.org/mozilla-central/rev/e90032db12be