Created attachment 830271 [details] [diff] [review] Remove mPlatformContext mPlatformContext is another example (after bug 814159) of a piece of actually useful code that was added to support some platform (I believe Maemo/Meego) but due to our general lack of design around GLContext/GLContextProvider, couldn't be added in a very clean way, and ended up cluttering central code used on other platforms, and being half-maintained by people not necessarily understanding what it was there for. mPlatformContext was used to deal with platforms that already have their own OpenGL context that we should use, as opposed to creating our own OpenGL context, for the Gecko compositor. So this patch removes it, as it was seriously getting in the way of cleaning up the mSurface stuff (bug 925608), but that doesn't mean that it was a bad idea. Also, the platforms that it was useful for were already not supported anymore at least since bug 814159. Ideally, I would like us to continue doing more cleanup, then doing the big overhaul of GLContext planned in bug 901498, then we'll be in a place where we can have such platform-specific paths in a maintainable way.
Attachment #830271 - Flags: review?(jgilbert)
Hmm with current implementation we still can embed Moz EGL context into abstract Toolkit EGL context, why do you want to kill that?
IRC conversation with Oleg: [15:40] <romaxa> bjacob_: ping [15:41] <bjacob_> romaxa: pong, saw your comment [15:42] <bjacob_> romaxa: i wanted to make this change as part of bigger changes simplifying stuff around GLContextEGL::mSurface. Concretely, would something end-user-visible be broken by this change? [15:42] <romaxa> bjacob_: without that code the only way to render gecko to another GL context is rendering via special RenderTarget bounded to FBO texture [15:43] <romaxa> bjacob_: which force me to do one intermediate copy [15:44] <romaxa> bjacob_: nothing will be broken for current products [15:44] <bjacob_> romaxa: can you remind me why the code dealing with the situation "The compositor should use an existing GL context instead of creating its own" is code in GLContext, instead of being code in the compositor? [15:45] <bjacob_> romaxa: what about current ports of firefox that are currently functional and that you're maintaining? Are they broken by this change? [15:45] <romaxa> bjacob_: how else you can create GLContext wrapped around third-party egl/glx context? [15:45] <bjacob_> romaxa: oh i see, the compositor doesn't want a raw OpenGL context, it wants a mozilla GLContext. That makes sense. [15:46] --> nattokirai has joined this channel (nattokirai@moz-5F250CCC.dynamic.ppp.asahi-net.or.jp). [15:46] <bjacob_> romaxa: this falls exactly in the typicaly thing that I want to make doable in easily and cleanly in bug 901498 [15:46] <firebot> Bug https://bugzilla.mozilla.org/show_bug.cgi?id=901498 nor, --, ---, bjacob, NEW, Make GLContext more modular, with a core doing only the OpenGL entry points, and any extra features [15:46] <romaxa> bjacob_: yep, original gl context created on toolkit side... with special widget impelementation EmbedWidget, I do create GLContext wrapper around platform context [15:47] --> kamidphish has joined this channel (textual@moz-6AB00DA7.tpgi.com.au). [15:48] <bjacob_> romaxa: i see. this makes sense. but a real clean way to do that will be provided by bug 901498. So I would like to know: is this change breaking things for you today, or are you concerned about the future here? [15:48] <romaxa> bjacob_: I can survive for now [15:48] <romaxa> bjacob_: if better solution would be available [15:49] --> cpearce has joined this channel (chatzilla@538BABFE.A073F3E.97BBD552.IP). [15:49] <bjacob_> romaxa: thanks. i would like us to get to 901498 asap, so that we can get past the rather nasty current situation where i have to throw valid code out the window to get us to a place where we can fix ourselves [15:49] <romaxa> bjacob_: ok [15:50] <bjacob_> romaxa: once i'm done landing 925608 and its dependencies, i really want to build the case for bug 901498 so that i'm allowed to spend some time on it... i believe that it is wanted for a wide variety of things, of which what we're discussing here is only one
Attachment #830271 - Flags: review?(jgilbert) → review+
Assignee: nobody → bjacob
Target Milestone: --- → mozilla28
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.