625508 - Rework OpenGL blacklisting

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Move context creation out of LayerManagerOGL::Initialize()

- Removes support for reinitialization.
- Moves context creation up to the callers.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Moves the GfxInfo checks out of the layermanager

This moves all the GfxInfo checks out of the layermanager:
 - this makes the blacklist used on android
 - moves all the checks about whether or not to use OpenGL into one place.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 503641 [details] [diff] [review]
Move context creation out of LayerManagerOGL::Initialize()

Looks fine, but:

>+  if (PR_GetEnv("MOZ_LAYERS_PREFER_EGL")) {
>+    printf_stderr("Trying GL layers...\n");

Get rid of this printf_stderr, no need for it

>+        if (layerManager->Initialize(layerManager->CreateContext())) {

I don't really like this usage.  Why not just have Initialize() call CreateContext if it's passed a default-null arg?  It's much simpler and easier to read.  Also, unless you're planning on using CreateContext elsewhere, I'd make it protected (given that it creates a context for the mWindow widget and calling it twice could case badness.  No reason to have it be public that I can think of..

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #3)
> >+        if (layerManager->Initialize(layerManager->CreateContext())) {
> 
> I don't really like this usage.  Why not just have Initialize() call
> CreateContext if it's passed a default-null arg?  It's much simpler and easier
> to read.  Also, unless you're planning on using CreateContext elsewhere, I'd
> make it protected (given that it creates a context for the mWindow widget and
> calling it twice could case badness.  No reason to have it be public that I can
> think of..

I agree. I think this way made more sense in a previous revision.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: v2

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 503733 [details] [diff] [review]
v2

looks good, but you don't want to make CreateContext() protected?

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 503646 [details] [diff] [review]
Moves the GfxInfo checks out of the layermanager

+          if (layerManager->Initialize(LayerManagerOGL::CreateContext())) {

doesn't need to have the CreateContext() in here, given the rework of the other patch

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #6)
> Comment on attachment 503733 [details] [diff] [review]
> v2
> 
> looks good, but you don't want to make CreateContext() protected?

Why prefer protected over private?

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

er sure, private's fine too.. but it's neither :-)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

http://hg.mozilla.org/mozilla-central/rev/73fee13f0df3
http://hg.mozilla.org/mozilla-central/rev/5b07bfda5a41