Last Comment Bug 732865 - Clean up the GLContextProvider classes
: Clean up the GLContextProvider classes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: George Wright (:gw280) (:gwright)
:
:
Mentors:
Depends on: 736802
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-04 18:22 PST by George Wright (:gw280) (:gwright)
Modified: 2012-03-17 22:33 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Bug 732865 - Refactor GLContext classes so that they are more self contained (90.61 KB, patch)
2012-03-05 14:11 PST, George Wright (:gw280) (:gwright)
b56girard: review+
Details | Diff | Splinter Review
Updated patch that shouldn't break Win32 or OS X now. (91.41 KB, patch)
2012-03-06 14:39 PST, George Wright (:gw280) (:gwright)
b56girard: review+
joe: review+
Details | Diff | Splinter Review

Description George Wright (:gw280) (:gwright) 2012-03-04 18:22:54 PST
The source structure for these files is a mess. We should actively start trying to clean them up.
Comment 1 George Wright (:gw280) (:gwright) 2012-03-04 19:10:04 PST
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.
Comment 2 George Wright (:gw280) (:gwright) 2012-03-05 14:11:53 PST
Created attachment 603052 [details] [diff] [review]
Bug 732865 - Refactor GLContext classes so that they are more self contained

Review: :joedrew!
Comment 3 George Wright (:gw280) (:gwright) 2012-03-05 14:22:53 PST
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 4 Benoit Girard (:BenWa) 2012-03-05 15:15:38 PST
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 :)
Comment 5 George Wright (:gw280) (:gwright) 2012-03-06 14:39:25 PST
Created attachment 603474 [details] [diff] [review]
Updated patch that shouldn't break Win32 or OS X now.

Review: :joedrew!
Comment 6 Joe Drew (not getting mail) 2012-03-08 12:07:59 PST
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
Comment 7 Benoit Girard (:BenWa) 2012-03-08 12:40:49 PST
Comment on attachment 603474 [details] [diff] [review]
Updated patch that shouldn't break Win32 or OS X now.

Nice!
Comment 8 George Wright (:gw280) (:gwright) 2012-03-16 11:34:36 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/aad5bc8b3433
Comment 9 George Wright (:gw280) (:gwright) 2012-03-16 15:26:58 PDT
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
Comment 10 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-17 17:07:14 PDT
https://hg.mozilla.org/mozilla-central/rev/aad5bc8b3433
Comment 11 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-17 17:08:20 PDT
https://hg.mozilla.org/mozilla-central/rev/372f90787ec8
Comment 12 Phil Ringnalda (:philor) 2012-03-17 17:29:49 PDT
https://hg.mozilla.org/mozilla-central/rev/35bf4deefc88

Note You need to log in before you can comment on or make changes to this bug.