Closed Bug 625508 Opened 9 years ago Closed 9 years ago

Rework OpenGL blacklisting

Categories

(Core :: Graphics, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
- Removes support for reinitialization.
- Moves context creation up to the callers.
Attachment #503641 - Flags: review?(vladimir)
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.
OS: Mac OS X → All
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..
(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.
Attached patch v2Splinter Review
Attachment #503641 - Attachment is obsolete: true
Attachment #503733 - Flags: review?(vladimir)
Attachment #503641 - Flags: review?(vladimir)
Attachment #503646 - Flags: review?(vladimir)
Comment on attachment 503733 [details] [diff] [review]
v2

looks good, but you don't want to make CreateContext() protected?
Attachment #503733 - Flags: review?(vladimir) → review+
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
Attachment #503646 - Flags: review?(vladimir) → review+
(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?
er sure, private's fine too.. but it's neither :-)
Assignee: nobody → jmuizelaar
You need to log in before you can comment on or make changes to this bug.