Clean up the GLContextProvider classes

RESOLVED FIXED in mozilla14

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gw280, Assigned: gw280)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

The source structure for these files is a mess. We should actively start trying to clean them up.
A couple of thoughts already:

GLContext is currently a subclass of LibrarySymbolLoader. This doesn't make much sense; it should probably be a composition instead.

There's a huge embedded class declaration, EGLLibrary, in GLContextProviderEGL.cpp that really should be split off into its own file. Worse yet, it handles stuff like EGL symbol loading and stuff, but doesn't inherit from LibrarySymbolLoader, but instead manages the library loading itself then uses a bunch of statics in LibrarySymbolLoader. I'm not sure why we can't just use LibrarySymbolLoader itself, or at least subclass it.
Created attachment 603052 [details] [diff] [review]
Bug 732865 - Refactor GLContext classes so that they are more self contained

Review: :joedrew!
Attachment #603052 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #603052 - Flags: review?(joe)
Attachment #603052 - Flags: review?(bgirard)
Attachment #603052 - Flags: review?
This patch does the following:

- Renames LibrarySymbolLoader to GLLibraryLoader
- Moves it into GLLibraryLoader.h/cpp
- Moves EGLLibrary out of GLContextProviderEGL.cpp and into GLLibraryEGL.cpp
- Renames EGLLibrary to GLLibraryEGL

This should lay the groundwork for more invasive architectural changes such as changing of inheritance trees etc.
Comment on attachment 603052 [details] [diff] [review]
Bug 732865 - Refactor GLContext classes so that they are more self contained

r+ assuming there are no diffs within the move :)
Attachment #603052 - Flags: review?(bgirard) → review+
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 603474 [details] [diff] [review]
Updated patch that shouldn't break Win32 or OS X now.

Review: :joedrew!
Attachment #603052 - Attachment is obsolete: true
Attachment #603474 - Flags: review?(bgirard)
Attachment #603052 - Flags: review?(joe)
(Assignee)

Updated

5 years ago
Attachment #603474 - Flags: review?(joe)
Comment on attachment 603474 [details] [diff] [review]
Updated patch that shouldn't break Win32 or OS X now.

Review of attachment 603474 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLLibraryEGL.cpp
@@ +3,5 @@
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> + *
> + * The contents of this file are subject to the Mozilla Public License Version
> + * 1.1 (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at

MPL2 please.

::: gfx/gl/GLLibraryEGL.h
@@ +4,5 @@
> + *
> + * The contents of this file are subject to the Mozilla Public License Version
> + * 1.1 (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + * http://www.mozilla.org/MPL/

MPL2 please.

@@ +14,5 @@
> + *
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + *   Mozilla Corporation.

FYI, the Initial Developer is Mozilla Foundation for employees of the Corporation, though it's not relevant with the MPL2 boilerplate.

@@ +63,5 @@
> +
> +#define EGL_LIB "libEGL.so"
> +#define GLES2_LIB "libGLESv2.so"
> +#define EGL_LIB1 "libEGL.so.1"
> +#define GLES2_LIB2 "libGLESv2.so.2"

A lot of this stuff feels like it should be in the .cpp file.

@@ +114,5 @@
> +#define AFTER_GL_CALL do {           \
> +    AfterGLCall(MOZ_FUNCTION_NAME);  \
> +} while (0)
> +
> +static void BeforeGLCall(const char* glFunction)

These should be "static inline", but I'd prefer them even more in the .cpp file as they used to be.

::: gfx/gl/GLLibraryLoader.cpp
@@ +1,3 @@
> +/* -*- Mode: c++; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40; -*- */
> +/* ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL2

::: gfx/gl/GLLibraryLoader.h
@@ +7,5 @@
> + * the License. You may obtain a copy of the License at
> + * http://www.mozilla.org/MPL/
> + *
> + * Software distributed under the License is distributed on an "AS IS" basis,
> + * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License

MPL2
Attachment #603474 - Flags: review?(joe) → review+
Comment on attachment 603474 [details] [diff] [review]
Updated patch that shouldn't break Win32 or OS X now.

Nice!
Attachment #603474 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/aad5bc8b3433
(Assignee)

Updated

5 years ago
Assignee: nobody → gwright
Target Milestone: --- → Future
OK, last patch nuked it. Current patch should be fine (just ran through try and it looks ok).

https://hg.mozilla.org/integration/mozilla-inbound/rev/35bf4deefc88
https://hg.mozilla.org/mozilla-central/rev/aad5bc8b3433
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla14
https://hg.mozilla.org/mozilla-central/rev/372f90787ec8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/35bf4deefc88
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 736802
You need to log in before you can comment on or make changes to this bug.